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

Hi Oliver,

On Mon, Feb 22, 2021 at 10:14 AM Oliver Neukum [off-list ref] wrote:

Am Freitag, den 19.02.2021, 07:30 +0000 schrieb Grant Grundler:

quoted

On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum [off-list ref] wrote:

Hi,

quoted

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. :)

quoted

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