[PATCHv2 0/3] usbnet: speed reporting for devices without MDIO

[PATCHv2 0/3] usbnet: speed reporting for devices without MDIO

[Oliver Neukum]

This series introduces support for USB network devices that report
speed as a part of their protocol, not emulating an MII to be accessed
over MDIO.

v2: adjusted to recent changes

[PATCHv2 1/3] usbnet: specify naming of usbnet_set/get_link_ksettings

[Oliver Neukum]

The old generic functions assume that the devices includes
an MDIO interface. This is true only for genuine ethernet.
Devices with a higher level of abstraction or based on different
technologies do not have it. So in preparation for
supporting that, we rename the old functions to something specific.

v2: adjusted to recent changes

Signed-off-by : Oliver Neukum <oneukum@suse.com>
Tested-by: Roland Dreier <roland@kernel.org>
---
 drivers/net/usb/asix_devices.c | 12 ++++++------
 drivers/net/usb/cdc_ncm.c      |  4 ++--
 drivers/net/usb/dm9601.c       |  4 ++--
 drivers/net/usb/mcs7830.c      |  4 ++--
 drivers/net/usb/sierra_net.c   |  4 ++--
 drivers/net/usb/smsc75xx.c     |  4 ++--
 drivers/net/usb/sr9700.c       |  4 ++--
 drivers/net/usb/sr9800.c       |  4 ++--
 drivers/net/usb/usbnet.c       | 15 +++++++++------
 include/linux/usb/usbnet.h     |  4 ++--
 10 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 6e13d8165852..f5006890873d 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -125,8 +125,8 @@ static const struct ethtool_ops ax88172_ethtool_ops = {
 	.get_eeprom		= asix_get_eeprom,
 	.set_eeprom		= asix_set_eeprom,
 	.nway_reset		= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static void ax88172_set_multicast(struct net_device *net)
@@ -291,8 +291,8 @@ static const struct ethtool_ops ax88772_ethtool_ops = {
 	.get_eeprom		= asix_get_eeprom,
 	.set_eeprom		= asix_set_eeprom,
 	.nway_reset		= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static int ax88772_link_reset(struct usbnet *dev)
@@ -782,8 +782,8 @@ static const struct ethtool_ops ax88178_ethtool_ops = {
 	.get_eeprom		= asix_get_eeprom,
 	.set_eeprom		= asix_set_eeprom,
 	.nway_reset		= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static int marvell_phy_init(struct usbnet *dev)
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 291e76d32abe..04174704bf7c 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -142,8 +142,8 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.get_sset_count    = cdc_ncm_get_sset_count,
 	.get_strings       = cdc_ncm_get_strings,
 	.get_ethtool_stats = cdc_ncm_get_ethtool_stats,
-	.get_link_ksettings      = usbnet_get_link_ksettings,
-	.set_link_ksettings      = usbnet_set_link_ksettings,
+	.get_link_ksettings      = usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings      = usbnet_set_link_ksettings_mdio,
 };
 
 static u32 cdc_ncm_check_rx_max(struct usbnet *dev, u32 new_rx)
diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index b5d2ac55a874..9065aa13540b 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -282,8 +282,8 @@ static const struct ethtool_ops dm9601_ethtool_ops = {
 	.get_eeprom_len	= dm9601_get_eeprom_len,
 	.get_eeprom	= dm9601_get_eeprom,
 	.nway_reset	= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static void dm9601_set_multicast(struct net_device *net)
diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index fc512b780d15..c52f52b1e619 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -452,8 +452,8 @@ static const struct ethtool_ops mcs7830_ethtool_ops = {
 	.get_msglevel		= usbnet_get_msglevel,
 	.set_msglevel		= usbnet_set_msglevel,
 	.nway_reset		= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static const struct net_device_ops mcs7830_netdev_ops = {
diff --git a/drivers/net/usb/sierra_net.c b/drivers/net/usb/sierra_net.c
index 55a244eca5ca..cbab5c9f19b8 100644
--- a/drivers/net/usb/sierra_net.c
+++ b/drivers/net/usb/sierra_net.c
@@ -629,8 +629,8 @@ static const struct ethtool_ops sierra_net_ethtool_ops = {
 	.get_msglevel = usbnet_get_msglevel,
 	.set_msglevel = usbnet_set_msglevel,
 	.nway_reset = usbnet_nway_reset,
-	.get_link_ksettings = usbnet_get_link_ksettings,
-	.set_link_ksettings = usbnet_set_link_ksettings,
+	.get_link_ksettings = usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings = usbnet_set_link_ksettings_mdio,
 };
 
 static int sierra_net_get_fw_attr(struct usbnet *dev, u16 *datap)
diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 4353b370249f..e4c6c7f3c69b 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -741,8 +741,8 @@ static const struct ethtool_ops smsc75xx_ethtool_ops = {
 	.set_eeprom	= smsc75xx_ethtool_set_eeprom,
 	.get_wol	= smsc75xx_ethtool_get_wol,
 	.set_wol	= smsc75xx_ethtool_set_wol,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static int smsc75xx_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd)
diff --git a/drivers/net/usb/sr9700.c b/drivers/net/usb/sr9700.c
index 878557ad03ad..e552ab95ced2 100644
--- a/drivers/net/usb/sr9700.c
+++ b/drivers/net/usb/sr9700.c
@@ -250,8 +250,8 @@ static const struct ethtool_ops sr9700_ethtool_ops = {
 	.get_eeprom_len	= sr9700_get_eeprom_len,
 	.get_eeprom	= sr9700_get_eeprom,
 	.nway_reset	= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static void sr9700_set_multicast(struct net_device *netdev)
diff --git a/drivers/net/usb/sr9800.c b/drivers/net/usb/sr9800.c
index da56735d7755..b466d53047c5 100644
--- a/drivers/net/usb/sr9800.c
+++ b/drivers/net/usb/sr9800.c
@@ -527,8 +527,8 @@ static const struct ethtool_ops sr9800_ethtool_ops = {
 	.get_eeprom_len	= sr_get_eeprom_len,
 	.get_eeprom	= sr_get_eeprom,
 	.nway_reset	= usbnet_nway_reset,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 static int sr9800_link_reset(struct usbnet *dev)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 1447da1d5729..e2ca88259b05 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -944,7 +944,10 @@ EXPORT_SYMBOL_GPL(usbnet_open);
  * they'll probably want to use this base set.
  */
 
-int usbnet_get_link_ksettings(struct net_device *net,
+/* These methods are written on the assumption that the device
+ * uses MII
+ */
+int usbnet_get_link_ksettings_mdio(struct net_device *net,
 			      struct ethtool_link_ksettings *cmd)
 {
 	struct usbnet *dev = netdev_priv(net);
@@ -956,9 +959,9 @@ int usbnet_get_link_ksettings(struct net_device *net,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings);
+EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio);
 
-int usbnet_set_link_ksettings(struct net_device *net,
+int usbnet_set_link_ksettings_mdio(struct net_device *net,
 			      const struct ethtool_link_ksettings *cmd)
 {
 	struct usbnet *dev = netdev_priv(net);
@@ -978,7 +981,7 @@ int usbnet_set_link_ksettings(struct net_device *net,
 
 	return retval;
 }
-EXPORT_SYMBOL_GPL(usbnet_set_link_ksettings);
+EXPORT_SYMBOL_GPL(usbnet_set_link_ksettings_mdio);
 
 u32 usbnet_get_link (struct net_device *net)
 {
@@ -1043,8 +1046,8 @@ static const struct ethtool_ops usbnet_ethtool_ops = {
 	.get_msglevel		= usbnet_get_msglevel,
 	.set_msglevel		= usbnet_set_msglevel,
 	.get_ts_info		= ethtool_op_get_ts_info,
-	.get_link_ksettings	= usbnet_get_link_ksettings,
-	.set_link_ksettings	= usbnet_set_link_ksettings,
+	.get_link_ksettings	= usbnet_get_link_ksettings_mdio,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mdio,
 };
 
 /*-------------------------------------------------------------------------*/
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index cfbfd6fe01df..fd65b7a5ee15 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -267,9 +267,9 @@ extern void usbnet_pause_rx(struct usbnet *);
 extern void usbnet_resume_rx(struct usbnet *);
 extern void usbnet_purge_paused_rxq(struct usbnet *);
 
-extern int usbnet_get_link_ksettings(struct net_device *net,
+extern int usbnet_get_link_ksettings_mdio(struct net_device *net,
 				     struct ethtool_link_ksettings *cmd);
-extern int usbnet_set_link_ksettings(struct net_device *net,
+extern int usbnet_set_link_ksettings_mdio(struct net_device *net,
 				     const struct ethtool_link_ksettings *cmd);
 extern u32 usbnet_get_link(struct net_device *net);
 extern u32 usbnet_get_msglevel(struct net_device *);
-- 
2.26.2

Re: [PATCHv2 1/3] usbnet: specify naming of usbnet_set/get_link_ksettings

[Andrew Lunn]

On Thu, Jan 21, 2021 at 01:57:29PM +0100, Oliver Neukum wrote:
> The old generic functions assume that the devices includes
> an MDIO interface. This is true only for genuine ethernet.
> Devices with a higher level of abstraction or based on different
> technologies do not have it. So in preparation for
> supporting that, we rename the old functions to something specific.
> 
> v2: adjusted to recent changes

Hi Oliver

It  looks like my comment:

https://www.spinics.net/lists/netdev/msg711869.html

was ignored. Do you not like the name mii?

    Andrew

Re: [PATCHv2 1/3] usbnet: specify naming of usbnet_set/get_link_ksettings

[Oliver Neukum]

Am Freitag, den 22.01.2021, 02:10 +0100 schrieb Andrew Lunn:
> On Thu, Jan 21, 2021 at 01:57:29PM +0100, Oliver Neukum wrote:
> > The old generic functions assume that the devices includes
> > an MDIO interface. This is true only for genuine ethernet.
> > Devices with a higher level of abstraction or based on different
> > technologies do not have it. So in preparation for
> > supporting that, we rename the old functions to something specific.
> > 
> > v2: adjusted to recent changes
> 
> Hi Oliver
> 
> It  looks like my comment:
> 
> https://www.spinics.net/lists/netdev/msg711869.html
> 
> was ignored. Do you not like the name mii?

Hi,

sorry for not replying earlier.

It was my understanding that on the hardware level of the
networking devices we are using MII, but to control MII we
use MDIO, don't we?
So it seems to me that hardware could use MII but not
MDIO, yet for this purpose we require MDIO. So could
you please explain your reasoning about networking stuff?

I do not want to create false impressions in users.

	Regards
		Oliver

Re: [PATCHv2 1/3] usbnet: specify naming of usbnet_set/get_link_ksettings

[Andrew Lunn]

On Tue, Jan 26, 2021 at 10:42:09AM +0100, Oliver Neukum wrote:
> Am Freitag, den 22.01.2021, 02:10 +0100 schrieb Andrew Lunn:
> > On Thu, Jan 21, 2021 at 01:57:29PM +0100, Oliver Neukum wrote:
> > > The old generic functions assume that the devices includes
> > > an MDIO interface. This is true only for genuine ethernet.
> > > Devices with a higher level of abstraction or based on different
> > > technologies do not have it. So in preparation for
> > > supporting that, we rename the old functions to something specific.
> > > 
> > > v2: adjusted to recent changes
> > 
> > Hi Oliver
> > 
> > It  looks like my comment:
> > 
> > https://www.spinics.net/lists/netdev/msg711869.html
> > 
> > was ignored. Do you not like the name mii?
> 
> Hi,
> 
> sorry for not replying earlier.
> 
> It was my understanding that on the hardware level of the
> networking devices we are using MII, but to control MII we
> use MDIO, don't we?
> So it seems to me that hardware could use MII but not
> MDIO, yet for this purpose we require MDIO. So could
> you please explain your reasoning about networking stuff?

Hi Oliver

To some extent, it is a terminology problem. First off, MII includes
the two MDIO pins. MDIO is a subset of MII.

However, the bigger issue is Linux has two different bits of code
which can be used to talk to the PHY. There is the old mii code,
driver/net/mii.c. This code assumes the PHY exactly follows 802.3
clause 22.

Then we have drivers/net/mdio, drivers/net/phy, phylib, and a
collection of PHYs drivers. The MDIO drivers implement the MDIO bus,
allowing transfers over the bus. And the PHY drivers then handle the
devices on this bus. These PHY drivers can handle nearly any quirk the
PHY might have which deviate from C22. It also allows drivers to use
C45, the alternative register set PHYs can use. And it allows for
added extras, like temperature sensors, cable diagnostics and
statistics, none of which is standardised.

The code you are changing makes use of the older mii code.  There are
however some USB devices which use phylib. By using the postfix _mii
for these ops, it makes it clear it is using the older mii code. In
the future, there might be _phylib versions of these ops. It is very
unlikely any USB device driver will directly use an MDIO bus drivers,
so _mdio does not really make sense, from the perspective of Linux
code.

    Andrew

[PATCHv2 2/3] usbnet: add method for reporting speed without MDIO

[Oliver Neukum]

The old method for reporting network speed upwards
assumed that a device uses MDIO and uses the generic phy
functions based on that.
Add a a primitive internal version not making the assumption
reporting back directly what the status operations record.

v2: adjusted to recent changes

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Tested-by: Roland Dreier <roland@kernel.org>
---
 drivers/net/usb/usbnet.c   | 23 +++++++++++++++++++++++
 include/linux/usb/usbnet.h |  4 ++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e2ca88259b05..6f8fcc276ca7 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mdio(struct net_device *net,
 }
 EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio);
 
+int usbnet_get_link_ksettings_internal(struct net_device *net,
+					struct ethtool_link_ksettings *cmd)
+{
+	struct usbnet *dev = netdev_priv(net);
+
+	/* the assumption that speed is equal on tx and rx
+	 * is deeply engrained into the networking layer.
+	 * For wireless stuff it is not true.
+	 * We assume that rxspeed matters more.
+	 */
+	if (dev->rxspeed != SPEED_UNKNOWN)
+		cmd->base.speed = dev->rxspeed / 1000000;
+	else if (dev->txspeed != SPEED_UNKNOWN)
+		cmd->base.speed = dev->txspeed / 1000000;
+	else
+		cmd->base.speed = SPEED_UNKNOWN;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);
+
 int usbnet_set_link_ksettings_mdio(struct net_device *net,
 			      const struct ethtool_link_ksettings *cmd)
 {
@@ -1664,6 +1685,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	dev->intf = udev;
 	dev->driver_info = info;
 	dev->driver_name = name;
+	dev->rxspeed = SPEED_UNKNOWN; /* unknown or handled by MII */
+	dev->txspeed = SPEED_UNKNOWN;
 
 	net->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
 	if (!net->tstats)
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index fd65b7a5ee15..a91c6defb104 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -53,6 +53,8 @@ struct usbnet {
 	u32			hard_mtu;	/* count any extra framing */
 	size_t			rx_urb_size;	/* size for rx urbs */
 	struct mii_if_info	mii;
+	long			rxspeed;	/* if MII is not used */
+	long			txspeed;	/* if MII is not used */
 
 	/* various kinds of pending driver work */
 	struct sk_buff_head	rxq;
@@ -269,6 +271,8 @@ extern void usbnet_purge_paused_rxq(struct usbnet *);
 
 extern int usbnet_get_link_ksettings_mdio(struct net_device *net,
 				     struct ethtool_link_ksettings *cmd);
+extern int usbnet_get_link_ksettings_internal(struct net_device *net,
+					struct ethtool_link_ksettings *cmd);
 extern int usbnet_set_link_ksettings_mdio(struct net_device *net,
 				     const struct ethtool_link_ksettings *cmd);
 extern u32 usbnet_get_link(struct net_device *net);
-- 
2.26.2

Re: [PATCHv2 2/3] usbnet: add method for reporting speed without MDIO

[Grant Grundler]

On Thu, Jan 21, 2021 at 12:57 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> The old method for reporting network speed upwards
> assumed that a device uses MDIO and uses the generic phy
> functions based on that.
> Add a a primitive internal version not making the assumption
> reporting back directly what the status operations record.

Excellent!  I wasted a bunch of time looking at emulating the MDIO
interface and decided that was too hacky.  I didn't realize it could
be so easy to fork off a different method to collect the most recently
reported speed. Thank you!

> v2: adjusted to recent changes
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>
> ---
>  drivers/net/usb/usbnet.c   | 23 +++++++++++++++++++++++
>  include/linux/usb/usbnet.h |  4 ++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index e2ca88259b05..6f8fcc276ca7 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mdio(struct net_device *net,
>  }
>  EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio);
>
> +int usbnet_get_link_ksettings_internal(struct net_device *net,
> +                                       struct ethtool_link_ksettings *cmd)
> +{
> +       struct usbnet *dev = netdev_priv(net);
> +
> +       /* the assumption that speed is equal on tx and rx
> +        * is deeply engrained into the networking layer.
> +        * For wireless stuff it is not true.
> +        * We assume that rxspeed matters more.
> +        */
> +       if (dev->rxspeed != SPEED_UNKNOWN)
> +               cmd->base.speed = dev->rxspeed / 1000000;
> +       else if (dev->txspeed != SPEED_UNKNOWN)
> +               cmd->base.speed = dev->txspeed / 1000000;
> +       else
> +               cmd->base.speed = SPEED_UNKNOWN;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);
> +
>  int usbnet_set_link_ksettings_mdio(struct net_device *net,
>                               const struct ethtool_link_ksettings *cmd)
>  {
> @@ -1664,6 +1685,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>         dev->intf = udev;
>         dev->driver_info = info;
>         dev->driver_name = name;
> +       dev->rxspeed = SPEED_UNKNOWN; /* unknown or handled by MII */
> +       dev->txspeed = SPEED_UNKNOWN;

Nit: Use of SPEED_UNKNOWN here can be confusing since the units for
rxspeed/txspeed are bps, not Mbps (AFAICT SPEED_XXX are Mbps numbers).
In other words, this is the only usbnet location that could use
SPEED_xxx define and other developers might not realize that.
Personally, I'd rather set the fields to zero here.

>
>         net->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
>         if (!net->tstats)
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index fd65b7a5ee15..a91c6defb104 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -53,6 +53,8 @@ struct usbnet {
>         u32                     hard_mtu;       /* count any extra framing */
>         size_t                  rx_urb_size;    /* size for rx urbs */
>         struct mii_if_info      mii;
> +       long                    rxspeed;        /* if MII is not used */
> +       long                    txspeed;        /* if MII is not used */

Do you want to note the units used (bps) in the trailing comment?

The numbers for cdc_ncm and cdc_ether will be "bits per second" due to
how older modems could report "odd" (from an ethernet point of view)
speeds.

Also, the changes I just submitted (and kuba@kernel.org accepted)
named these "rx_speed" (with underscore). I don't care which it is but
just wanted to warn about and apologize for the conflict.

cheers,
grant

>         /* various kinds of pending driver work */
>         struct sk_buff_head     rxq;
> @@ -269,6 +271,8 @@ extern void usbnet_purge_paused_rxq(struct usbnet *);
>
>  extern int usbnet_get_link_ksettings_mdio(struct net_device *net,
>                                      struct ethtool_link_ksettings *cmd);
> +extern int usbnet_get_link_ksettings_internal(struct net_device *net,
> +                                       struct ethtool_link_ksettings *cmd);
>  extern int usbnet_set_link_ksettings_mdio(struct net_device *net,
>                                      const struct ethtool_link_ksettings *cmd);
>  extern u32 usbnet_get_link(struct net_device *net);
> --
> 2.26.2
>

Re: [PATCHv2 2/3] usbnet: add method for reporting speed without MDIO

[Grant Grundler]

On Thu, Jan 21, 2021 at 12:57 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> The old method for reporting network speed upwards
> assumed that a device uses MDIO and uses the generic phy
> functions based on that.
> Add a a primitive internal version not making the assumption
> reporting back directly what the status operations record.
>
> v2: adjusted to recent changes
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>
> ---
>  drivers/net/usb/usbnet.c   | 23 +++++++++++++++++++++++
>  include/linux/usb/usbnet.h |  4 ++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index e2ca88259b05..6f8fcc276ca7 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mdio(struct net_device *net,
>  }
>  EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio);
>
> +int usbnet_get_link_ksettings_internal(struct net_device *net,
> +                                       struct ethtool_link_ksettings *cmd)
> +{
> +       struct usbnet *dev = netdev_priv(net);
> +
> +       /* the assumption that speed is equal on tx and rx
> +        * is deeply engrained into the networking layer.

Another nit: s/engrained/ingrained

Also, the word "symmetric" is more commonly used to describe when RX
speed == TX speed (vs "equal").

cheers,
grant

> +        * For wireless stuff it is not true.
> +        * We assume that rxspeed matters more.
> +        */
> +       if (dev->rxspeed != SPEED_UNKNOWN)
> +               cmd->base.speed = dev->rxspeed / 1000000;
> +       else if (dev->txspeed != SPEED_UNKNOWN)
> +               cmd->base.speed = dev->txspeed / 1000000;
> +       else
> +               cmd->base.speed = SPEED_UNKNOWN;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);
> +
>  int usbnet_set_link_ksettings_mdio(struct net_device *net,
>                               const struct ethtool_link_ksettings *cmd)
>  {
> @@ -1664,6 +1685,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
>         dev->intf = udev;
>         dev->driver_info = info;
>         dev->driver_name = name;
> +       dev->rxspeed = SPEED_UNKNOWN; /* unknown or handled by MII */
> +       dev->txspeed = SPEED_UNKNOWN;
>
>         net->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
>         if (!net->tstats)
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index fd65b7a5ee15..a91c6defb104 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -53,6 +53,8 @@ struct usbnet {
>         u32                     hard_mtu;       /* count any extra framing */
>         size_t                  rx_urb_size;    /* size for rx urbs */
>         struct mii_if_info      mii;
> +       long                    rxspeed;        /* if MII is not used */
> +       long                    txspeed;        /* if MII is not used */
>
>         /* various kinds of pending driver work */
>         struct sk_buff_head     rxq;
> @@ -269,6 +271,8 @@ extern void usbnet_purge_paused_rxq(struct usbnet *);
>
>  extern int usbnet_get_link_ksettings_mdio(struct net_device *net,
>                                      struct ethtool_link_ksettings *cmd);
> +extern int usbnet_get_link_ksettings_internal(struct net_device *net,
> +                                       struct ethtool_link_ksettings *cmd);
>  extern int usbnet_set_link_ksettings_mdio(struct net_device *net,
>                                      const struct ethtool_link_ksettings *cmd);
>  extern u32 usbnet_get_link(struct net_device *net);
> --
> 2.26.2
>

[PATCHv2 3/3] CDC-NCM: record speed in status method

[Oliver Neukum]

The driver has a status method for receiving speed updates.
The framework, however, had support functions only for devices
that reported their speed upon an explicit query over a MDIO
interface.
CDC_NCM however gets direct notifications from the device.
As new support functions have become available, we shall now
record such notifications and tell the usbnet framework
to make direct use of them without going through the PHY layer.

v2: adjusted to recent changes

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Tested-by: Roland Dreier <roland@kernel.org>
---
 drivers/net/usb/cdc_ncm.c | 29 ++++-------------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 04174704bf7c..9b5bb8ae5eb8 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -142,7 +142,7 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
 	.get_sset_count    = cdc_ncm_get_sset_count,
 	.get_strings       = cdc_ncm_get_strings,
 	.get_ethtool_stats = cdc_ncm_get_ethtool_stats,
-	.get_link_ksettings      = usbnet_get_link_ksettings_mdio,
+	.get_link_ksettings      = usbnet_get_link_ksettings_internal,
 	.set_link_ksettings      = usbnet_set_link_ksettings_mdio,
 };
 
@@ -1827,30 +1827,9 @@ cdc_ncm_speed_change(struct usbnet *dev,
 	uint32_t rx_speed = le32_to_cpu(data->DLBitRRate);
 	uint32_t tx_speed = le32_to_cpu(data->ULBitRate);
 
-	/* if the speed hasn't changed, don't report it.
-	 * RTL8156 shipped before 2021 sends notification about every 32ms.
-	 */
-	if (dev->rx_speed == rx_speed && dev->tx_speed == tx_speed)
-		return;
-
-	dev->rx_speed = rx_speed;
-	dev->tx_speed = tx_speed;
-
-	/*
-	 * Currently the USB-NET API does not support reporting the actual
-	 * device speed. Do print it instead.
-	 */
-	if ((tx_speed > 1000000) && (rx_speed > 1000000)) {
-		netif_info(dev, link, dev->net,
-			   "%u mbit/s downlink %u mbit/s uplink\n",
-			   (unsigned int)(rx_speed / 1000000U),
-			   (unsigned int)(tx_speed / 1000000U));
-	} else {
-		netif_info(dev, link, dev->net,
-			   "%u kbit/s downlink %u kbit/s uplink\n",
-			   (unsigned int)(rx_speed / 1000U),
-			   (unsigned int)(tx_speed / 1000U));
-	}
+	 /* RTL8156 shipped before 2021 sends notification about every 32ms. */
+	dev->rxspeed = rx_speed;
+	dev->txspeed = tx_speed;
 }
 
 static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
-- 
2.26.2

Re: [PATCHv2 3/3] CDC-NCM: record speed in status method

[Grant Grundler]

On Thu, Jan 21, 2021 at 12:57 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> The driver has a status method for receiving speed updates.
> The framework, however, had support functions only for devices
> that reported their speed upon an explicit query over a MDIO
> interface.
> CDC_NCM however gets direct notifications from the device.
> As new support functions have become available, we shall now
> record such notifications and tell the usbnet framework
> to make direct use of them without going through the PHY layer.
>
> v2: adjusted to recent changes
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>
> ---
>  drivers/net/usb/cdc_ncm.c | 29 ++++-------------------------
>  1 file changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 04174704bf7c..9b5bb8ae5eb8 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -142,7 +142,7 @@ static const struct ethtool_ops cdc_ncm_ethtool_ops = {
>         .get_sset_count    = cdc_ncm_get_sset_count,
>         .get_strings       = cdc_ncm_get_strings,
>         .get_ethtool_stats = cdc_ncm_get_ethtool_stats,
> -       .get_link_ksettings      = usbnet_get_link_ksettings_mdio,
> +       .get_link_ksettings      = usbnet_get_link_ksettings_internal,
>         .set_link_ksettings      = usbnet_set_link_ksettings_mdio,
>  };
>
> @@ -1827,30 +1827,9 @@ cdc_ncm_speed_change(struct usbnet *dev,
>         uint32_t rx_speed = le32_to_cpu(data->DLBitRRate);
>         uint32_t tx_speed = le32_to_cpu(data->ULBitRate);
>
> -       /* if the speed hasn't changed, don't report it.
> -        * RTL8156 shipped before 2021 sends notification about every 32ms.
> -        */
> -       if (dev->rx_speed == rx_speed && dev->tx_speed == tx_speed)
> -               return;
> -
> -       dev->rx_speed = rx_speed;
> -       dev->tx_speed = tx_speed;

Oliver,
This patch removes the use of "rx_speed" field but doesn't remove the
field from struct usbnet (usbnet.h).

It might be better to revert my patch (make that the first patch in
your series) and use your original patches.
OR use "rx_speed" as the field name in the entire series and move them
to the preferred location in struct usbnet (patch 2/3).

cheers,
grant

> -
> -       /*
> -        * Currently the USB-NET API does not support reporting the actual
> -        * device speed. Do print it instead.
> -        */
> -       if ((tx_speed > 1000000) && (rx_speed > 1000000)) {
> -               netif_info(dev, link, dev->net,
> -                          "%u mbit/s downlink %u mbit/s uplink\n",
> -                          (unsigned int)(rx_speed / 1000000U),
> -                          (unsigned int)(tx_speed / 1000000U));
> -       } else {
> -               netif_info(dev, link, dev->net,
> -                          "%u kbit/s downlink %u kbit/s uplink\n",
> -                          (unsigned int)(rx_speed / 1000U),
> -                          (unsigned int)(tx_speed / 1000U));
> -       }
> +        /* RTL8156 shipped before 2021 sends notification about every 32ms. */
> +       dev->rxspeed = rx_speed;
> +       dev->txspeed = tx_speed;
>  }
>
>  static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
> --
> 2.26.2
>

Re: [PATCHv2 0/3] usbnet: speed reporting for devices without MDIO

[Andrew Lunn]

On Thu, Jan 21, 2021 at 01:57:28PM +0100, Oliver Neukum wrote:
> This series introduces support for USB network devices that report
> speed as a part of their protocol, not emulating an MII to be accessed
> over MDIO.
> 
> v2: adjusted to recent changes

Hi Oliver

Please give more details what actually changed.  Does this mean you
just rebased it on net-next? Or have you made real changes?

The discussion with v1 suggested that this framework should also be
used by anything which gets notified in CDC style. So i was expecting
to see cdc_ether.c also use this.

	    Andrew

Re: [PATCHv2 0/3] usbnet: speed reporting for devices without MDIO

[Grant Grundler]

On Fri, Jan 22, 2021 at 1:29 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Jan 21, 2021 at 01:57:28PM +0100, Oliver Neukum wrote:
> > This series introduces support for USB network devices that report
> > speed as a part of their protocol, not emulating an MII to be accessed
> > over MDIO.
> >
> > v2: adjusted to recent changes
>
> Hi Oliver
>
> Please give more details what actually changed.  Does this mean you
> just rebased it on net-next? Or have you made real changes?

My apologies to Oliver - the changes he's referring to are the ones I submitted:
   https://www.spinics.net/lists/netdev/msg715248.html

which is related to this series:
   https://www.spinics.net/lists/netdev/msg714493.html

I wasn't aware of and didn't look for the series Oliver had previously
posted. *sigh*  I have been talking to Realtek about getting the issue
of RTL8156 spewing notifications every 32ms fixed (thinking a FW
change could fix it) for nearly three months.  It is unfortunate
timing that Roland Dreier decided to do something about it in December
- which I didn't expect to happen given this problem was reported
nearly two years ago.

> The discussion with v1 suggested that this framework should also be
> used by anything which gets notified in CDC style. So i was expecting
> to see cdc_ether.c also use this.

Agreed. That's a two lines change to cdc_ether.c. I can submit this if
Oliver doesn't want to spin the series.

I've reviewed all three patches and besides one nit (which could be
ignored or fixed later), I'm offering my
   Reviewed-by: Grant Grundler <grundler@chromium.org>

in the off chance that helps get this accepted into net-next (and/or
5.11 RC release).

cheers,
grant

>
>             Andrew

Re: [PATCHv2 0/3] usbnet: speed reporting for devices without MDIO

[Grant Grundler]

Oliver,
it's been a few weeks, do you have time to post an updated patchset or
is there someone else you would trust to help land this series?

cheers,
grant

On Fri, Jan 22, 2021 at 2:10 AM Grant Grundler <grundler@chromium.org> wrote:
>
> On Fri, Jan 22, 2021 at 1:29 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Jan 21, 2021 at 01:57:28PM +0100, Oliver Neukum wrote:
> > > This series introduces support for USB network devices that report
> > > speed as a part of their protocol, not emulating an MII to be accessed
> > > over MDIO.
> > >
> > > v2: adjusted to recent changes
> >
> > Hi Oliver
> >
> > Please give more details what actually changed.  Does this mean you
> > just rebased it on net-next? Or have you made real changes?
>
> My apologies to Oliver - the changes he's referring to are the ones I submitted:
>    https://www.spinics.net/lists/netdev/msg715248.html
>
> which is related to this series:
>    https://www.spinics.net/lists/netdev/msg714493.html
>
> I wasn't aware of and didn't look for the series Oliver had previously
> posted. *sigh*  I have been talking to Realtek about getting the issue
> of RTL8156 spewing notifications every 32ms fixed (thinking a FW
> change could fix it) for nearly three months.  It is unfortunate
> timing that Roland Dreier decided to do something about it in December
> - which I didn't expect to happen given this problem was reported
> nearly two years ago.
>
> > The discussion with v1 suggested that this framework should also be
> > used by anything which gets notified in CDC style. So i was expecting
> > to see cdc_ether.c also use this.
>
> Agreed. That's a two lines change to cdc_ether.c. I can submit this if
> Oliver doesn't want to spin the series.
>
> I've reviewed all three patches and besides one nit (which could be
> ignored or fixed later), I'm offering my
>    Reviewed-by: Grant Grundler <grundler@chromium.org>
>
> in the off chance that helps get this accepted into net-next (and/or
> 5.11 RC release).
>
> cheers,
> grant
>
> >
> >             Andrew