Message-ID: <Y5b5lsUfZqeNBSss@gvm01>
Date:   Mon, 12 Dec 2022 10:51:18 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        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.
Fair enough, I'll change this. However, I would suggest to write these
kind of rules (about following the new style vs keeping consistency with
what it's there already) to help newcomers like me understanding what
the policy actually is. I got different opinions about that.

> 
> > 
> > > 
> > > > + * @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.
Okay.

> > 
> > > 
> > > > + */
> > > > +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.
> 
Russle, I did not actually ignore your comment. Looking back at the
history, you were commenting on the functions in plca.c and we were
talking about the global rtnl lock.

So here what it looks to me:
1. in a previous version of the patchset, the phy_ethtool_*_plca_*
functions were exported, therefore the phydev lock had to be acquired as
you cannot say from which context exactly these could be called.

2. After your comments about NOT exporting these functions, I believe we
can safely assume these are called only from plca.c (ethtool interface).

3. As you can see all of these functions are called with the rtnl lock
being held from ethtool.

Therefore... do we really need to take the phydev mutex at all?
Do we really need to perform (again) checks against phydev being not
NULL and such?

I would suggest to just remove all of these checks and the phydev mutex
locking, unless you see any reason for whcih we should take it.

Thoughts?


> > > 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) ?
Fair enough, as I said, this makes sesne to me, I just wanted to be sure
we agree on the purpose.

I moved the memset to the upper layers.

Thanks!
Piergiorgio

Powered by blists - more mailing lists