[PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[Grant Grundler]

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
v4: fix misplaced hunks; reword some commit messages;
    add same change for cdc_ether
v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by

I'm reposting Oliver Neukum's <oneukum@suse.com> patch series with
fix ups for "misplaced hunks" (landed in the wrong patches).
Please fixup the "author" if "git am" fails to attribute the
patches 1-3 (of 4) to Oliver.

I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB
and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel)
+ Alpha Network AUE2500C were connected directly to the NT-S25G
to get 2.5Gbps link rate:
# ethtool enx002427880815
Settings for enx002427880815:
        Supported ports: [  ]
        Supported link modes:   Not reported
        Supported pause frame use: No
        Supports auto-negotiation: No
        Supported FEC modes: Not reported
        Advertised link modes:  Not reported
        Advertised pause frame use: No
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 2500Mb/s
        Duplex: Half
        Auto-negotiation: off
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        MDI-X: Unknown
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes


"Duplex" is a lie since we get no information about it.

I expect "Auto-Negotiation" is always true for cdc_ncm and
cdc_ether devices and perhaps someone knows offhand how
to have ethtool report "true" instead.

But this is good step in the right direction.

base-commit: 1c273e10bc0cc7efb933e0ca10e260cdfc9f0b8c

[PATCH net-next v4 1/4] usbnet: add _mii suffix to usbnet_set/get_link_ksettings

[Grant Grundler]

From: Oliver Neukum <oneukum@suse.com>

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
v4: moved hunks to correct patch; reworded commmit messages

Signed-off-by : Oliver Neukum <oneukum@suse.com>
Tested-by: Roland Dreier <roland@kernel.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
Tested-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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(-)

I'm reposting this with corrections to "misplaced" patch hunks
that I pointed out on Feb 19.  Code otherwise should be the same
and Oliver Neukum should be listed as author in git.

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 8acf30115428..2644234d4c4d 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_mii,
+	.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 f4f37ecfed58..5b4629c80b4b 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,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_mii);
 
-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 +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_mii);
 
 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_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..a89e1452107d 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_mii(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_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.30.1

[PATCH net-next v4 2/4] usbnet: add method for reporting speed without MII

[Grant Grundler]

From: Oliver Neukum <oneukum@suse.com>

The old method for reporting link speed assumed a driver uses the
generic phy (mii) MDIO read/write functions. CDC devices don't
expose the phy.

Add a primitive internal version reporting back directly what
the CDC notification/status operations recorded.

v2: rebased on upstream
v3: changed names and made clear which units are used
v4: moved hunks to correct patch; rewrote commmit messages

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Tested-by: Roland Dreier <roland@kernel.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
Tested-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/usbnet.c   | 23 +++++++++++++++++++++++
 include/linux/usb/usbnet.h |  7 +++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 5b4629c80b4b..ecf62849f4c1 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -961,6 +961,27 @@ int usbnet_get_link_ksettings_mii(struct net_device *net,
 }
 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 rx_speed matters more.
+	 */
+	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;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);
+
 int usbnet_set_link_ksettings_mii(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->rx_speed = SPEED_UNSET;
+	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 a89e1452107d..8336e86ce606 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -53,6 +53,9 @@ struct usbnet {
 	u32			hard_mtu;	/* count any extra framing */
 	size_t			rx_urb_size;	/* size for rx urbs */
 	struct mii_if_info	mii;
+	long			rx_speed;	/* If MII not used */
+	long			tx_speed;	/* If MII not used */
+#		define SPEED_UNSET	-1
 
 	/* various kinds of pending driver work */
 	struct sk_buff_head	rxq;
@@ -81,8 +84,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)
@@ -271,6 +272,8 @@ extern int usbnet_get_link_ksettings_mii(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 int usbnet_get_link_ksettings_internal(struct net_device *net,
+				     struct ethtool_link_ksettings *cmd);
 extern u32 usbnet_get_link(struct net_device *net);
 extern u32 usbnet_get_msglevel(struct net_device *);
 extern void usbnet_set_msglevel(struct net_device *, u32);
-- 
2.30.1

[PATCH net-next v4 3/4] net: cdc_ncm: record speed in status method

[Grant Grundler]

From: Oliver Neukum <oneukum@suse.com>

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 funtions (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/state.
Link speed/state is now available via ethtool.

This is especially useful given all current 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"

v2: rebased on upstream
v3: changed variable names
v4: rewrote commit message

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Tested-by: Roland Dreier <roland@kernel.org>
Signed-off-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/usb/cdc_ncm.c | 55 ++++++++++++---------------------------
 1 file changed, 17 insertions(+), 38 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 2644234d4c4d..6f330c7dcad2 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -133,17 +133,17 @@ static void cdc_ncm_get_strings(struct net_device __always_unused *netdev, u32 s
 static void cdc_ncm_update_rxtx_max(struct usbnet *dev, u32 new_rx, u32 new_tx);
 
 static const struct ethtool_ops cdc_ncm_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_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_mii,
-	.set_link_ksettings      = usbnet_set_link_ksettings_mii,
+	.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_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_internal,
+	.set_link_ksettings	= NULL,
 };
 
 static u32 cdc_ncm_check_rx_max(struct usbnet *dev, u32 new_rx)
@@ -1826,33 +1826,9 @@ static void
 cdc_ncm_speed_change(struct usbnet *dev,
 		     struct usb_cdc_speed_change *data)
 {
-	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->rx_speed = le32_to_cpu(data->DLBitRRate);
+	dev->tx_speed = le32_to_cpu(data->ULBitRate);
 }
 
 static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
@@ -1878,6 +1854,9 @@ static void cdc_ncm_status(struct usbnet *dev, struct urb *urb)
 		 * USB_CDC_NOTIFY_NETWORK_CONNECTION notification shall be
 		 * sent by device after USB_CDC_NOTIFY_SPEED_CHANGE.
 		 */
+		/* RTL8156 shipped before 2021 sends notification about
+		 * every 32ms. Don't forward notification if state is same.
+		 */
 		if (netif_carrier_ok(dev->net) != !!event->wValue)
 			usbnet_link_change(dev, !!event->wValue, 0);
 		break;
-- 
2.30.1

[PATCH net-next v4 4/4] net: cdc_ether: record speed in status method

[Grant Grundler]

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.

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.

User space can now get the most recent link speed/state using ethtool.

v4: added to series since cdc_ether uses same notifications
    as cdc_ncm driver.

Signed-off-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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..7eb0109e9baa 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.1

Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[Andrew Lunn]

On Mon, Apr 05, 2021 at 04:13:40PM -0700, Grant Grundler 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: rebased on recent upstream changes
> v3: incorporated hints on naming and comments
> v4: fix misplaced hunks; reword some commit messages;
>     add same change for cdc_ether
> v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by
> 
> I'm reposting Oliver Neukum's <oneukum@suse.com> patch series with
> fix ups for "misplaced hunks" (landed in the wrong patches).
> Please fixup the "author" if "git am" fails to attribute the
> patches 1-3 (of 4) to Oliver.
> 
> I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB
> and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel)
> + Alpha Network AUE2500C were connected directly to the NT-S25G
> to get 2.5Gbps link rate:
> # ethtool enx002427880815
> Settings for enx002427880815:
>         Supported ports: [  ]
>         Supported link modes:   Not reported
>         Supported pause frame use: No
>         Supports auto-negotiation: No
>         Supported FEC modes: Not reported
>         Advertised link modes:  Not reported
>         Advertised pause frame use: No
>         Advertised auto-negotiation: No
>         Advertised FEC modes: Not reported
>         Speed: 2500Mb/s
>         Duplex: Half
>         Auto-negotiation: off
>         Port: Twisted Pair
>         PHYAD: 0
>         Transceiver: internal
>         MDI-X: Unknown
>         Current message level: 0x00000007 (7)
>                                drv probe link
>         Link detected: yes
> 
> 
> "Duplex" is a lie since we get no information about it.

You can ask the PHY. At least those using mii or phylib.  If you are
using mii, then mii_ethtool_get_link_ksettings() should set it
correctly. If you are using phylib, phy_ethtool_get_link_ksettings()
will correctly set it. If you are not using either of these, you are
on your own.

Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only
ever see 10 Half and occasionally 100 Half. Anything above that will
be full duplex.

It is probably best to admit the truth and use DUPLEX_UNKNOWN.

> I expect "Auto-Negotiation" is always true for cdc_ncm and
> cdc_ether devices and perhaps someone knows offhand how
> to have ethtool report "true" instead.

ethtool_link_ksettings contains three bitmaps:

supported: The capabilities of this device.
advertising: What this device is telling the link peer it can do.
lp_advertising: What the link peer is telling us it can do.

So to get Supports auto-negotiation to be true you need to set bit
ETHTOOL_LINK_MODE_Autoneg_BIT in supported.
For Advertised auto-negotiation: you need to set the same bit in
advertising. 

Auto-negotiation: off is i think from base.autoneg.

	Andrew

Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[Grant Grundler]

On Tue, Apr 6, 2021 at 12:09 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Apr 05, 2021 at 04:13:40PM -0700, Grant Grundler 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: rebased on recent upstream changes
> > v3: incorporated hints on naming and comments
> > v4: fix misplaced hunks; reword some commit messages;
> >     add same change for cdc_ether
> > v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by
> >
> > I'm reposting Oliver Neukum's <oneukum@suse.com> patch series with
> > fix ups for "misplaced hunks" (landed in the wrong patches).
> > Please fixup the "author" if "git am" fails to attribute the
> > patches 1-3 (of 4) to Oliver.
> >
> > I've tested v4 series with "5.12-rc3+" kernel on Intel NUC6i5SYB
> > and + Sabrent NT-S25G. Google Pixelbook Go (chromeos-4.4 kernel)
> > + Alpha Network AUE2500C were connected directly to the NT-S25G
> > to get 2.5Gbps link rate:
> > # ethtool enx002427880815
> > Settings for enx002427880815:
> >         Supported ports: [  ]
> >         Supported link modes:   Not reported
> >         Supported pause frame use: No
> >         Supports auto-negotiation: No
> >         Supported FEC modes: Not reported
> >         Advertised link modes:  Not reported
> >         Advertised pause frame use: No
> >         Advertised auto-negotiation: No
> >         Advertised FEC modes: Not reported
> >         Speed: 2500Mb/s
> >         Duplex: Half
> >         Auto-negotiation: off
> >         Port: Twisted Pair
> >         PHYAD: 0
> >         Transceiver: internal
> >         MDI-X: Unknown
> >         Current message level: 0x00000007 (7)
> >                                drv probe link
> >         Link detected: yes
> >
> >
> > "Duplex" is a lie since we get no information about it.
>
> You can ask the PHY. At least those using mii or phylib.  If you are
> using mii, then mii_ethtool_get_link_ksettings() should set it
> correctly. If you are using phylib, phy_ethtool_get_link_ksettings()
> will correctly set it. If you are not using either of these, you are
> on your own.
>
> Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only
> ever see 10 Half and occasionally 100 Half. Anything above that will
> be full duplex.
>
> It is probably best to admit the truth and use DUPLEX_UNKNOWN.

Agreed. I didn't notice this "lie" until I was writing the commit
message and wasn't sure off-hand how to fix it. Decided a follow on
patch could fix it up once this series lands.

You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default.
Additionally, if RX and TX speed are equal, I am willing to assume
this is DUPLEX_FULL.
I can propose something like this in a patch:

grundler <1637>git diff
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 86eb1d107433..a7ad9a0fb6ae 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct
net_device *net,
        else
                cmd->base.speed = SPEED_UNKNOWN;

+       if (dev->rx_speed == dev->tx_speed)
+               cmd->base.duplex = DUPLEX_FULL;
+       else
+               cmd->base.duplex =DUPLEX_UNKNOWN;
+
        return 0;
 }
 EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);

Probably should check that link speed is > 100Mbps to be more certain
about this assumption (based on your comments above).

I can send this out later once this series lands or you are welcome to
post this with additional checks if you like.

The messy case is when RX != TX speed and I didn't want to delay
landing the current series to figure out DUPLEX.

> > I expect "Auto-Negotiation" is always true for cdc_ncm and
> > cdc_ether devices and perhaps someone knows offhand how
> > to have ethtool report "true" instead.
>
> ethtool_link_ksettings contains three bitmaps:
>
> supported: The capabilities of this device.
> advertising: What this device is telling the link peer it can do.
> lp_advertising: What the link peer is telling us it can do.
>
> So to get Supports auto-negotiation to be true you need to set bit
> ETHTOOL_LINK_MODE_Autoneg_BIT in supported.
> For Advertised auto-negotiation: you need to set the same bit in
> advertising.
>
> Auto-negotiation: off is i think from base.autoneg.

Thanks for explaining! :)  I understand the three bitmaps. I just
hadn't taken the time to figure out how to access/set those from
link_ksettings API.

If we want to assume autoneg is always on (regardless of which type of
media cdc_ncm/cdc_ether are talking to), we could set both supported
and advertising to AUTO and lp_advertising to UNKNOWN. But I would
prefer to add more checks to prove this is correct (vs making "well
intentioned" assumptions).

cheers,
grant

Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[Andrew Lunn]

> > Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only
> > ever see 10 Half and occasionally 100 Half. Anything above that will
> > be full duplex.
> >
> > It is probably best to admit the truth and use DUPLEX_UNKNOWN.
> 
> Agreed. I didn't notice this "lie" until I was writing the commit
> message and wasn't sure off-hand how to fix it. Decided a follow on
> patch could fix it up once this series lands.
> 
> You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default.
> Additionally, if RX and TX speed are equal, I am willing to assume
> this is DUPLEX_FULL.

Is this same interface used by WiFi? Ethernet does not support
different rates in each direction. So if RX and TX are different, i
would actually say something is broken. 10 Half is still doing 10Mbps
in each direction, it just cannot do both at the same time.
WiFi can have asymmetric speeds.

> I can propose something like this in a patch:
> 
> grundler <1637>git diff
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 86eb1d107433..a7ad9a0fb6ae 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct
> net_device *net,
>         else
>                 cmd->base.speed = SPEED_UNKNOWN;
> 
> +       if (dev->rx_speed == dev->tx_speed)
> +               cmd->base.duplex = DUPLEX_FULL;
> +       else
> +               cmd->base.duplex =DUPLEX_UNKNOWN;
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);

So i would say this is wrong. I would just set DUPLEX_UNKNOWN and be
done.

> I can send this out later once this series lands or you are welcome to
> post this with additional checks if you like.

Yes, this discussion should not prevent this patchset from being
merged.

> If we want to assume autoneg is always on (regardless of which type of
> media cdc_ncm/cdc_ether are talking to), we could set both supported
> and advertising to AUTO and lp_advertising to UNKNOWN.

I pretty much agree autoneg has to be on. If it is not, and it is
using a forced speed, there would need to be an additional API to set
what it is forced to. There could be such proprietary calls, but the
generic cdc_ncm/cdc_ether won't support them.

But i also don't know how setting autoneg actually helps the user.
Everybody just assumes it is supported. If you really know auto-neg is
not supported and you can reliably indicate that autoneg is not
supported, that would be useful. But i expect most users want to know
if their USB 2.0 device is just doing 100Mbps, or if their USB 3.0
device can do 2.5G. For that, you need to see what is actually
supported.

	Andrew

Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[Grant Grundler]

[Key part of Andew's reply: "Yes, this discussion should not prevent
this patchset from being merged."]

On Tue, Apr 6, 2021 at 1:00 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > Speed: 2500Mb/s and Duplex: Half is very unlikely. You really only
> > > ever see 10 Half and occasionally 100 Half. Anything above that will
> > > be full duplex.
> > >
> > > It is probably best to admit the truth and use DUPLEX_UNKNOWN.
> >
> > Agreed. I didn't notice this "lie" until I was writing the commit
> > message and wasn't sure off-hand how to fix it. Decided a follow on
> > patch could fix it up once this series lands.
> >
> > You are right that DUPLEX_UNKNOWN is the safest (and usually correct) default.
> > Additionally, if RX and TX speed are equal, I am willing to assume
> > this is DUPLEX_FULL.
>
> Is this same interface used by WiFi?

I doubt WiFi could work with this driver interface (though maybe with
"SendEncapsulatedCommand").
All the Wifi Devices I'm familiar with need WPA support and
communicate through 80211 kernel subsystem.

I was thinking of just about everything else: Cellular modem
(cdc_ether), xDSL, or other broadband.

> Ethernet does not support
> different rates in each direction. So if RX and TX are different, i
> would actually say something is broken.

Agreed. The question is: Is there data or some heuristics we can use
to determine if the physical layer/link is ethernet?
I'm pessimistic we will be able to since this is at odds with the
intent of the CDC spec.

> 10 Half is still doing 10Mbps
> in each direction, it just cannot do both at the same time.
> WiFi can have asymmetric speeds.

*nod*

> > I can propose something like this in a patch:
> >
> > grundler <1637>git diff
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > index 86eb1d107433..a7ad9a0fb6ae 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -978,6 +978,11 @@ int usbnet_get_link_ksettings_internal(struct
> > net_device *net,
> >         else
> >                 cmd->base.speed = SPEED_UNKNOWN;
> >
> > +       if (dev->rx_speed == dev->tx_speed)
> > +               cmd->base.duplex = DUPLEX_FULL;
> > +       else
> > +               cmd->base.duplex =DUPLEX_UNKNOWN;
> > +
> >         return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_internal);
>
> So i would say this is wrong. I would just set DUPLEX_UNKNOWN and be
> done.

Ok.

> > I can send this out later once this series lands or you are welcome to
> > post this with additional checks if you like.
>
> Yes, this discussion should not prevent this patchset from being
> merged.

Good. That's what I'm hoping for.

> > If we want to assume autoneg is always on (regardless of which type of
> > media cdc_ncm/cdc_ether are talking to), we could set both supported
> > and advertising to AUTO and lp_advertising to UNKNOWN.
>
> I pretty much agree autoneg has to be on. If it is not, and it is
> using a forced speed, there would need to be an additional API to set
> what it is forced to. There could be such proprietary calls, but the
> generic cdc_ncm/cdc_ether won't support them.

Good observation. Agreed.

> But i also don't know how setting autoneg actually helps the user.

Just to let them know the link rate can change and is dynamically determined.

> Everybody just assumes it is supported. If you really know auto-neg is
> not supported and you can reliably indicate that autoneg is not
> supported, that would be useful. But i expect most users want to know
> if their USB 2.0 device is just doing 100Mbps, or if their USB 3.0
> device can do 2.5G. For that, you need to see what is actually
> supported.

Yes. Other than using a table to look up USB VID:PID, I don't see
anything in the spec which provides "media-specific" information.

I was curious about the "can do 2.5Gbps?" question by looking at the
CDC Ethernet Networking Functional Descriptor (USBECM12) and other CDC
specs. The spec feels like a "compatibility wrapper" to make a
cellular modem look like an ethernet device. This statement in the
ECM120.pdf I have suggests we can not determine media layer:
    The effect of a "reset" on the device physical layer is
media-dependent and beyond the scope of this specification.

cheers,
grant

Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[Oliver Neukum]

Am Dienstag, den 06.04.2021, 18:01 +0000 schrieb Grant Grundler:

> > Ethernet does not support
> > different rates in each direction. So if RX and TX are different, i
> > would actually say something is broken.
> 
> Agreed. The question is: Is there data or some heuristics we can use
> to determine if the physical layer/link is ethernet?
> I'm pessimistic we will be able to since this is at odds with the
> intent of the CDC spec.

Yes, CDC intends to hide that. We can conclude that an asymmetric link
rules out ethernet, but anything else is difficult.

	Regards
		Oliver

Re: [PATCH net-next v4 0/4] usbnet: speed reporting for devices without MDIO

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Mon,  5 Apr 2021 16:13:40 -0700 you 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: rebased on recent upstream changes
> v3: incorporated hints on naming and comments
> v4: fix misplaced hunks; reword some commit messages;
>     add same change for cdc_ether
> v4-repost: added "net-next" to subject and Andrew Lunn's Reviewed-by
> 
> [...]

Here is the summary with links:
  - [net-next,v4,1/4] usbnet: add _mii suffix to usbnet_set/get_link_ksettings
    https://git.kernel.org/netdev/net-next/c/77651900cede
  - [net-next,v4,2/4] usbnet: add method for reporting speed without MII
    https://git.kernel.org/netdev/net-next/c/956baa99571b
  - [net-next,v4,3/4] net: cdc_ncm: record speed in status method
    https://git.kernel.org/netdev/net-next/c/eb47c274d8c4
  - [net-next,v4,4/4] net: cdc_ether: record speed in status method
    https://git.kernel.org/netdev/net-next/c/d42ebcbb6353

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html