lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
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