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]

Message-ID: <CANEJEGvsYPmnxx2sV89aLPJzK_SgWEW8FPhgo2zNk2d5zijK2Q@mail.gmail.com>
Date:   Fri, 19 Feb 2021 07:30:29 +0000
From:   Grant Grundler <grundler@...omium.org>
To:     Oliver Neukum <oneukum@...e.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Grant Grundler <grundler@...omium.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

On Thu, Feb 18, 2021 at 10:21 AM Oliver Neukum <oneukum@...e.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@...e.com>
> Tested-by: Roland Dreier <roland@...nel.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
>