netdev - Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA

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]

Date:   Mon, 12 Dec 2022 16:22:51 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between
 ethtool and phylib for PLCA

On Sun, Dec 11, 2022 at 08:34:14PM +0000, Russell King (Oracle) wrote:
> On Sun, Dec 11, 2022 at 08:03:15PM +0100, Piergiorgio Beruto wrote:
> > On Sun, Dec 11, 2022 at 12:23:53PM +0000, Russell King (Oracle) wrote:
> > > On Sat, Dec 10, 2022 at 11:46:39PM +0100, Piergiorgio Beruto wrote:
> > > > This patch adds the required connection between netlink ethtool and
> > > > phylib to resolve PLCA get/set config and get status messages.
> > > > 
> > > > Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@...il.com>
> > > > ---
> > > >  drivers/net/phy/phy.c        | 175 +++++++++++++++++++++++++++++++++++
> > > >  drivers/net/phy/phy_device.c |   3 +
> > > >  include/linux/phy.h          |   7 ++
> > > >  3 files changed, 185 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > > > index e5b6cb1a77f9..40d90ed2f0fb 100644
> > > > --- a/drivers/net/phy/phy.c
> > > > +++ b/drivers/net/phy/phy.c
> > > > @@ -543,6 +543,181 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
> > > >  }
> > > >  EXPORT_SYMBOL(phy_ethtool_get_stats);
> > > >  
> > > > +/**
> > > > + * phy_ethtool_get_plca_cfg - Get PLCA RS configuration
> > > > + *
> > > 
> > > You shouldn't have an empty line in the comment here
> > I was trying to follow the style of this file. All other functions start
> > like this, including an empty line. Do you want me to:
> > a) follow your indication and leave all other functions as they are?
> > b) Change all functions docs to follow your suggestion?
> > c) leave it as-is?
> > 
> > Please, advise.
> 
> Please see Documentation/doc-guide/kernel-doc.rst
> 
> "Function parameters
> ~~~~~~~~~~~~~~~~~~~
> 
> Each function argument should be described in order, immediately following
> the short function description.  Do not leave a blank line between the
> function description and the arguments, nor between the arguments."
> 
> Note the last sentence - there should _not_ be a blank line, so please
> follow this for new submissions. I don't think we care enough to fix
> what's already there though.
> 
> > 
> > > 
> > > > + * @phydev: the phy_device struct
> > > > + * @plca_cfg: where to store the retrieved configuration
> > > 
> > > Maybe have an empty line, followed by a bit of text describing what this
> > > function does and the return codes it generates?
> > Again, I was trying to follow the style of the docs in this file.
> > Do you still want me to add a description here?
> 
> Convention is a blank line - as illustrated by the general format
> in the documentation file I refer to above.
> 
> > 
> > > 
> > > > + */
> > > > +int phy_ethtool_get_plca_cfg(struct phy_device *phydev,
> > > > +			     struct phy_plca_cfg *plca_cfg)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (!phydev->drv) {
> > > > +		ret = -EIO;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (!phydev->drv->get_plca_cfg) {
> > > > +		ret = -EOPNOTSUPP;
> > Once more, all other functions in this file take the mutex -after-
> > checking for phydev->drv and checking the specific function. Therefore,
> > I assumed that was a safe thing to do. If not, should we fix all of
> > these functions in this file?
> 
> This is a review comment I've made already, but you seem to have ignored
> it. Please ensure that new contributions are safe. Yes, existing code
> may not be, and that's something we should fix, but your contribution
> should at least be safer than the existing code.

I have a patch ready for fixing the cable test examples of performing
the test before taking the lock. I was going to post it to net-next in
a couple of weeks time. Or do you think it should be to net?

	Andrew