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

[PATCH net-next v2 0/3] 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 introduce
ethtool_phy_ops function pointers which can be dynamically registered
when PHYLIB loads.

Florian Fainelli (3):
  net: ethtool: Introduce ethtool_phy_ops
  net: phy: Register ethtool PHY operations
  net: ethtool: Remove PHYLIB direct dependency

 drivers/net/phy/phy_device.c |  7 +++++++
 include/linux/ethtool.h      | 25 +++++++++++++++++++++++++
 net/Kconfig                  |  1 -
 net/ethtool/cabletest.c      | 18 ++++++++++++++++--
 net/ethtool/common.c         | 11 +++++++++++
 net/ethtool/common.h         |  2 ++
 6 files changed, 61 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH net-next v2 1/3] net: ethtool: Introduce ethtool_phy_ops

[Florian Fainelli]

In order to decouple ethtool from its PHY library dependency, define an
ethtool_phy_ops singleton which can be overriden by the PHY library when
it loads with an appropriate set of function pointers.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/ethtool.h | 25 +++++++++++++++++++++++++
 net/ethtool/common.c    | 11 +++++++++++
 net/ethtool/common.h    |  2 ++
 3 files changed, 38 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 48ad3b6a0150..0c139a93b67a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -502,5 +502,30 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 				       const struct ethtool_link_ksettings *cmd,
 				       u32 *dev_speed, u8 *dev_duplex);
 
+struct netlink_ext_ack;
+struct phy_device;
+struct phy_tdr_config;
+
+/**
+ * struct ethtool_phy_ops - Optional PHY device options
+ * @start_cable_test - Start a cable test
+ * @start_cable_test_tdr - Start a Time Domain Reflectometry cable test
+ *
+ * All operations are optional (i.e. the function pointer may be set to %NULL)
+ * and callers must take this into account. Callers must hold the RTNL lock.
+ */
+struct ethtool_phy_ops {
+	int (*start_cable_test)(struct phy_device *phydev,
+				struct netlink_ext_ack *extack);
+	int (*start_cable_test_tdr)(struct phy_device *phydev,
+				    struct netlink_ext_ack *extack,
+				    const struct phy_tdr_config *config);
+};
+
+/**
+ * ethtool_set_ethtool_phy_ops - Set the ethtool_phy_ops singleton
+ * @ops: Ethtool PHY operations to set
+ */
+void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops);
 
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index aaecfc916a4d..ce4dbae5a943 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -2,6 +2,7 @@
 
 #include <linux/net_tstamp.h>
 #include <linux/phy.h>
+#include <linux/rtnetlink.h>
 
 #include "common.h"
 
@@ -373,3 +374,13 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 
 	return 0;
 }
+
+const struct ethtool_phy_ops *ethtool_phy_ops;
+
+void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops)
+{
+	rtnl_lock();
+	ethtool_phy_ops = ops;
+	rtnl_unlock();
+}
+EXPORT_SYMBOL_GPL(ethtool_set_ethtool_phy_ops);
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index a62f68ccc43a..b83bef38368c 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -37,4 +37,6 @@ bool convert_legacy_settings_to_link_ksettings(
 int ethtool_get_max_rxfh_channel(struct net_device *dev, u32 *max);
 int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info);
 
+extern const struct ethtool_phy_ops *ethtool_phy_ops;
+
 #endif /* _ETHTOOL_COMMON_H */
-- 
2.25.1

[PATCH net-next v2 2/3] net: phy: Register ethtool PHY operations

[Florian Fainelli]

Utilize ethtool_set_ethtool_phy_ops to register a suitable set of PHY
ethtool operations in a dynamic fashion such that ethtool will no longer
directy reference PHY library symbols.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index eb1068a77ce1..94a5aa30b70f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3029,6 +3029,11 @@ static struct phy_driver genphy_driver = {
 	.set_loopback   = genphy_loopback,
 };
 
+static const struct ethtool_phy_ops phy_ethtool_phy_ops = {
+	.start_cable_test	= phy_start_cable_test,
+	.start_cable_test_tdr	= phy_start_cable_test_tdr,
+};
+
 static int __init phy_init(void)
 {
 	int rc;
@@ -3037,6 +3042,7 @@ static int __init phy_init(void)
 	if (rc)
 		return rc;
 
+	ethtool_set_ethtool_phy_ops(&phy_ethtool_phy_ops);
 	features_init();
 
 	rc = phy_driver_register(&genphy_c45_driver, THIS_MODULE);
@@ -3058,6 +3064,7 @@ static void __exit phy_exit(void)
 	phy_driver_unregister(&genphy_c45_driver);
 	phy_driver_unregister(&genphy_driver);
 	mdio_bus_exit();
+	ethtool_set_ethtool_phy_ops(NULL);
 }
 
 subsys_initcall(phy_init);
-- 
2.25.1

[PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

[Florian Fainelli]

Now that we have introduced ethtool_phy_ops and the PHY library
dynamically registers its operations with that function pointer, we can
remove the direct PHYLIB dependency in favor of using dynamic
operations.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/Kconfig             |  1 -
 net/ethtool/cabletest.c | 18 ++++++++++++++++--
 2 files changed, 16 insertions(+), 3 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 7194956aa09e..4f9fbdf7610c 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 ethtool_phy_ops *ops;
 	struct net_device *dev;
 	int ret;
 
@@ -81,11 +82,17 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	rtnl_lock();
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->start_cable_test) {
+		ret = -EOPNOTSUPP;
+		goto out_rtnl;
+	}
+
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = phy_start_cable_test(dev->phydev, info->extack);
+	ret = ops->start_cable_test(dev->phydev, info->extack);
 
 	ethnl_ops_complete(dev);
 
@@ -308,6 +315,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 ethtool_phy_ops *ops;
 	struct phy_tdr_config cfg;
 	struct net_device *dev;
 	int ret;
@@ -337,11 +345,17 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 		goto out_dev_put;
 
 	rtnl_lock();
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->start_cable_test_tdr) {
+		ret = -EOPNOTSUPP;
+		goto out_rtnl;
+	}
+
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		goto out_rtnl;
 
-	ret = phy_start_cable_test_tdr(dev->phydev, info->extack, &cfg);
+	ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg);
 
 	ethnl_ops_complete(dev);
 
-- 
2.25.1

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

[Andrew Lunn]

On Sun, Jul 05, 2020 at 09:27:55PM -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 introduce
> ethtool_phy_ops function pointers which can be dynamically registered
> when PHYLIB loads.
 
Hi Florian

This looks good. I would suggest leaving it a day or two for 0-day to
randconfig it for a while.

	   Andrew

Re: [PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

[Jakub Kicinski]

On Sun,  5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote:
> +	ops = ethtool_phy_ops;
> +	if (!ops || !ops->start_cable_test) {

nit: don't think member-by-member checking is necessary. We don't
expect there to be any alternative versions of the ops, right?

> +		ret = -EOPNOTSUPP;
> +		goto out_rtnl;
> +	}
> +
>  	ret = ethnl_ops_begin(dev);
>  	if (ret < 0)
>  		goto out_rtnl;
>  
> -	ret = phy_start_cable_test(dev->phydev, info->extack);
> +	ret = ops->start_cable_test(dev->phydev, info->extack);

nit: my personal preference would be to hide checking the ops and
calling the member in a static inline helper.

Note that we should be able to remove this from phy.h now:

#if IS_ENABLED(CONFIG_PHYLIB)
int phy_start_cable_test(struct phy_device *phydev,
			 struct netlink_ext_ack *extack);
int phy_start_cable_test_tdr(struct phy_device *phydev,
			     struct netlink_ext_ack *extack,
			     const struct phy_tdr_config *config);
#else
static inline
int phy_start_cable_test(struct phy_device *phydev,
			 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,
			     struct netlink_ext_ack *extack,
			     const struct phy_tdr_config *config)
{
	NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support");
	return -EOPNOTSUPP;
}
#endif


We could even risk a direct call:

#if IS_REACHABLE(CONFIG_PHYLIB)
static inline int do_x()
{
	return __do_x();
}
#else
static inline int do_x()
{
	if (!ops)
		return -EOPNOTSUPP;
	return ops->do_x();
}
#endif

But that's perhaps doing too much...

Re: [PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

[Florian Fainelli]

On 7/6/2020 11:40 AM, Jakub Kicinski wrote:
> On Sun,  5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote:
>> +	ops = ethtool_phy_ops;
>> +	if (!ops || !ops->start_cable_test) {
> 
> nit: don't think member-by-member checking is necessary. We don't
> expect there to be any alternative versions of the ops, right?

There could be, a network device driver not using PHYLIB could register
its own operations and only implement a subset of these operations.

> 
>> +		ret = -EOPNOTSUPP;
>> +		goto out_rtnl;
>> +	}
>> +
>>  	ret = ethnl_ops_begin(dev);
>>  	if (ret < 0)
>>  		goto out_rtnl;
>>  
>> -	ret = phy_start_cable_test(dev->phydev, info->extack);
>> +	ret = ops->start_cable_test(dev->phydev, info->extack);
> 
> nit: my personal preference would be to hide checking the ops and
> calling the member in a static inline helper.
> 
> Note that we should be able to remove this from phy.h now:

I would prefer to keep thsose around in case a network device driver
cannot punt entirely onto PHYLIB and instead needs to wrap those calls
around.

> 
> #if IS_ENABLED(CONFIG_PHYLIB)
> int phy_start_cable_test(struct phy_device *phydev,
> 			 struct netlink_ext_ack *extack);
> int phy_start_cable_test_tdr(struct phy_device *phydev,
> 			     struct netlink_ext_ack *extack,
> 			     const struct phy_tdr_config *config);
> #else
> static inline
> int phy_start_cable_test(struct phy_device *phydev,
> 			 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,
> 			     struct netlink_ext_ack *extack,
> 			     const struct phy_tdr_config *config)
> {
> 	NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support");
> 	return -EOPNOTSUPP;
> }
> #endif
> 
> 
> We could even risk a direct call:
> 
> #if IS_REACHABLE(CONFIG_PHYLIB)
> static inline int do_x()
> {
> 	return __do_x();
> }
> #else
> static inline int do_x()
> {
> 	if (!ops)
> 		return -EOPNOTSUPP;
> 	return ops->do_x();
> }
> #endif
> 
> But that's perhaps doing too much...

Fine either way with me, let us see what Michal and Andrew think about that.
-- 
Florian

Re: [PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

[Jakub Kicinski]

On Mon, 6 Jul 2020 11:45:38 -0700 Florian Fainelli wrote:
> On 7/6/2020 11:40 AM, Jakub Kicinski wrote:
> > On Sun,  5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote:  
> >> +	ops = ethtool_phy_ops;
> >> +	if (!ops || !ops->start_cable_test) {  
> > 
> > nit: don't think member-by-member checking is necessary. We don't
> > expect there to be any alternative versions of the ops, right?  
> 
> There could be, a network device driver not using PHYLIB could register
> its own operations and only implement a subset of these operations.

I'd strongly prefer drivers did not insert themselves into
subsys-to-subsys glue :S

> > We could even risk a direct call:
> > 
> > #if IS_REACHABLE(CONFIG_PHYLIB)
> > static inline int do_x()
> > {
> > 	return __do_x();
> > }
> > #else
> > static inline int do_x()
> > {
> > 	if (!ops)
> > 		return -EOPNOTSUPP;
> > 	return ops->do_x();
> > }
> > #endif
> > 
> > But that's perhaps doing too much...  
> 
> Fine either way with me, let us see what Michal and Andrew think about that.

Ack, let's hear it :)

Re: [PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

[Andrew Lunn]

On Mon, Jul 06, 2020 at 11:40:00AM -0700, Jakub Kicinski wrote:
> On Sun,  5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote:
> > +	ops = ethtool_phy_ops;
> > +	if (!ops || !ops->start_cable_test) {
> 
> nit: don't think member-by-member checking is necessary. We don't
> expect there to be any alternative versions of the ops, right?

I would not like to see anything else registering an ops. So i think
taking an Opps would be a good indication somebody is doing something
wrong and needs fixing.

> We could even risk a direct call:
> 
> #if IS_REACHABLE(CONFIG_PHYLIB)
> static inline int do_x()
> {
> 	return __do_x();
> }
> #else
> static inline int do_x()
> {
> 	if (!ops)
> 		return -EOPNOTSUPP;
> 	return ops->do_x();
> }
> #endif
> 
> But that's perhaps doing too much...

I would say it is too far. Two ways of doing the same thing requires
twice as much testing. And these are not hot paths where we want to
eliminate as many instructions and trampolines as possible.

	  Andrew

Re: [PATCH net-next v2 3/3] net: ethtool: Remove PHYLIB direct dependency

[Michal Kubecek]

On Mon, Jul 06, 2020 at 09:56:03PM +0200, Andrew Lunn wrote:
> On Mon, Jul 06, 2020 at 11:40:00AM -0700, Jakub Kicinski wrote:
> > On Sun,  5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote:
> > > +	ops = ethtool_phy_ops;
> > > +	if (!ops || !ops->start_cable_test) {
> > 
> > nit: don't think member-by-member checking is necessary. We don't
> > expect there to be any alternative versions of the ops, right?
> 
> I would not like to see anything else registering an ops. So i think
> taking an Opps would be a good indication somebody is doing something
> wrong and needs fixing.
> 
> > We could even risk a direct call:
> > 
> > #if IS_REACHABLE(CONFIG_PHYLIB)
> > static inline int do_x()
> > {
> > 	return __do_x();
> > }
> > #else
> > static inline int do_x()
> > {
> > 	if (!ops)
> > 		return -EOPNOTSUPP;
> > 	return ops->do_x();
> > }
> > #endif
> > 
> > But that's perhaps doing too much...
> 
> I would say it is too far. Two ways of doing the same thing requires
> twice as much testing. And these are not hot paths where we want to
> eliminate as many instructions and trampolines as possible.

Agreed, it seems a bit over the top.

Michal

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

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Sun,  5 Jul 2020 21:27:55 -0700

> 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 introduce
> ethtool_phy_ops function pointers which can be dynamically registered
> when PHYLIB loads.

Series applied, thanks for doing this work Florian.