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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Feb 2021 05:24:20 +0000
From:   Grant Grundler <grundler@...omium.org>
To:     Oliver Neukum <oneukum@...e.com>
Cc:     Grant Grundler <grundler@...omium.org>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        davem@...emloft.org, Hayes Wang <hayeswang@...ltek.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Roland Dreier <roland@...nel.org>,
        nic_swsd <nic_swsd@...ltek.com>
Subject: Re: [PATCHv3 3/3] CDC-NCM: record speed in status method

Hi Oliver,

On Mon, Feb 22, 2021 at 10:14 AM Oliver Neukum <oneukum@...e.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@...e.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

View attachment "0001-net-cdc_ether-record-speed-in-status-method.patch" of type "text/x-patch" (3555 bytes)