netdev - Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200621234431.GZ1551@shell.armlinux.org.uk>
Date:   Mon, 22 Jun 2020 00:44:31 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Colton Lewis <colton.w.lewis@...tonmail.com>
Cc:     Andrew Lunn <andrew@...n.ch>, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc
 inconsistencies

On Sun, Jun 21, 2020 at 11:02:30PM +0000, Colton Lewis wrote:
> On Sunday, June 21, 2020 10:53:45 AM CDT Russell King - ARM Linux admin wrote:
> > > ---
> > >   */
> > >  struct phylink_config {
> > >  	struct device *dev;
> > > @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
> > >   *
> > >   * For most 10GBASE-R, there is no advertisement.
> > >   */
> > > -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> > > +int *pcs_config(struct phylink_config *config, unsigned int mode,
> > >  		  phy_interface_t interface, const unsigned long *advertising);
> > 
> > *Definitely* a NAK on this and two changes below.  You're changing the
> > function signature to be incorrect.  If the documentation can't parse
> > a legitimate C function pointer declaration and allow it to be
> > documented, then that's a problem with the documentation's parsing of
> > C code, rather than a problem with the C code itself.
> 
> I realize this changes the signature, but this declaration is not compiled. It is under an #if 0 with a comment stating it exists for kernel-doc purposes only. The *real* function pointer declaration exists in struct phylink_pcs_ops.
> 
> Given the declaration is there exclusively for documentation, it makes sense to change it so the documentation system can parse it.

My objection is that you are changing the return type from (e.g.)
"int" to "int *", which will then end up in the documentation as
such, and the documentation will, therefore, be incorrect.

I have subsequently realised that I didn't follow my own pattern
for documenting phylink_mac_ops - a correct solution would be to
drop the parens _and_ the "*" preceding the function name, so:

int pcs_config(struct phylink_config *config, unsigned int mode,
...

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!