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

[Oliver Neukum <oneukum@suse.com>]

Am Donnerstag, den 31.12.2020, 10:51 -0800 schrieb Roland Dreier:
> I haven't tried these patches yet but they don't look quite right to
> me.  inlining the first 0001 patch:

OK, let's see.

>  > 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
[...]
>  > +EXPORT_SYMBOL_GPL(usbnet_get_link_ksettings_mdio);
> 
> why keep and export the old function when it will have no callers?

But they will callers. Have a dozen drivers use them. The goal
of this patch set is to not touch them.
> 
>  > +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.

Yes. This is a drawback. Yet the speed is unknown is it not?

>  > @@ -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.

Correct. The next iteration will do that.

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

Yes.

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

Then I would have to touch them all. The problem is that the MDIO
stuff really is pretty much a layering violation. It should never
have been default. But now it is.

	Regards
		Oliver