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

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