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

[Russell King (Oracle)]

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@xxxxxxxxx>
> > > ---
> > >  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;
> > > +		goto out;
> > > +	}
> > > +
> > > +	memset(plca_cfg, 0xFF, sizeof(*plca_cfg));
> > > +
> > > +	mutex_lock(&phydev->lock);
> > 
> > Maybe move the memset() and mutex_lock() before the first if() statement
> > above? 
> 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.

> > Maybe the memset() should be done by plca_get_cfg_prepare_data()?
> I put the memset there when the function was exported. Since we're not
> exporting it anymore, we can put it in the _prepare() function in plca.c
> as you suggest. I just wonder if there is a real advantage in doing
> this?

... because of what I said in the following line below.

> 
> > Wouldn't all implementations need to memset this to 0xff?
> It actually depends on what these implementations are trying to achieve.
> I would say, likely yes, but not necessairly.

Why wouldn't they want this initialisation, given that the use of
negative values means "not implemented" - surely we want common code
to indicate everything is not implemented until something writes to
the parameter?

What if an implementation decides to manually initialise each member
to -1 and then someone adds an additional field later (e.g. for that
0x0A) ?

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