Messages in this thread Patch in this message |  | | Date | Sun, 11 Dec 2022 20:03:15 +0100 | From | Piergiorgio Beruto <> | 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@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
|  |