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
