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

[PATCHv3 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: rebased on recent upstream changes
v3: incorporated hints on naming and comments

[PATCHv3 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: rebased on changed upstream
v3: changed names to clearly say that this does NOT use phylib

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       | 36 ++++++++++++++++++++++++++++------
 include/linux/usb/usbnet.h     |  6 ++++--
 10 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 6e13d8165852..19a8fafb8f04 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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 4087c9e33781..0d26cbeb6e04 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_internal,
+	.set_link_ksettings      = usbnet_set_link_ksettings_mii,
 };
 
 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..89cc61d7a675 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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..9f9352a4522f 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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..55025202dc4f 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_mii,
+	.set_link_ksettings = usbnet_set_link_ksettings_mii,
 };
 
 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..f8cdabb9ef5a 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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..ce29261263cd 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 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..a822d81310d5 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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 static int sr9800_link_reset(struct usbnet *dev)
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index b4c8080e6f87..f3e5ad9befd0 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_mii(struct net_device *net,
 			      struct ethtool_link_ksettings *cmd)
 {
 	struct usbnet *dev = netdev_priv(net);
@@ -956,9 +959,30 @@ 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_mii);
+
+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(struct net_device *net,
+int usbnet_set_link_ksettings_mii(struct net_device *net,
 			      const struct ethtool_link_ksettings *cmd)
 {
 	struct usbnet *dev = netdev_priv(net);
@@ -978,7 +1002,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_mii);
 
 u32 usbnet_get_link (struct net_device *net)
 {
@@ -1043,8 +1067,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_mii,
+	.set_link_ksettings	= usbnet_set_link_ksettings_mii,
 };
 
 /*-------------------------------------------------------------------------*/
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index cfbfd6fe01df..132c1b5e14bb 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -267,9 +267,11 @@ 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_mii(struct net_device *net,
 				     struct ethtool_link_ksettings *cmd);
-extern int usbnet_set_link_ksettings(struct net_device *net,
+extern int usbnet_get_link_ksettings_internal(struct net_device *net,
+					struct ethtool_link_ksettings *cmd);
+extern int usbnet_set_link_ksettings_mii(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

[PATCHv3 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: rebased on upstream
v3: changed names and made clear which units are used

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

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f3e5ad9befd0..368428a4290b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -971,10 +971,10 @@ int usbnet_get_link_ksettings_internal(struct net_device *net,
 	 * 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;
+	if (dev->rx_speed != SPEED_UNSET)
+		cmd->base.speed = dev->rx_speed / 1000000;
+	else if (dev->tx_speed != SPEED_UNSET)
+		cmd->base.speed = dev->tx_speed / 1000000;
 	else
 		cmd->base.speed = SPEED_UNKNOWN;
 
@@ -1685,6 +1685,9 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	dev->intf = udev;
 	dev->driver_info = info;
 	dev->driver_name = name;
+	/* cannot use 0, as simplex devices may exist */
+	dev->rx_speed = SPEED_UNSET; /* unknown or handled by MII */
+	dev->tx_speed = SPEED_UNSET; 
 
 	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 132c1b5e14bb..7f445c1e0003 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -53,6 +53,10 @@ struct usbnet {
 	u32			hard_mtu;	/* count any extra framing */
 	size_t			rx_urb_size;	/* size for rx urbs */
 	struct mii_if_info	mii;
+	/* These are bits per second unlike what ethernet uses */
+	long			rx_speed;	/* if MII is not used */
+	long			tx_speed;	/* if MII is not used */
+#		define SPEED_UNSET	-1
 
 	/* various kinds of pending driver work */
 	struct sk_buff_head	rxq;
@@ -81,8 +85,6 @@ struct usbnet {
 #		define EVENT_LINK_CHANGE	11
 #		define EVENT_SET_RX_MODE	12
 #		define EVENT_NO_IP_ALIGN	13
-	u32			rx_speed;	/* in bps - NOT Mbps */
-	u32			tx_speed;	/* in bps - NOT Mbps */
 };
 
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
2.26.2

[PATCHv3 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: rebased on upstream
v3: changed variable names

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

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 0d26cbeb6e04..74c1a86b1a71 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1829,30 +1829,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;
-
+	 /* RTL8156 shipped before 2021 sends notification about every 32ms. */
 	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));
-	}
 }
 
 static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
-- 
2.26.2

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

[Jakub Kicinski]

On Thu, 18 Feb 2021 11:20:36 +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: rebased on changed upstream
> v3: changed names to clearly say that this does NOT use phylib
> 
> Signed-off-by : Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>

This one does not build.

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

[Jakub Kicinski]

On Thu, 18 Feb 2021 11:20:37 +0100 Oliver Neukum 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: rebased on upstream
> v3: changed names and made clear which units are used
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>

ERROR: trailing whitespace
#48: FILE: drivers/net/usb/usbnet.c:1690:
+^Idev->tx_speed = SPEED_UNSET; $

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

[Grant Grundler]

Oliver, Jakub,
Can I post v4 and deal with the issues below?

I can compile and test against ToT for now and I'll like to get this
mess behind me.

I still think the first patch in this series should revert my previous
change (de658a195ee23ca6aaffe197d1d2ea040beea0a2).

On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> 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.

I'd like to propose a rewrite of the commit message:
    usbnet: add _mii suffix to usbnet_set/get_link_ksettings

    The generic functions assumed devices provided an MDIO interface (accessed
    via older mii code, not phylib). This is true only for genuine ethernet.

    Devices with a higher level of abstraction or based on different
    technologies do not have MDIO. To support this case, first rename
    the existing functions with _mii suffix.

> v2: rebased on changed upstream
> v3: changed names to clearly say that this does NOT use phylib

Nit: The v2/v3 lines should be included BELOW the '---' line since
they don't belong in the commit message.

Nit2: V3 folds part of a successive patch into this one. If that was
intentional, the commit message needs further rewrite and the changes
to usbnet.h got lost.

> 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       | 36 ++++++++++++++++++++++++++++------
>  include/linux/usb/usbnet.h     |  6 ++++--
>  10 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 6e13d8165852..19a8fafb8f04 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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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 4087c9e33781..0d26cbeb6e04 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_internal,
> +       .set_link_ksettings      = usbnet_set_link_ksettings_mii,
>  };
>
>  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..89cc61d7a675 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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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..9f9352a4522f 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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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..55025202dc4f 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_mii,
> +       .set_link_ksettings = usbnet_set_link_ksettings_mii,
>  };
>
>  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..f8cdabb9ef5a 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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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..ce29261263cd 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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  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..a822d81310d5 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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  static int sr9800_link_reset(struct usbnet *dev)
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index b4c8080e6f87..f3e5ad9befd0 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_mii(struct net_device *net,
>                               struct ethtool_link_ksettings *cmd)
>  {
>         struct usbnet *dev = netdev_priv(net);
> @@ -956,9 +959,30 @@ 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_mii);
> +
> +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)

rxspeed doesn't exist yet - that was part of the successive patch
(added to usbnet.h)

cheers,
grant

> +               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(struct net_device *net,
> +int usbnet_set_link_ksettings_mii(struct net_device *net,
>                               const struct ethtool_link_ksettings *cmd)
>  {
>         struct usbnet *dev = netdev_priv(net);
> @@ -978,7 +1002,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_mii);
>
>  u32 usbnet_get_link (struct net_device *net)
>  {
> @@ -1043,8 +1067,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_mii,
> +       .set_link_ksettings     = usbnet_set_link_ksettings_mii,
>  };
>
>  /*-------------------------------------------------------------------------*/
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index cfbfd6fe01df..132c1b5e14bb 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -267,9 +267,11 @@ 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_mii(struct net_device *net,
>                                      struct ethtool_link_ksettings *cmd);
> -extern int usbnet_set_link_ksettings(struct net_device *net,
> +extern int usbnet_get_link_ksettings_internal(struct net_device *net,
> +                                       struct ethtool_link_ksettings *cmd);
> +extern int usbnet_set_link_ksettings_mii(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: [PATCHv3 1/3] usbnet: specify naming of usbnet_set/get_link_ksettings

[Andrew Lunn]

On Thu, Feb 18, 2021 at 07:31:41PM +0000, Grant Grundler wrote:
> Oliver, Jakub,
> Can I post v4 and deal with the issues below?

You should probably wait for two weeks. We are far enough into the
merge window that i doubt it will get picked up. So please wait,
rebase, and then post.

> Nit: The v2/v3 lines should be included BELOW the '---' line since
> they don't belong in the commit message.

netdev actually prefers them above, so we see the history of how a
patched changed during review.

	Andrew

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

[Grant Grundler]

On Thu, Feb 18, 2021 at 7:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Feb 18, 2021 at 07:31:41PM +0000, Grant Grundler wrote:
> > Oliver, Jakub,
> > Can I post v4 and deal with the issues below?
>
> You should probably wait for two weeks. We are far enough into the
> merge window that i doubt it will get picked up. So please wait,
> rebase, and then post.

ACK - thank you.

> > Nit: The v2/v3 lines should be included BELOW the '---' line since
> > they don't belong in the commit message.
>
> netdev actually prefers them above, so we see the history of how a
> patched changed during review.

Ok. Thanks for clarifying. :)

cheers,
grant

>
>         Andrew

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

[Grant Grundler]

On Thu, Feb 18, 2021 at 10:21 AM 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.

Since this patch is missing the hunks that landed in the previous
patch and needs a v4, I'll offer my version of the commit message in
case you like it better:
    net: cdc_ncm: record speed in status method

    Until very recently, the usbnet framework only had support functions
    for devices which reported the link speed by explicitly querying the
    PHY over a MDIO interface. However, the cdc_ncm devices send
    notifications when the link state or link speeds change and do not
    expose the PHY (or modem) directly.

    Support functions (e.g. usbnet_get_link_ksettings_internal()) to directly
    query state recorded by the cdc_ncm driver were added in a previous patch.

    So instead of cdc_ncm spewing the link speed into the dmesg buffer,
    record the link speed encoded in these notifications and tell the
    usbnet framework to use the new functions to get link speed.

    This is especially useful given all existing RTL8156 devices emit
    a connection/speed status notification every 32ms and this would
    fill the dmesg buffer. This implementation replaces the one
    recently submitted in de658a195ee23ca6aaffe197d1d2ea040beea0a2 :
       "net: usb: cdc_ncm: don't spew notifications"

cheers,
grant

> v2: rebased on upstream
> v3: changed variable names
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>
> ---
>  drivers/net/usb/cdc_ncm.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 0d26cbeb6e04..74c1a86b1a71 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1829,30 +1829,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;
> -
> +        /* RTL8156 shipped before 2021 sends notification about every 32ms. */
>         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));
> -       }
>  }
>
>  static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
> --
> 2.26.2
>

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

[Grant Grundler]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

Oliver,
Can you include a 4th patch in the series to bring cdc_ether driver in
line with the cdc_ncm behavior?

I apologize for not including the patch inline - but it's late and I
don't want to fight with gmail at this point. Patch is attached. Not
tested.

cheers,
grant

On Thu, Feb 18, 2021 at 10:21 AM 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: rebased on upstream
> v3: changed variable names
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Tested-by: Roland Dreier <roland@kernel.org>
> ---
>  drivers/net/usb/cdc_ncm.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
>
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 0d26cbeb6e04..74c1a86b1a71 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1829,30 +1829,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;
> -
> +        /* RTL8156 shipped before 2021 sends notification about every 32ms. */
>         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));
> -       }
>  }
>
>  static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
> --
> 2.26.2
>

[-- Attachment #2: 0001-net-cdc_ether-record-speed-in-status-method.patch --]
[-- Type: text/x-patch, Size: 3555 bytes --]

From e5ac528c08bb35b25cbbf9a5e91d2491100e6763 Mon Sep 17 00:00:00 2001
From: Grant Grundler <Grant Grundler grundler@chromium.org>
Date: Wed, 17 Feb 2021 20:55:57 -0800
Subject: [PATCH] net: cdc_ether: record speed in status method

Until very recently, the usbnet framework only had support functions
for devices which reported the link speed by explicitly querying the
PHY over a MDIO interface. However, the cdc_ether devices send
notifications when the link state or link speeds change and do not
expose the PHY (or modem) directly.

Support funtions (e.g. usbnet_get_link_ksettings_internal()) to directly
query state recorded by the cdc_ether driver were added in a previous patch.

So instead of cdc_ether spewing the link speed into the dmesg buffer,
record the link speed encoded in these notifications and tell the
usbnet framework to use the new functions to get link speed/state.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 drivers/net/usb/cdc_ether.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index a9b551028659..e13e9b799432 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -92,6 +92,18 @@ void usbnet_cdc_update_filter(struct usbnet *dev)
 }
 EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
 
+/* We need to override usbnet_*_link_ksettings in bind() */
+static const struct ethtool_ops cdc_ether_ethtool_ops = {
+	.get_link		= usbnet_get_link,
+	.nway_reset		= usbnet_nway_reset,
+	.get_drvinfo		= usbnet_get_drvinfo,
+	.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_internal,
+	.set_link_ksettings	= NULL,
+};
+
 /* probes control interface, claims data interface, collects the bulk
  * endpoints, activates data interface (if needed), maybe sets MTU.
  * all pure cdc, except for certain firmware workarounds, and knowing
@@ -310,6 +322,9 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 		return -ENODEV;
 	}
 
+	/* override ethtool_ops */
+	dev->net->ethtool_ops = &cdc_ether_ethtool_ops;
+
 	return 0;
 
 bad_desc:
@@ -379,12 +394,10 @@ EXPORT_SYMBOL_GPL(usbnet_cdc_unbind);
  * (by Brad Hards) talked with, with more functionality.
  */
 
-static void dumpspeed(struct usbnet *dev, __le32 *speeds)
+static void speed_change(struct usbnet *dev, __le32 *speeds)
 {
-	netif_info(dev, timer, dev->net,
-		   "link speeds: %u kbps up, %u kbps down\n",
-		   __le32_to_cpu(speeds[0]) / 1000,
-		   __le32_to_cpu(speeds[1]) / 1000);
+	dev->tx_speed = __le32_to_cpu(speeds[0]);
+	dev->rx_speed = __le32_to_cpu(speeds[1]);
 }
 
 void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
@@ -396,7 +409,7 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
 
 	/* SPEED_CHANGE can get split into two 8-byte packets */
 	if (test_and_clear_bit(EVENT_STS_SPLIT, &dev->flags)) {
-		dumpspeed(dev, (__le32 *) urb->transfer_buffer);
+		speed_change(dev, (__le32 *) urb->transfer_buffer);
 		return;
 	}
 
@@ -413,7 +426,7 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
 		if (urb->actual_length != (sizeof(*event) + 8))
 			set_bit(EVENT_STS_SPLIT, &dev->flags);
 		else
-			dumpspeed(dev, (__le32 *) &event[1]);
+			speed_change(dev, (__le32 *) &event[1]);
 		break;
 	/* USB_CDC_NOTIFY_RESPONSE_AVAILABLE can happen too (e.g. RNDIS),
 	 * but there are no standard formats for the response data.
-- 
2.30.0

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

[Oliver Neukum]

Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler:
> On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote:

Hi,

> Since this patch is missing the hunks that landed in the previous
> patch and needs a v4, I'll offer my version of the commit message in

That is bad. I will have to search for that.

> case you like it better:

Something written by a native speaker with knowledge in the field is
always better. I will take it, wait two weeks and then resubmit.

	Regards
		Oliver

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

[Grant Grundler]

[-- Attachment #1: Type: text/plain, Size: 3454 bytes --]

Hi Oliver,

On Mon, Feb 22, 2021 at 10:14 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler:
> > On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Hi,
>
> > Since this patch is missing the hunks that landed in the previous
> > patch and needs a v4, I'll offer my version of the commit message in
>
> That is bad. I will have to search for that.

No worries - it happens. To make this easier for you, let me point out
what I've observed.

I just noticed there are two hunks that landed in the wrong (posted) patches.

1) In "[PATCHv3 2/3] usbnet: add method for reporting speed without
MDIO", this hunk is "repairing" the part that failed to build for
Jakub:

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index f3e5ad9befd0..368428a4290b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -971,10 +971,10 @@ int usbnet_get_link_ksettings_internal(struct
net_device *net,
         * 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;
+       if (dev->rx_speed != SPEED_UNSET)
+               cmd->base.speed = dev->rx_speed / 1000000;
+       else if (dev->tx_speed != SPEED_UNSET)
+               cmd->base.speed = dev->tx_speed / 1000000;
        else
                cmd->base.speed = SPEED_UNKNOWN;

Just this hunk should have been folded into the previous patch:
"[PATCHv3 1/3] usbnet: specify naming of
usbnet_set/get_link_ksettings"

2) "[PATCHv3 1/3]" has the hunk to override .get_link_ksettings and
.set_link_ksettings fields for cdc_ncm.c:
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4087c9e33781..0d26cbeb6e04 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_internal,
+       .set_link_ksettings      = usbnet_set_link_ksettings_mii,
 };

This hunk should have been included in "[PATCHv3 3/3] CDC-NCM: record
speed in status method" (email thread I'm replying to).

Also, I believe this hunk is incorrect: .set_link_ksettings should be
set to NULL since speed can't be changed by cdc_ncm driver (AFAIK).


Swap around where those hunks landed and you'll be golden. :)

> > case you like it better:
>
> Something written by a native speaker with knowledge in the field is
> always better.

"knowledge in the field" sounds quite generous. I'll claim I
understand this particular problem reasonably well. :)

> I will take it, wait two weeks and then resubmit.

Excellent!

Just to be clear, I'm understanding you will resubmit next week. SGTM.

Also, please add the cdc_ether patch (attached) to the series (since
it depends on the work you are doing). Andrew Lunn
also expected cdc_ether to be updated in the same series (in reply to
"[PATCHv2 0/3] usbnet: speed reporting...").

cheers,
grant

[-- Attachment #2: 0001-net-cdc_ether-record-speed-in-status-method.patch --]
[-- Type: text/x-patch, Size: 3555 bytes --]

From e5ac528c08bb35b25cbbf9a5e91d2491100e6763 Mon Sep 17 00:00:00 2001
From: Grant Grundler <Grant Grundler grundler@chromium.org>
Date: Wed, 17 Feb 2021 20:55:57 -0800
Subject: [PATCH] net: cdc_ether: record speed in status method

Until very recently, the usbnet framework only had support functions
for devices which reported the link speed by explicitly querying the
PHY over a MDIO interface. However, the cdc_ether devices send
notifications when the link state or link speeds change and do not
expose the PHY (or modem) directly.

Support funtions (e.g. usbnet_get_link_ksettings_internal()) to directly
query state recorded by the cdc_ether driver were added in a previous patch.

So instead of cdc_ether spewing the link speed into the dmesg buffer,
record the link speed encoded in these notifications and tell the
usbnet framework to use the new functions to get link speed/state.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 drivers/net/usb/cdc_ether.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index a9b551028659..e13e9b799432 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -92,6 +92,18 @@ void usbnet_cdc_update_filter(struct usbnet *dev)
 }
 EXPORT_SYMBOL_GPL(usbnet_cdc_update_filter);
 
+/* We need to override usbnet_*_link_ksettings in bind() */
+static const struct ethtool_ops cdc_ether_ethtool_ops = {
+	.get_link		= usbnet_get_link,
+	.nway_reset		= usbnet_nway_reset,
+	.get_drvinfo		= usbnet_get_drvinfo,
+	.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_internal,
+	.set_link_ksettings	= NULL,
+};
+
 /* probes control interface, claims data interface, collects the bulk
  * endpoints, activates data interface (if needed), maybe sets MTU.
  * all pure cdc, except for certain firmware workarounds, and knowing
@@ -310,6 +322,9 @@ int usbnet_generic_cdc_bind(struct usbnet *dev, struct usb_interface *intf)
 		return -ENODEV;
 	}
 
+	/* override ethtool_ops */
+	dev->net->ethtool_ops = &cdc_ether_ethtool_ops;
+
 	return 0;
 
 bad_desc:
@@ -379,12 +394,10 @@ EXPORT_SYMBOL_GPL(usbnet_cdc_unbind);
  * (by Brad Hards) talked with, with more functionality.
  */
 
-static void dumpspeed(struct usbnet *dev, __le32 *speeds)
+static void speed_change(struct usbnet *dev, __le32 *speeds)
 {
-	netif_info(dev, timer, dev->net,
-		   "link speeds: %u kbps up, %u kbps down\n",
-		   __le32_to_cpu(speeds[0]) / 1000,
-		   __le32_to_cpu(speeds[1]) / 1000);
+	dev->tx_speed = __le32_to_cpu(speeds[0]);
+	dev->rx_speed = __le32_to_cpu(speeds[1]);
 }
 
 void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
@@ -396,7 +409,7 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
 
 	/* SPEED_CHANGE can get split into two 8-byte packets */
 	if (test_and_clear_bit(EVENT_STS_SPLIT, &dev->flags)) {
-		dumpspeed(dev, (__le32 *) urb->transfer_buffer);
+		speed_change(dev, (__le32 *) urb->transfer_buffer);
 		return;
 	}
 
@@ -413,7 +426,7 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
 		if (urb->actual_length != (sizeof(*event) + 8))
 			set_bit(EVENT_STS_SPLIT, &dev->flags);
 		else
-			dumpspeed(dev, (__le32 *) &event[1]);
+			speed_change(dev, (__le32 *) &event[1]);
 		break;
 	/* USB_CDC_NOTIFY_RESPONSE_AVAILABLE can happen too (e.g. RNDIS),
 	 * but there are no standard formats for the response data.
-- 
2.30.0

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

[Grant Grundler]

On Mon, Feb 22, 2021 at 10:14 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler:
> > On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> Hi,
>
> > Since this patch is missing the hunks that landed in the previous
> > patch and needs a v4, I'll offer my version of the commit message in
>
> That is bad. I will have to search for that.
>
> > case you like it better:
>
> Something written by a native speaker with knowledge in the field is
> always better. I will take it, wait two weeks and then resubmit.

3 weeks are up. Oliver, did you want to post V4?

cheers,
grant

>
>         Regards
>                 Oliver
>
>