[PATCH net-next 0/4] net: ethtool: Untangle PHYLIB dependency

[PATCH net-next 0/4] net: ethtool: Untangle PHYLIB dependency

[Florian Fainelli]

Hi all,

This patch series untangles the ethtool netlink dependency with PHYLIB
which exists because the cable test feature calls directly into PHY
library functions. The approach taken here is to utilize a new set of
net_device_ops function pointers which are automatically set to the PHY
library variants when a network device driver attaches to a PHY device.

Florian Fainelli (4):
  net: Add cable test netdevice operations
  net: phy: Change cable test arguments to net_device
  net: phy: Automatically set-up cable test netdev_ops
  net: ethtool: Remove PHYLIB dependency

 drivers/net/phy/phy.c        | 18 ++++++++++++++----
 drivers/net/phy/phy_device.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/netdevice.h    | 14 ++++++++++++++
 include/linux/phy.h          | 10 ++++++----
 net/Kconfig                  |  1 -
 net/ethtool/cabletest.c      | 12 ++++++++----
 6 files changed, 74 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH net-next 1/4] net: Add cable test netdevice operations

[Florian Fainelli]

In preparation for decoupling the ethtool cable test from the PHY
library, add definitions for two new network device operations:

* ndo_cable_test_start
* ndo_cable_test_tdr_start

In a subsequent patch we will start making use of those.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/netdevice.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 39e28e11863c..43f640579973 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -936,6 +936,8 @@ struct dev_ifalias {
 
 struct devlink;
 struct tlsdev_ops;
+struct phy_tdr_config;
+struct netlink_ext_ack;
 
 struct netdev_name_node {
 	struct hlist_node hlist;
@@ -1278,6 +1280,13 @@ struct netdev_net_notifier {
  * int (*ndo_tunnel_ctl)(struct net_device *dev, struct ip_tunnel_parm *p,
  *			 int cmd);
  *	Add, change, delete or get information on an IPv4 tunnel.
+ * int (*ndo_cable_test_start)(struct net_device *dev,
+ *			       struct netlink_ext_ack *extack);
+ *	Start a cable test.
+ * int (*ndo_cable_test_tdr_start)(struct net_device *dev,
+ *				   struct netlink_ext_ack *extack,
+ *				   const struct phy_tdr_config *config);
+ *	Start a raw TDR (Time Domain Reflectometry) cable test.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);
@@ -1485,6 +1494,11 @@ struct net_device_ops {
 	struct devlink_port *	(*ndo_get_devlink_port)(struct net_device *dev);
 	int			(*ndo_tunnel_ctl)(struct net_device *dev,
 						  struct ip_tunnel_parm *p, int cmd);
+	int			(*ndo_cable_test_start)(struct net_device *dev,
+							struct netlink_ext_ack *exact);
+	int			(*ndo_cable_test_tdr_start)(struct net_device *dev,
+							    struct netlink_ext_ack *exact,
+							    const struct phy_tdr_config *config);
 };
 
 /**
-- 
2.25.1

[PATCH net-next 2/4] net: phy: Change cable test arguments to net_device

[Florian Fainelli]

In order to untangle the ethtool/cabletest feature with the PHY library,
make the PHY library functions take a net_device argument and derive the
phy_device reference from there.

No functional changes introduced.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c   | 18 ++++++++++++++----
 include/linux/phy.h     |  8 ++++----
 net/ethtool/cabletest.c |  4 ++--
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 56cfae950472..fbb74f37b961 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -489,12 +489,17 @@ static void phy_abort_cable_test(struct phy_device *phydev)
 		phydev_err(phydev, "Error while aborting cable test");
 }
 
-int phy_start_cable_test(struct phy_device *phydev,
+int phy_start_cable_test(struct net_device *dev,
 			 struct netlink_ext_ack *extack)
 {
-	struct net_device *dev = phydev->attached_dev;
+	struct phy_device *phydev = dev->phydev;
 	int err = -ENOMEM;
 
+	if (!dev->phydev) {
+		NL_SET_ERR_MSG(extack, "Network device not attached to a PHY");
+		return -EOPNOTSUPP;
+	}
+
 	if (!(phydev->drv &&
 	      phydev->drv->cable_test_start &&
 	      phydev->drv->cable_test_get_status)) {
@@ -552,13 +557,18 @@ int phy_start_cable_test(struct phy_device *phydev,
 }
 EXPORT_SYMBOL(phy_start_cable_test);
 
-int phy_start_cable_test_tdr(struct phy_device *phydev,
+int phy_start_cable_test_tdr(struct net_device *dev,
 			     struct netlink_ext_ack *extack,
 			     const struct phy_tdr_config *config)
 {
-	struct net_device *dev = phydev->attached_dev;
+	struct phy_device *phydev = dev->phydev;
 	int err = -ENOMEM;
 
+	if (!dev->phydev) {
+		NL_SET_ERR_MSG(extack, "Network device not attached to a PHY");
+		return -EOPNOTSUPP;
+	}
+
 	if (!(phydev->drv &&
 	      phydev->drv->cable_test_tdr_start &&
 	      phydev->drv->cable_test_get_status)) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 101a48fa6750..53b95c52869d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1266,21 +1266,21 @@ int phy_restart_aneg(struct phy_device *phydev);
 int phy_reset_after_clk_enable(struct phy_device *phydev);
 
 #if IS_ENABLED(CONFIG_PHYLIB)
-int phy_start_cable_test(struct phy_device *phydev,
+int phy_start_cable_test(struct net_device *dev,
 			 struct netlink_ext_ack *extack);
-int phy_start_cable_test_tdr(struct phy_device *phydev,
+int phy_start_cable_test_tdr(struct net_device *dev,
 			     struct netlink_ext_ack *extack,
 			     const struct phy_tdr_config *config);
 #else
 static inline
-int phy_start_cable_test(struct phy_device *phydev,
+int phy_start_cable_test(struct net_device *dev,
 			 struct netlink_ext_ack *extack)
 {
 	NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support");
 	return -EOPNOTSUPP;
 }
 static inline
-int phy_start_cable_test_tdr(struct phy_device *phydev,
+int phy_start_cable_test_tdr(struct net_device *dev,
 			     struct netlink_ext_ack *extack,
 			     const struct phy_tdr_config *config)
 {
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 7194956aa09e..0d940a91493b 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -85,7 +85,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = phy_start_cable_test(dev->phydev, info->extack);
+	ret = phy_start_cable_test(dev, info->extack);
 
 	ethnl_ops_complete(dev);
 
@@ -341,7 +341,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = phy_start_cable_test_tdr(dev->phydev, info->extack, &cfg);
+	ret = phy_start_cable_test_tdr(dev, info->extack, &cfg);
 
 	ethnl_ops_complete(dev);
 
-- 
2.25.1

[PATCH net-next 3/4] net: phy: Automatically set-up cable test netdev_ops

[Florian Fainelli]

Upon attach, override the net_device operations with the PHY library
cable test operations and conversely, upon detach, revert to the
original net_device operations.

This will allows us in a subsequent patch to finally decouple the
ethtool/cabletest from the PHY library hard depenencies.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index eb1068a77ce1..100d85541a06 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1246,6 +1246,36 @@ phy_standalone_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(phy_standalone);
 
+static int phy_setup_netdev_ops(struct net_device *dev)
+{
+	struct phy_device *phydev = dev->phydev;
+	struct net_device_ops *ops;
+
+	ops = devm_kzalloc(&phydev->mdio.dev, sizeof(*ops), GFP_KERNEL);
+	if (!ops)
+		return -ENOMEM;
+
+	phydev->orig_ndo_ops = dev->netdev_ops;
+	if (phydev->orig_ndo_ops)
+		memcpy(ops, phydev->orig_ndo_ops, sizeof(*ops));
+
+	ops->ndo_cable_test_start = phy_start_cable_test;
+	ops->ndo_cable_test_tdr_start = phy_start_cable_test_tdr;
+
+	dev->netdev_ops = ops;
+
+	return 0;
+}
+
+static void phy_teardown_netdev_ops(struct net_device *dev)
+{
+	struct phy_device *phydev = dev->phydev;
+
+	if (phydev->orig_ndo_ops)
+		dev->netdev_ops = phydev->orig_ndo_ops;
+	phydev->orig_ndo_ops = NULL;
+}
+
 /**
  * phy_sfp_attach - attach the SFP bus to the PHY upstream network device
  * @upstream: pointer to the phy device
@@ -1380,6 +1410,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	if (dev) {
 		phydev->attached_dev = dev;
 		dev->phydev = phydev;
+		phy_setup_netdev_ops(dev);
 
 		if (phydev->sfp_bus_attached)
 			dev->sfp_bus = phydev->sfp_bus;
@@ -1676,6 +1707,7 @@ void phy_detach(struct phy_device *phydev)
 
 	phy_suspend(phydev);
 	if (dev) {
+		phy_teardown_netdev_ops(dev);
 		phydev->attached_dev->phydev = NULL;
 		phydev->attached_dev = NULL;
 	}
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 53b95c52869d..04e35afa43ae 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -544,6 +544,8 @@ struct phy_device {
 	/* MACsec management functions */
 	const struct macsec_ops *macsec_ops;
 #endif
+	/* Original attached network device netdev_ops pointer */
+	const struct net_device_ops *orig_ndo_ops;
 };
 #define to_phy_device(d) container_of(to_mdio_device(d), \
 				      struct phy_device, mdio)
-- 
2.25.1

[PATCH net-next 4/4] net: ethtool: Remove PHYLIB dependency

[Florian Fainelli]

Now that we have converted the ethtool/cabletest code to use netdev_ops,
we can remove the PHY library dependency since the function pointers
will now be provided upon PHY attachment to the network device.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/Kconfig             |  1 -
 net/ethtool/cabletest.c | 12 ++++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/Kconfig b/net/Kconfig
index d1672280d6a4..3831206977a1 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -455,7 +455,6 @@ config FAILOVER
 config ETHTOOL_NETLINK
 	bool "Netlink interface for ethtool"
 	default y
-	depends on PHYLIB=y || PHYLIB=n
 	help
 	  An alternative userspace interface for ethtool based on generic
 	  netlink. It provides better extensibility and some new features,
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 0d940a91493b..d8e2eb427613 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
 	struct ethnl_req_info req_info = {};
+	const struct net_device_ops *ops;
 	struct net_device *dev;
 	int ret;
 
@@ -75,7 +76,8 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+	ops = dev->netdev_ops;
+	if (!ops->ndo_cable_test_start) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
@@ -85,7 +87,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = phy_start_cable_test(dev, info->extack);
+	ret = ops->ndo_cable_test_start(dev, info->extack);
 
 	ethnl_ops_complete(dev);
 
@@ -308,6 +310,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_TDR_MAX + 1];
 	struct ethnl_req_info req_info = {};
+	const struct net_device_ops *ops;
 	struct phy_tdr_config cfg;
 	struct net_device *dev;
 	int ret;
@@ -326,7 +329,8 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 
 	dev = req_info.dev;
-	if (!dev->phydev) {
+	ops = dev->netdev_ops;
+	if (!ops->ndo_cable_test_tdr_start) {
 		ret = -EOPNOTSUPP;
 		goto out_dev_put;
 	}
@@ -341,7 +345,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = phy_start_cable_test_tdr(dev, info->extack, &cfg);
+	ret = ops->ndo_cable_test_tdr_start(dev, info->extack, &cfg);
 
 	ethnl_ops_complete(dev);
 
-- 
2.25.1

Re: [PATCH net-next 0/4] net: ethtool: Untangle PHYLIB dependency

[Michal Kubecek]

On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:
> Hi all,
> 
> This patch series untangles the ethtool netlink dependency with PHYLIB
> which exists because the cable test feature calls directly into PHY
> library functions. The approach taken here is to utilize a new set of
> net_device_ops function pointers which are automatically set to the PHY
> library variants when a network device driver attaches to a PHY device.

I'm not sure about the idea of creating a copy of netdev_ops for each
device using phylib. First, there would be some overhead (just checked
my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
quite frequent pattern of comparing dev->netdev_ops against known
constants to check if a network device is of certain type; I can't say
for sure if it is also used with devices using phylib in existing code
but it feels risky.

As the two pointers are universal for all devices, couldn't we simply
use one global structure with them like we do for IPv6 (ipv6_stub) or
some netfilter modules (e.g. nf_ct_hook)?

Michal

> Florian Fainelli (4):
>   net: Add cable test netdevice operations
>   net: phy: Change cable test arguments to net_device
>   net: phy: Automatically set-up cable test netdev_ops
>   net: ethtool: Remove PHYLIB dependency
> 
>  drivers/net/phy/phy.c        | 18 ++++++++++++++----
>  drivers/net/phy/phy_device.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/netdevice.h    | 14 ++++++++++++++
>  include/linux/phy.h          | 10 ++++++----
>  net/Kconfig                  |  1 -
>  net/ethtool/cabletest.c      | 12 ++++++++----
>  6 files changed, 74 insertions(+), 13 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH net-next 0/4] net: ethtool: Untangle PHYLIB dependency

[Andrew Lunn]

On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote:
> On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:
> > Hi all,
> > 
> > This patch series untangles the ethtool netlink dependency with PHYLIB
> > which exists because the cable test feature calls directly into PHY
> > library functions. The approach taken here is to utilize a new set of
> > net_device_ops function pointers which are automatically set to the PHY
> > library variants when a network device driver attaches to a PHY device.
> 
> I'm not sure about the idea of creating a copy of netdev_ops for each
> device using phylib. First, there would be some overhead (just checked
> my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
> quite frequent pattern of comparing dev->netdev_ops against known
> constants to check if a network device is of certain type; I can't say
> for sure if it is also used with devices using phylib in existing code
> but it feels risky.

I agree with Michal here. I don't like this.

I think we need phylib to register a set of ops with ethtool when it
loads. It would also allow us to clean up phy_ethtool_get_strings(),
phy_ethtool_get_sset_count(), phy_ethtool_get_stats().

      Andrew

Re: [PATCH net-next 0/4] net: ethtool: Untangle PHYLIB dependency

[Jakub Kicinski]

On Thu, 2 Jul 2020 18:34:24 +0200 Andrew Lunn wrote:
> On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote:
> > On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:  
> > > Hi all,
> > > 
> > > This patch series untangles the ethtool netlink dependency with PHYLIB
> > > which exists because the cable test feature calls directly into PHY
> > > library functions. The approach taken here is to utilize a new set of
> > > net_device_ops function pointers which are automatically set to the PHY
> > > library variants when a network device driver attaches to a PHY device.  
> > 
> > I'm not sure about the idea of creating a copy of netdev_ops for each
> > device using phylib. First, there would be some overhead (just checked
> > my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
> > quite frequent pattern of comparing dev->netdev_ops against known
> > constants to check if a network device is of certain type; I can't say
> > for sure if it is also used with devices using phylib in existing code
> > but it feels risky.  
> 
> I agree with Michal here. I don't like this.
> 
> I think we need phylib to register a set of ops with ethtool when it
> loads. It would also allow us to clean up phy_ethtool_get_strings(),
> phy_ethtool_get_sset_count(), phy_ethtool_get_stats().

+1

Re: [PATCH net-next 0/4] net: ethtool: Untangle PHYLIB dependency

[Florian Fainelli]

On 7/2/2020 9:35 AM, Jakub Kicinski wrote:
> On Thu, 2 Jul 2020 18:34:24 +0200 Andrew Lunn wrote:
>> On Thu, Jul 02, 2020 at 05:56:52PM +0200, Michal Kubecek wrote:
>>> On Wed, Jul 01, 2020 at 09:29:38PM -0700, Florian Fainelli wrote:  
>>>> Hi all,
>>>>
>>>> This patch series untangles the ethtool netlink dependency with PHYLIB
>>>> which exists because the cable test feature calls directly into PHY
>>>> library functions. The approach taken here is to utilize a new set of
>>>> net_device_ops function pointers which are automatically set to the PHY
>>>> library variants when a network device driver attaches to a PHY device.  
>>>
>>> I'm not sure about the idea of creating a copy of netdev_ops for each
>>> device using phylib. First, there would be some overhead (just checked
>>> my 5.8-rc3 kernel, struct netdev_ops is 632 bytes). Second, there is
>>> quite frequent pattern of comparing dev->netdev_ops against known
>>> constants to check if a network device is of certain type; I can't say
>>> for sure if it is also used with devices using phylib in existing code
>>> but it feels risky.  
>>
>> I agree with Michal here. I don't like this.
>>
>> I think we need phylib to register a set of ops with ethtool when it
>> loads. It would also allow us to clean up phy_ethtool_get_strings(),
>> phy_ethtool_get_sset_count(), phy_ethtool_get_stats().
> 
> +1

OK, that makes sense, I will work on that.
-- 
Florian