[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y5Ypc5fDP3Cbi+RZ@gvm01>
Date: Sun, 11 Dec 2022 20:03:15 +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 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.
>
> > + * @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
Powered by blists - more mailing lists