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

From: Andrew Lunn <andrew@lunn.ch>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	Oleksij Rempel <o.rempel@pengutronix.de>
Subject: Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA
Date: Mon, 12 Dec 2022 16:22:51 +0100	[thread overview]
Message-ID: <Y5dHS8jJkxS+aS/Q@lunn.ch> (raw)
In-Reply-To: <Y5Y+xu4Rk6ptCERg@shell.armlinux.org.uk>

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@gmail.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