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 12:23:53 +0000 From: "Russell King (Oracle)" <linux@...linux.org.uk> To: Piergiorgio Beruto <piergiorgio.beruto@...il.com> 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 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 > + * @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? > + */ > +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? Maybe the memset() should be done by plca_get_cfg_prepare_data()? Wouldn't all implementations need to memset this to 0xff? Also, lower-case 0xff please. > + 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.) > + 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. > + > + 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? > + 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. > + > + ret = phydev->drv->get_plca_cfg(phydev, curr_plca_cfg); > + if (ret) > + goto out_drv; > + > + if (curr_plca_cfg->enabled < 0 && plca_cfg->enabled >= 0) { > + NL_SET_ERR_MSG(extack, > + "PHY does not support changing the PLCA 'enable' attribute"); > + ret = -EINVAL; > + goto out_drv; > + } > + > + if (curr_plca_cfg->node_id < 0 && plca_cfg->node_id >= 0) { > + NL_SET_ERR_MSG(extack, > + "PHY does not support changing the PLCA 'local node ID' attribute"); > + ret = -EINVAL; > + goto out_drv; > + } > + > + if (curr_plca_cfg->node_cnt < 0 && plca_cfg->node_cnt >= 0) { > + NL_SET_ERR_MSG(extack, > + "PHY does not support changing the PLCA 'node count' attribute"); > + ret = -EINVAL; > + goto out_drv; > + } > + > + if (curr_plca_cfg->to_tmr < 0 && plca_cfg->to_tmr >= 0) { > + NL_SET_ERR_MSG(extack, > + "PHY does not support changing the PLCA 'TO timer' attribute"); > + ret = -EINVAL; > + goto out_drv; > + } > + > + if (curr_plca_cfg->burst_cnt < 0 && plca_cfg->burst_cnt >= 0) { > + NL_SET_ERR_MSG(extack, > + "PHY does not support changing the PLCA 'burst count' attribute"); > + ret = -EINVAL; > + goto out_drv; > + } > + > + if (curr_plca_cfg->burst_tmr < 0 && plca_cfg->burst_tmr >= 0) { > + NL_SET_ERR_MSG(extack, > + "PHY does not support changing the PLCA 'burst timer' attribute"); > + ret = -EINVAL; > + goto out_drv; > + } > + > + // if enabling PLCA, perform additional sanity checks > + if (plca_cfg->enabled > 0) { > + if (!linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT, > + phydev->advertising)) { > + ret = -EOPNOTSUPP; > + NL_SET_ERR_MSG(extack, > + "Point to Multi-Point mode is not enabled"); > + } > + > + // allow setting node_id concurrently with enabled > + if (plca_cfg->node_id >= 0) > + curr_plca_cfg->node_id = plca_cfg->node_id; > + > + if (curr_plca_cfg->node_id >= 255) { > + NL_SET_ERR_MSG(extack, "PLCA node ID is not set"); > + ret = -EINVAL; > + goto out_drv; > + } > + } > + > + ret = phydev->drv->set_plca_cfg(phydev, plca_cfg); > + if (ret) > + goto out_drv; Unnecessary if() statement. > + > +out_drv: > + kfree(curr_plca_cfg); > + mutex_unlock(&phydev->lock); > +out: > + return ret; > +} > + > +/** > + * phy_ethtool_get_plca_status - Get PLCA RS status information > + * > + * @phydev: the phy_device struct > + * @plca_st: where to store the retrieved status information > + */ > +int phy_ethtool_get_plca_status(struct phy_device *phydev, > + struct phy_plca_status *plca_st) > +{ > + int ret; > + > + if (!phydev->drv) { > + ret = -EIO; > + goto out; > + } > + > + if (!phydev->drv->get_plca_status) { > + ret = -EOPNOTSUPP; > + goto out; > + } > + > + mutex_lock(&phydev->lock); Same comment here. > + ret = phydev->drv->get_plca_status(phydev, plca_st); > + > + if (ret) > + goto out_drv; And here. > + > +out_drv: > + mutex_unlock(&phydev->lock); > +out: > + return ret; > +} > + > /** > * phy_start_cable_test - Start a cable test > * > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 8e48b3cec5e7..44bd06be9691 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -3276,6 +3276,9 @@ static const struct ethtool_phy_ops phy_ethtool_phy_ops = { > .get_sset_count = phy_ethtool_get_sset_count, > .get_strings = phy_ethtool_get_strings, > .get_stats = phy_ethtool_get_stats, > + .get_plca_cfg = phy_ethtool_get_plca_cfg, > + .set_plca_cfg = phy_ethtool_set_plca_cfg, > + .get_plca_status = phy_ethtool_get_plca_status, > .start_cable_test = phy_start_cable_test, > .start_cable_test_tdr = phy_start_cable_test_tdr, > }; > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 2a5c2d3a5da5..e0dcd534fe6f 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -1845,6 +1845,13 @@ int phy_ethtool_get_strings(struct phy_device *phydev, u8 *data); > int phy_ethtool_get_sset_count(struct phy_device *phydev); > int phy_ethtool_get_stats(struct phy_device *phydev, > struct ethtool_stats *stats, u64 *data); > +int phy_ethtool_get_plca_cfg(struct phy_device *phydev, > + struct phy_plca_cfg *plca_cfg); > +int phy_ethtool_set_plca_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg, > + struct netlink_ext_ack *extack); > +int phy_ethtool_get_plca_status(struct phy_device *phydev, > + struct phy_plca_status *plca_st); > > static inline int phy_package_read(struct phy_device *phydev, u32 regnum) > { > -- > 2.37.4 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists