netdev - Re: [PATCH] CDC-NCM: remove "connected" log message

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: <CAG4TOxOOPgAqUtX14V7k-qPCbOm7+5gaHOqBvgWBYQwJkO6v8g@mail.gmail.com>
Date:   Thu, 31 Dec 2020 10:51:09 -0800
From:   Roland Dreier <roland@...nel.org>
To:     Oliver Neukum <oneukum@...e.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH] CDC-NCM: remove "connected" log message

I haven't tried these patches yet but they don't look quite right to
me.  inlining the first 0001 patch:

 > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
 > index 1447da1d5729..bcd17f6d6de6 100644
 > --- a/drivers/net/usb/usbnet.c
 > +++ b/drivers/net/usb/usbnet.c
 > @@ -944,7 +944,7 @@ EXPORT_SYMBOL_GPL(usbnet_open);
 >   * they'll probably want to use this base set.
 >   */
 >
 > -int usbnet_get_link_ksettings(struct net_device *net,
 > +int usbnet_get_link_ksettings_mdio(struct net_device *net,
 >                    struct ethtool_link_ksettings *cmd)
 >  {
 >      struct usbnet *dev = netdev_priv(net);
 > @@ -956,6 +956,32 @@ int usbnet_get_link_ksettings(struct net_device *net,
 >
 >      return 0;
 >  }
 > +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio);

why keep and export the old function when it will have no callers?

 > +int usbnet_get_link_ksettings(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 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 a minidriver does not record speed we try to
 > +     * fall back on MDIO
 > +     */
 > +    else if (!dev->mii.mdio_read)
 > +        cmd->base.speed = SPEED_UNKNOWN;
 > +    else
 > +        mii_ethtool_get_link_ksettings(&dev->mii, cmd);
 > +
 > +    return 0;

This is a change in behavior for every driver that doesn't set rxspeed
/ txspeed - the old get_link function would return EOPNOTSUPP if
mdio_read isn't implemented, now we give SPEED_UNKNOWN with a
successful return code.

 > @@ -1661,6 +1687,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->rxspeed = -1; /* unknown or handled by MII */
 > +    dev->txspeed = -1;

Minor nit: if we're going to test these against SPEED_UNKNOWN above,
then I think it's clearer to initialize them to that value via the
same constant.

 > diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
 > index 88a7673894d5..f748c758f82a 100644
 > --- a/include/linux/usb/usbnet.h
 > +++ b/include/linux/usb/usbnet.h
 > @@ -267,8 +269,11 @@ extern void usbnet_purge_paused_rxq(struct usbnet *);
 >
 >  extern int usbnet_get_link_ksettings(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_mdio(struct net_device *net,
 >                       const struct ethtool_link_ksettings *cmd);
 > +/* Legacy - to be used if you really need an error to be returned */
 > +extern int usbnet_set_link_ksettings(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 *);
 >  extern void usbnet_set_msglevel(struct net_device *, u32);

I think this was meant to be changing get_link, not set_link.

Also I don't understand the "Legacy" comment.  Is that referring to
the EOPNOTSUPP change I mentioned above?  If so, wouldn't it be better
to preserve the legacy behavior rather than changing the behavior of
every usbnet driver all at once?  Like make a new
usbnet_get_link_ksettings_nonmdio and update only cdc_ncm to use it?

 - R.