[RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[Russell King]

Provide a pointer to the SFP bus in struct net_device, so that the
ethtool module EEPROM methods can access the SFP directly, rather
than needing every user to provide a hook for it.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Questions:
1. Is it worth adding a pointer to struct net_device for these two
   methods, rather than having multiple duplicate veneers to vector
   the ethtool module EEPROM ioctls through to the SFP bus layer?

2. Should this allow network/phy drivers to override the default -
   the code is currently structured to allow phy drivers to override
   network drivers implementations, which seems the wrong way around.

 drivers/net/phy/phylink.c             | 28 ----------------------------
 drivers/net/phy/sfp-bus.c             |  6 ++----
 include/linux/netdevice.h             |  3 +++
 include/linux/phylink.h               |  3 ---
 net/core/ethtool.c                    |  7 +++++++
 5 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index db5d5726ced9..0f59d7149a61 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1247,34 +1247,6 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 }
 EXPORT_SYMBOL_GPL(phylink_ethtool_set_pauseparam);
 
-int phylink_ethtool_get_module_info(struct phylink *pl,
-				    struct ethtool_modinfo *modinfo)
-{
-	int ret = -EOPNOTSUPP;
-
-	WARN_ON(!lockdep_rtnl_is_held());
-
-	if (pl->sfp_bus)
-		ret = sfp_get_module_info(pl->sfp_bus, modinfo);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phylink_ethtool_get_module_info);
-
-int phylink_ethtool_get_module_eeprom(struct phylink *pl,
-				      struct ethtool_eeprom *ee, u8 *buf)
-{
-	int ret = -EOPNOTSUPP;
-
-	WARN_ON(!lockdep_rtnl_is_held());
-
-	if (pl->sfp_bus)
-		ret = sfp_get_module_eeprom(pl->sfp_bus, ee, buf);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phylink_ethtool_get_module_eeprom);
-
 /**
  * phylink_ethtool_get_eee_err() - read the energy efficient ethernet error
  *   counter
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
index 1356dba0d9d3..4d61099b1357 100644
--- a/drivers/net/phy/sfp-bus.c
+++ b/drivers/net/phy/sfp-bus.c
@@ -321,6 +321,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
 	}
 	if (bus->started)
 		bus->socket_ops->start(bus->sfp);
+	bus->netdev->sfp_bus = bus;
 	bus->registered = true;
 	return 0;
 }
@@ -335,6 +336,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
 		if (bus->phydev && ops && ops->disconnect_phy)
 			ops->disconnect_phy(bus->upstream);
 	}
+	bus->netdev->sfp_bus = NULL;
 	bus->registered = false;
 }
 
@@ -350,8 +352,6 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
  */
 int sfp_get_module_info(struct sfp_bus *bus, struct ethtool_modinfo *modinfo)
 {
-	if (!bus->registered)
-		return -ENOIOCTLCMD;
 	return bus->socket_ops->module_info(bus->sfp, modinfo);
 }
 EXPORT_SYMBOL_GPL(sfp_get_module_info);
@@ -370,8 +370,6 @@ EXPORT_SYMBOL_GPL(sfp_get_module_info);
 int sfp_get_module_eeprom(struct sfp_bus *bus, struct ethtool_eeprom *ee,
 			  u8 *data)
 {
-	if (!bus->registered)
-		return -ENOIOCTLCMD;
 	return bus->socket_ops->module_eeprom(bus->sfp, ee, data);
 }
 EXPORT_SYMBOL_GPL(sfp_get_module_eeprom);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ef789e1d679e..99a0a155c319 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -57,6 +57,7 @@ struct device;
 struct phy_device;
 struct dsa_port;
 
+struct sfp_bus;
 /* 802.11 specific */
 struct wireless_dev;
 /* 802.15.4 specific */
@@ -1644,6 +1645,7 @@ enum netdev_priv_flags {
  *	@priomap:	XXX: need comments on this one
  *	@phydev:	Physical device may attach itself
  *			for hardware timestamping
+ *	@sfp_bus:	attached &struct sfp_bus structure.
  *
  *	@qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
  *	@qdisc_running_key: lockdep class annotating Qdisc->running seqcount
@@ -1922,6 +1924,7 @@ struct net_device {
 	struct netprio_map __rcu *priomap;
 #endif
 	struct phy_device	*phydev;
+	struct sfp_bus		*sfp_bus;
 	struct lock_class_key	*qdisc_tx_busylock;
 	struct lock_class_key	*qdisc_running_key;
 	bool			proto_down;
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index bd137c273d38..618fa5e83564 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -211,9 +211,6 @@ void phylink_ethtool_get_pauseparam(struct phylink *,
 				    struct ethtool_pauseparam *);
 int phylink_ethtool_set_pauseparam(struct phylink *,
 				   struct ethtool_pauseparam *);
-int phylink_ethtool_get_module_info(struct phylink *, struct ethtool_modinfo *);
-int phylink_ethtool_get_module_eeprom(struct phylink *,
-				      struct ethtool_eeprom *, u8 *);
 int phylink_get_eee_err(struct phylink *);
 int phylink_ethtool_get_eee(struct phylink *, struct ethtool_eee *);
 int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f8fcf450a36e..86a6b3d05116 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -22,6 +22,7 @@
 #include <linux/bitops.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
+#include <linux/sfp.h>
 #include <linux/slab.h>
 #include <linux/rtnetlink.h>
 #include <linux/sched/signal.h>
@@ -2217,6 +2218,9 @@ static int __ethtool_get_module_info(struct net_device *dev,
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	struct phy_device *phydev = dev->phydev;
 
+	if (dev->sfp_bus)
+		return sfp_get_module_info(dev->sfp_bus, modinfo);
+
 	if (phydev && phydev->drv && phydev->drv->module_info)
 		return phydev->drv->module_info(phydev, modinfo);
 
@@ -2251,6 +2255,9 @@ static int __ethtool_get_module_eeprom(struct net_device *dev,
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	struct phy_device *phydev = dev->phydev;
 
+	if (dev->sfp_bus)
+		return sfp_get_module_eeprom(dev->sfp_bus, ee, data);
+
 	if (phydev && phydev->drv && phydev->drv->module_eeprom)
 		return phydev->drv->module_eeprom(phydev, ee, data);
 
-- 
2.7.4

Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[Russell King - ARM Linux]

> Questions:
> 1. Is it worth adding a pointer to struct net_device for these two
>    methods, rather than having multiple duplicate veneers to vector
>    the ethtool module EEPROM ioctls through to the SFP bus layer?
> 
> 2. Should this allow network/phy drivers to override the default -
>    the code is currently structured to allow phy drivers to override
>    network drivers implementations, which seems the wrong way around.

I should also mention that there's another place that having the
sfp bus pointer in the network device comes in handy - the case
where we have a SFP module connected to a PHY rather than the MAC.

In this case, phylink itself is not used to link the SFP to the
netdev, and phylib doesn't provide the necessary hooks into the PHY
driver for the PHY driver to know when the network device comes up
or goes down.  SFP needs to know that to assert/deassert the TX
DISABLE signal to disable the module laser.

Having the net_device structure contain a pointer to the SFP bus
allows phylink or network drivers to directly inform SFP of the
state of the network device, without needing intermediaries to
forward the state.

It's possible that this may not be the best approach - the only setup
I'm aware of at present that has the "mac <-> phy <-> sfp" setup is
the Macchiatobin, but if other phys are involved, it may be better
if instead of having PHY drivers having to add support for SFP, we
instead do it in phylib.  The counter argument to that is that SFP
likely needs more in-depth knowledge of the PHY than a the generic
phylib parts could know about.

The patches as they currently stand are in my "phy" branch, browsable
via:

 http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=phy

specifically:

 sfp: use netdev sfp_bus for start/stop
 net: phy: Add SFP support to Marvell 10G PHY driver

The last patch is does not (yet) take into account the RX_LOS signal
when determining the link state, which it ought to to avoid false
link assertions as can happen when there's noise pickup by the
detector.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[Andrew Lunn]

On Sun, Dec 17, 2017 at 02:48:27PM +0000, Russell King wrote:
> Provide a pointer to the SFP bus in struct net_device, so that the
> ethtool module EEPROM methods can access the SFP directly, rather
> than needing every user to provide a hook for it.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Questions:
> 1. Is it worth adding a pointer to struct net_device for these two
>    methods, rather than having multiple duplicate veneers to vector
>    the ethtool module EEPROM ioctls through to the SFP bus layer?
> 
> 2. Should this allow network/phy drivers to override the default -
>    the code is currently structured to allow phy drivers to override
>    network drivers implementations, which seems the wrong way around.

Hi Russell

Looking at drivers which implement reading the EEPROM, very few of
them expose the i2c bus, as a linux i2c bus. They seem to send
commands off to the firmware, and have it return a block of data.  So
converting to using the generic SFP code is not going to be too easy.

Probably a low hanging fruit is to expose a few library like functions
for parsing the EEPROM data. As you said, there seems to be a few bugs
in the drivers with respect to actually interpreting the data. So
having one central implementation, without bugs, would be good.

Rather than adding the sfp bus to net_device, i think phylink will get
more use. And the default implementation of these methods can look at
the phylink to see if there is an sfp device. We are unlikely to be
able to replace phydev with phylink, but maybe all new 10Gbps PHY and
fibre modules not hidden behind firmware could use phylink? So having
phylink in net_device could make sense. There has been a move to
remove phydev from the drivers private structure and use the one in
net_device. Maybe we should do the same for phylink?

    Andrew

Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[Russell King - ARM Linux]

On Sun, Dec 17, 2017 at 07:29:22PM +0100, Andrew Lunn wrote:
> On Sun, Dec 17, 2017 at 02:48:27PM +0000, Russell King wrote:
> > Provide a pointer to the SFP bus in struct net_device, so that the
> > ethtool module EEPROM methods can access the SFP directly, rather
> > than needing every user to provide a hook for it.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > Questions:
> > 1. Is it worth adding a pointer to struct net_device for these two
> >    methods, rather than having multiple duplicate veneers to vector
> >    the ethtool module EEPROM ioctls through to the SFP bus layer?
> > 
> > 2. Should this allow network/phy drivers to override the default -
> >    the code is currently structured to allow phy drivers to override
> >    network drivers implementations, which seems the wrong way around.
> 
> Hi Russell
> 
> Looking at drivers which implement reading the EEPROM, very few of
> them expose the i2c bus, as a linux i2c bus. They seem to send
> commands off to the firmware, and have it return a block of data.  So
> converting to using the generic SFP code is not going to be too easy.
> 
> Probably a low hanging fruit is to expose a few library like functions
> for parsing the EEPROM data. As you said, there seems to be a few bugs
> in the drivers with respect to actually interpreting the data. So
> having one central implementation, without bugs, would be good.
> 
> Rather than adding the sfp bus to net_device, i think phylink will get
> more use. And the default implementation of these methods can look at
> the phylink to see if there is an sfp device.

You can't layer phylink on top of phylib on top of phylink for the
situation where we have a SFP cage connected to a PHY - that's the
problem.

SFP needs to know what is happening with the net device, and when
to enable or disable the laser, and although there's notifiers for
the netdev up/down, using that is far from ideal in this case - to
do so, SFP would need the reverse phandle in DT so it knows which
network device its associated with.  I've already been there with
a previous iteration of the SFP code.

> We are unlikely to be
> able to replace phydev with phylink, but maybe all new 10Gbps PHY and
> fibre modules not hidden behind firmware could use phylink? So having
> phylink in net_device could make sense. There has been a move to
> remove phydev from the drivers private structure and use the one in
> net_device. Maybe we should do the same for phylink?

I would suggest you read the patch that adds SFP support to the 88x3310
PHY driver - that case makes no use of phylink.  As I mention above,
it's not possible to layer phylink on top of phylib.  Not only would
that lead to nested locks, but phy drivers do not have the knowledge
necessary to know when to make various phylink calls, as phylib
drivers have no clue when the network device comes up/goes down for
example.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[Florian Fainelli]

On 12/17/2017 06:48 AM, Russell King wrote:
> Provide a pointer to the SFP bus in struct net_device, so that the
> ethtool module EEPROM methods can access the SFP directly, rather
> than needing every user to provide a hook for it.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Questions:
> 1. Is it worth adding a pointer to struct net_device for these two
>    methods, rather than having multiple duplicate veneers to vector
>    the ethtool module EEPROM ioctls through to the SFP bus layer?

Considering the negative diffstat and the fact that it solves real
problems for you, I would say yes.

> 
> 2. Should this allow network/phy drivers to override the default -
>    the code is currently structured to allow phy drivers to override
>    network drivers implementations, which seems the wrong way around.

This would be nice, but at this point, I would defer until we have all
major PHYLINK, SFP et al. users merged in tree so we have a good
understanding and view of the different possible combinations that might
exist. Then we can try to define an interface allowing network drivers
more flexibility.

If that seems like an appropriate course of action, do you mind
resubmitting this as non RFC?


> 
>  drivers/net/phy/phylink.c             | 28 ----------------------------
>  drivers/net/phy/sfp-bus.c             |  6 ++----
>  include/linux/netdevice.h             |  3 +++
>  include/linux/phylink.h               |  3 ---
>  net/core/ethtool.c                    |  7 +++++++
>  5 files changed, 12 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index db5d5726ced9..0f59d7149a61 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1247,34 +1247,6 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
>  }
>  EXPORT_SYMBOL_GPL(phylink_ethtool_set_pauseparam);
>  
> -int phylink_ethtool_get_module_info(struct phylink *pl,
> -				    struct ethtool_modinfo *modinfo)
> -{
> -	int ret = -EOPNOTSUPP;
> -
> -	WARN_ON(!lockdep_rtnl_is_held());
> -
> -	if (pl->sfp_bus)
> -		ret = sfp_get_module_info(pl->sfp_bus, modinfo);
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(phylink_ethtool_get_module_info);
> -
> -int phylink_ethtool_get_module_eeprom(struct phylink *pl,
> -				      struct ethtool_eeprom *ee, u8 *buf)
> -{
> -	int ret = -EOPNOTSUPP;
> -
> -	WARN_ON(!lockdep_rtnl_is_held());
> -
> -	if (pl->sfp_bus)
> -		ret = sfp_get_module_eeprom(pl->sfp_bus, ee, buf);
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(phylink_ethtool_get_module_eeprom);
> -
>  /**
>   * phylink_ethtool_get_eee_err() - read the energy efficient ethernet error
>   *   counter
> diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c
> index 1356dba0d9d3..4d61099b1357 100644
> --- a/drivers/net/phy/sfp-bus.c
> +++ b/drivers/net/phy/sfp-bus.c
> @@ -321,6 +321,7 @@ static int sfp_register_bus(struct sfp_bus *bus)
>  	}
>  	if (bus->started)
>  		bus->socket_ops->start(bus->sfp);
> +	bus->netdev->sfp_bus = bus;
>  	bus->registered = true;
>  	return 0;
>  }
> @@ -335,6 +336,7 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
>  		if (bus->phydev && ops && ops->disconnect_phy)
>  			ops->disconnect_phy(bus->upstream);
>  	}
> +	bus->netdev->sfp_bus = NULL;
>  	bus->registered = false;
>  }
>  
> @@ -350,8 +352,6 @@ static void sfp_unregister_bus(struct sfp_bus *bus)
>   */
>  int sfp_get_module_info(struct sfp_bus *bus, struct ethtool_modinfo *modinfo)
>  {
> -	if (!bus->registered)
> -		return -ENOIOCTLCMD;
>  	return bus->socket_ops->module_info(bus->sfp, modinfo);
>  }
>  EXPORT_SYMBOL_GPL(sfp_get_module_info);
> @@ -370,8 +370,6 @@ EXPORT_SYMBOL_GPL(sfp_get_module_info);
>  int sfp_get_module_eeprom(struct sfp_bus *bus, struct ethtool_eeprom *ee,
>  			  u8 *data)
>  {
> -	if (!bus->registered)
> -		return -ENOIOCTLCMD;
>  	return bus->socket_ops->module_eeprom(bus->sfp, ee, data);
>  }
>  EXPORT_SYMBOL_GPL(sfp_get_module_eeprom);
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ef789e1d679e..99a0a155c319 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -57,6 +57,7 @@ struct device;
>  struct phy_device;
>  struct dsa_port;
>  
> +struct sfp_bus;
>  /* 802.11 specific */
>  struct wireless_dev;
>  /* 802.15.4 specific */
> @@ -1644,6 +1645,7 @@ enum netdev_priv_flags {
>   *	@priomap:	XXX: need comments on this one
>   *	@phydev:	Physical device may attach itself
>   *			for hardware timestamping
> + *	@sfp_bus:	attached &struct sfp_bus structure.
>   *
>   *	@qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
>   *	@qdisc_running_key: lockdep class annotating Qdisc->running seqcount
> @@ -1922,6 +1924,7 @@ struct net_device {
>  	struct netprio_map __rcu *priomap;
>  #endif
>  	struct phy_device	*phydev;
> +	struct sfp_bus		*sfp_bus;
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	struct lock_class_key	*qdisc_running_key;
>  	bool			proto_down;
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index bd137c273d38..618fa5e83564 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -211,9 +211,6 @@ void phylink_ethtool_get_pauseparam(struct phylink *,
>  				    struct ethtool_pauseparam *);
>  int phylink_ethtool_set_pauseparam(struct phylink *,
>  				   struct ethtool_pauseparam *);
> -int phylink_ethtool_get_module_info(struct phylink *, struct ethtool_modinfo *);
> -int phylink_ethtool_get_module_eeprom(struct phylink *,
> -				      struct ethtool_eeprom *, u8 *);
>  int phylink_get_eee_err(struct phylink *);
>  int phylink_ethtool_get_eee(struct phylink *, struct ethtool_eee *);
>  int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *);
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f8fcf450a36e..86a6b3d05116 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -22,6 +22,7 @@
>  #include <linux/bitops.h>
>  #include <linux/uaccess.h>
>  #include <linux/vmalloc.h>
> +#include <linux/sfp.h>
>  #include <linux/slab.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/sched/signal.h>
> @@ -2217,6 +2218,9 @@ static int __ethtool_get_module_info(struct net_device *dev,
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
>  	struct phy_device *phydev = dev->phydev;
>  
> +	if (dev->sfp_bus)
> +		return sfp_get_module_info(dev->sfp_bus, modinfo);
> +
>  	if (phydev && phydev->drv && phydev->drv->module_info)
>  		return phydev->drv->module_info(phydev, modinfo);
>  
> @@ -2251,6 +2255,9 @@ static int __ethtool_get_module_eeprom(struct net_device *dev,
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
>  	struct phy_device *phydev = dev->phydev;
>  
> +	if (dev->sfp_bus)
> +		return sfp_get_module_eeprom(dev->sfp_bus, ee, data);
> +
>  	if (phydev && phydev->drv && phydev->drv->module_eeprom)
>  		return phydev->drv->module_eeprom(phydev, ee, data);
>  
> 


-- 
Florian

Re: [RFC net-next] sfp/phylink: move module EEPROM ethtool access into netdev core ethtool

[Andrew Lunn]

On Mon, Mar 19, 2018 at 12:20:32PM -0700, Florian Fainelli wrote:
> On 12/17/2017 06:48 AM, Russell King wrote:
> > Provide a pointer to the SFP bus in struct net_device, so that the
> > ethtool module EEPROM methods can access the SFP directly, rather
> > than needing every user to provide a hook for it.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > Questions:
> > 1. Is it worth adding a pointer to struct net_device for these two
> >    methods, rather than having multiple duplicate veneers to vector
> >    the ethtool module EEPROM ioctls through to the SFP bus layer?
> 
> Considering the negative diffstat and the fact that it solves real
> problems for you, I would say yes.

We have also received a bunch of patches removing the phydev pointer
for driver private structures and making use of the net_device one. It
would be nice to avoid the same with phylink.

      Andrew