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 <[email protected]>
---
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
+ *
+ * @phydev: the phy_device struct
+ * @plca_cfg: where to store the retrieved configuration
+ */
+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);
+ ret = phydev->drv->get_plca_cfg(phydev, plca_cfg);
+
+ if (ret)
+ goto out_drv;
+
+out_drv:
+ 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;
+
+ 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);
+ memset(curr_plca_cfg, 0xFF, sizeof(*curr_plca_cfg));
+
+ mutex_lock(&phydev->lock);
+
+ 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;
+
+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);
+ ret = phydev->drv->get_plca_status(phydev, plca_st);
+
+ if (ret)
+ goto out_drv;
+
+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
Hi Piergiorgio,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Piergiorgio-Beruto/add-PLCA-RS-support-and-onsemi-NCN26000/20221211-064827
patch link: https://lore.kernel.org/r/75cb0eab15e62fc350e86ba9e5b0af72ea45b484.1670712151.git.piergiorgio.beruto%40gmail.com
patch subject: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA
config: m68k-randconfig-s043-20221211
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/333e7a2a5e52756e9320c13b6fbe1f5e9aef65d4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Piergiorgio-Beruto/add-PLCA-RS-support-and-onsemi-NCN26000/20221211-064827
git checkout 333e7a2a5e52756e9320c13b6fbe1f5e9aef65d4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=m68k SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/phy.c:593:46: sparse: sparse: Using plain integer as NULL pointer
vim +593 drivers/net/phy/phy.c
580
581 /**
582 * phy_ethtool_set_plca_cfg - Set PLCA RS configuration
583 *
584 * @phydev: the phy_device struct
585 * @extack: extack for reporting useful error messages
586 * @plca_cfg: new PLCA configuration to apply
587 */
588 int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
589 const struct phy_plca_cfg *plca_cfg,
590 struct netlink_ext_ack *extack)
591 {
592 int ret;
> 593 struct phy_plca_cfg *curr_plca_cfg = 0;
594
595 if (!phydev->drv) {
596 ret = -EIO;
597 goto out;
598 }
599
600 if (!phydev->drv->set_plca_cfg ||
601 !phydev->drv->get_plca_cfg) {
602 ret = -EOPNOTSUPP;
603 goto out;
604 }
605
606 curr_plca_cfg = kmalloc(sizeof(*curr_plca_cfg), GFP_KERNEL);
607 memset(curr_plca_cfg, 0xFF, sizeof(*curr_plca_cfg));
608
609 mutex_lock(&phydev->lock);
610
611 ret = phydev->drv->get_plca_cfg(phydev, curr_plca_cfg);
612 if (ret)
613 goto out_drv;
614
615 if (curr_plca_cfg->enabled < 0 && plca_cfg->enabled >= 0) {
616 NL_SET_ERR_MSG(extack,
617 "PHY does not support changing the PLCA 'enable' attribute");
618 ret = -EINVAL;
619 goto out_drv;
620 }
621
622 if (curr_plca_cfg->node_id < 0 && plca_cfg->node_id >= 0) {
623 NL_SET_ERR_MSG(extack,
624 "PHY does not support changing the PLCA 'local node ID' attribute");
625 ret = -EINVAL;
626 goto out_drv;
627 }
628
629 if (curr_plca_cfg->node_cnt < 0 && plca_cfg->node_cnt >= 0) {
630 NL_SET_ERR_MSG(extack,
631 "PHY does not support changing the PLCA 'node count' attribute");
632 ret = -EINVAL;
633 goto out_drv;
634 }
635
636 if (curr_plca_cfg->to_tmr < 0 && plca_cfg->to_tmr >= 0) {
637 NL_SET_ERR_MSG(extack,
638 "PHY does not support changing the PLCA 'TO timer' attribute");
639 ret = -EINVAL;
640 goto out_drv;
641 }
642
643 if (curr_plca_cfg->burst_cnt < 0 && plca_cfg->burst_cnt >= 0) {
644 NL_SET_ERR_MSG(extack,
645 "PHY does not support changing the PLCA 'burst count' attribute");
646 ret = -EINVAL;
647 goto out_drv;
648 }
649
650 if (curr_plca_cfg->burst_tmr < 0 && plca_cfg->burst_tmr >= 0) {
651 NL_SET_ERR_MSG(extack,
652 "PHY does not support changing the PLCA 'burst timer' attribute");
653 ret = -EINVAL;
654 goto out_drv;
655 }
656
657 // if enabling PLCA, perform additional sanity checks
658 if (plca_cfg->enabled > 0) {
659 if (!linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
660 phydev->advertising)) {
661 ret = -EOPNOTSUPP;
662 NL_SET_ERR_MSG(extack,
663 "Point to Multi-Point mode is not enabled");
664 }
665
666 // allow setting node_id concurrently with enabled
667 if (plca_cfg->node_id >= 0)
668 curr_plca_cfg->node_id = plca_cfg->node_id;
669
670 if (curr_plca_cfg->node_id >= 255) {
671 NL_SET_ERR_MSG(extack, "PLCA node ID is not set");
672 ret = -EINVAL;
673 goto out_drv;
674 }
675 }
676
677 ret = phydev->drv->set_plca_cfg(phydev, plca_cfg);
678 if (ret)
679 goto out_drv;
680
681 out_drv:
682 kfree(curr_plca_cfg);
683 mutex_unlock(&phydev->lock);
684 out:
685 return ret;
686 }
687
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
> ---
> 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!
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 <[email protected]>
> > ---
> > 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
On Sun, Dec 11, 2022 at 08:03:15PM +0100, Piergiorgio Beruto wrote:
> 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 <[email protected]>
> > > ---
> > > 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.
Please see Documentation/doc-guide/kernel-doc.rst
"Function parameters
~~~~~~~~~~~~~~~~~~~
Each function argument should be described in order, immediately following
the short function description. Do not leave a blank line between the
function description and the arguments, nor between the arguments."
Note the last sentence - there should _not_ be a blank line, so please
follow this for new submissions. I don't think we care enough to fix
what's already there though.
>
> >
> > > + * @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?
Convention is a blank line - as illustrated by the general format
in the documentation file I refer to above.
>
> >
> > > + */
> > > +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?
This is a review comment I've made already, but you seem to have ignored
it. Please ensure that new contributions are safe. Yes, existing code
may not be, and that's something we should fix, but your contribution
should at least be safer than the existing code.
> > 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?
... because of what I said in the following line below.
>
> > 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.
Why wouldn't they want this initialisation, given that the use of
negative values means "not implemented" - surely we want common code
to indicate everything is not implemented until something writes to
the parameter?
What if an implementation decides to manually initialise each member
to -1 and then someone adds an additional field later (e.g. for that
0x0A) ?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Dec 11, 2022 at 08:34:14PM +0000, Russell King (Oracle) wrote:
> On Sun, Dec 11, 2022 at 08:03:15PM +0100, Piergiorgio Beruto wrote:
> > 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 <[email protected]>
> > > > ---
> > > > 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.
>
> Please see Documentation/doc-guide/kernel-doc.rst
>
> "Function parameters
> ~~~~~~~~~~~~~~~~~~~
>
> Each function argument should be described in order, immediately following
> the short function description. Do not leave a blank line between the
> function description and the arguments, nor between the arguments."
>
> Note the last sentence - there should _not_ be a blank line, so please
> follow this for new submissions. I don't think we care enough to fix
> what's already there though.
Fair enough, I'll change this. However, I would suggest to write these
kind of rules (about following the new style vs keeping consistency with
what it's there already) to help newcomers like me understanding what
the policy actually is. I got different opinions about that.
>
> >
> > >
> > > > + * @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?
>
> Convention is a blank line - as illustrated by the general format
> in the documentation file I refer to above.
Okay.
> >
> > >
> > > > + */
> > > > +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?
>
> This is a review comment I've made already, but you seem to have ignored
> it. Please ensure that new contributions are safe. Yes, existing code
> may not be, and that's something we should fix, but your contribution
> should at least be safer than the existing code.
>
Russle, I did not actually ignore your comment. Looking back at the
history, you were commenting on the functions in plca.c and we were
talking about the global rtnl lock.
So here what it looks to me:
1. in a previous version of the patchset, the phy_ethtool_*_plca_*
functions were exported, therefore the phydev lock had to be acquired as
you cannot say from which context exactly these could be called.
2. After your comments about NOT exporting these functions, I believe we
can safely assume these are called only from plca.c (ethtool interface).
3. As you can see all of these functions are called with the rtnl lock
being held from ethtool.
Therefore... do we really need to take the phydev mutex at all?
Do we really need to perform (again) checks against phydev being not
NULL and such?
I would suggest to just remove all of these checks and the phydev mutex
locking, unless you see any reason for whcih we should take it.
Thoughts?
> > > 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.
>
> Why wouldn't they want this initialisation, given that the use of
> negative values means "not implemented" - surely we want common code
> to indicate everything is not implemented until something writes to
> the parameter?
>
> What if an implementation decides to manually initialise each member
> to -1 and then someone adds an additional field later (e.g. for that
> 0x0A) ?
Fair enough, as I said, this makes sesne to me, I just wanted to be sure
we agree on the purpose.
I moved the memset to the upper layers.
Thanks!
Piergiorgio
On Sun, Dec 11, 2022 at 08:34:14PM +0000, Russell King (Oracle) wrote:
> On Sun, Dec 11, 2022 at 08:03:15PM +0100, Piergiorgio Beruto wrote:
> > 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 <[email protected]>
> > > > ---
> > > > 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.
>
> Please see Documentation/doc-guide/kernel-doc.rst
>
> "Function parameters
> ~~~~~~~~~~~~~~~~~~~
>
> Each function argument should be described in order, immediately following
> the short function description. Do not leave a blank line between the
> function description and the arguments, nor between the arguments."
>
> Note the last sentence - there should _not_ be a blank line, so please
> follow this for new submissions. I don't think we care enough to fix
> what's already there though.
>
> >
> > >
> > > > + * @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?
>
> Convention is a blank line - as illustrated by the general format
> in the documentation file I refer to above.
>
> >
> > >
> > > > + */
> > > > +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;
> > 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?
>
> This is a review comment I've made already, but you seem to have ignored
> it. Please ensure that new contributions are safe. Yes, existing code
> may not be, and that's something we should fix, but your contribution
> should at least be safer than the existing code.
I have a patch ready for fixing the cable test examples of performing
the test before taking the lock. I was going to post it to net-next in
a couple of weeks time. Or do you think it should be to net?
Andrew
On Mon, Dec 12, 2022 at 10:51:18AM +0100, Piergiorgio Beruto wrote:
> On Sun, Dec 11, 2022 at 08:34:14PM +0000, Russell King (Oracle) wrote:
> > Please see Documentation/doc-guide/kernel-doc.rst
> >
> > "Function parameters
> > ~~~~~~~~~~~~~~~~~~~
> >
> > Each function argument should be described in order, immediately following
> > the short function description. Do not leave a blank line between the
> > function description and the arguments, nor between the arguments."
> >
> > Note the last sentence - there should _not_ be a blank line, so please
> > follow this for new submissions. I don't think we care enough to fix
> > what's already there though.
> Fair enough, I'll change this. However, I would suggest to write these
> kind of rules (about following the new style vs keeping consistency with
> what it's there already) to help newcomers like me understanding what
> the policy actually is. I got different opinions about that.
phy.c has two different formats for docbook comments - some of them
are to the documented format, others with the extra blank line. Given
that correct form is without the blank line, new docbook comments
should conform to the standard format.
> > This is a review comment I've made already, but you seem to have ignored
> > it. Please ensure that new contributions are safe. Yes, existing code
> > may not be, and that's something we should fix, but your contribution
> > should at least be safer than the existing code.
> >
> Russle, I did not actually ignore your comment. Looking back at the
> history, you were commenting on the functions in plca.c and we were
> talking about the global rtnl lock.
Thanks for checking back, you're correct. You can ignore this comment
as it won't make any difference.
Looking at phy_remove(), it wouldn't make any difference anyway, so
please keep the code as-is.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!