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

[Piergiorgio Beruto <piergiorgio.beruto@gmail.com>]

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.

> 
> > + * @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?

> 
> > + */
> > +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?

> 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?

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

> 
> Also, lower-case 0xff please.
Done.

> 
> > +	ret = phydev->drv->get_plca_cfg(phydev, plca_cfg);
> > +
> > +	if (ret)
> > +		goto out_drv;
> > +
> > +out_drv:
> 
> This if() and out_drv label seems unused (although with the above
> suggested change, you will need to move the "out" label here.)
Noted, thanks.
> 
> > +	mutex_unlock(&phydev->lock);
> > +out:
> > +	return ret;
> > +}
> > +
> > +/**
> > + * phy_ethtool_set_plca_cfg - Set PLCA RS configuration
> > + *
> > + * @phydev: the phy_device struct
> > + * @extack: extack for reporting useful error messages
> > + * @plca_cfg: new PLCA configuration to apply
> > + */
> > +int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
> > +			     const struct phy_plca_cfg *plca_cfg,
> > +			     struct netlink_ext_ack *extack)
> > +{
> > +	int ret;
> > +	struct phy_plca_cfg *curr_plca_cfg = 0;
> 
> Unnecessary initialiser. Also, reverse Christmas-tree please.
Oops, that was not intentional. Fixed.

> > +
> > +	if (!phydev->drv) {
> > +		ret = -EIO;
> > +		goto out;
> > +	}
> > +
> > +	if (!phydev->drv->set_plca_cfg ||
> > +	    !phydev->drv->get_plca_cfg) {
> > +		ret = -EOPNOTSUPP;
> > +		goto out;
> > +	}
> > +
> > +	curr_plca_cfg = kmalloc(sizeof(*curr_plca_cfg), GFP_KERNEL);
> 
> What if kmalloc() returns NULL?
Fixed, returning -ENOMEM now.

> 
> > +	memset(curr_plca_cfg, 0xFF, sizeof(*curr_plca_cfg));
> > +
> > +	mutex_lock(&phydev->lock);
> 
> Consider moving the above three to the beginning of the function so
> phydev->drv is checked under the mutex.
Same discussion as before. No other functions in this file do this. Let
me know how would you like to see this fixed.

> > +
> > +	ret = phydev->drv->set_plca_cfg(phydev, plca_cfg);
> > +	if (ret)
> > +		goto out_drv;
> 
> Unnecessary if() statement.
Yup, fixed.

> > +	ret = phydev->drv->get_plca_status(phydev, plca_st);
> > +
> > +	if (ret)
> > +		goto out_drv;
> 
> And here.
Fixed.


Please, let me know how to proceed.
Thanks again for your kind review.

Piergiorgio