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

[Grant Grundler <grundler@chromium.org>]

[-- 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