Jakub Kicinski wrote on Mon, Dec 02, 2024 at 04:26:53PM -0800:
My problematic device here has FLAG_POINTTOPOINT and a (locally admistered) mac address set, so it was not renamed up till now, but the new check makes the locally admistered mac address being set mean that it is no longer eligible to keep the usbX name.
Ideally, udev would be the best option, like Greg said. This driver is already a fragile pile of workarounds.
Right, as I replied to Greg I'm fine with this as long as it's what was intended.
Half of the reason I sent the mail in the first place is I don't understand what commit 8a7d12d674ac ("net: usb: usbnet: fix name regression") actually fixes: the commit message desribes something about mac address not being set before bind() but the code does not change what address is looked at (net->dev_addr), just which bits of the address is checked; and I don't see what which bytes are being looked at changing has anything to do with the "fixed" commit bab8eb0dd4cb9 ("usbnet: modern method to get random MAC") ... And now we've started discussing this and I understand the check better, I also don't see what having a mac set by the previous driver has to do with the link not being P2P either.
(The other half was I was wondering what kind of policy stable would have for this kind of things, but that was made clear enough)
If you really really want the old behavior tho, let's convert the zero check to !is_zero_ether_addr() && !is_local_ether_addr().
As far as I understand, !is_local_ether_addr (mac[0] & 0x2) implies !is_zero_ether_addr (all bits of mac or'd), so that'd get us back to exactly the old check.
Maybe factor out the P2P + address validation to a helper because the && vs || is getting complicated.
... And I can definitely relate to this part :)
So: - final behavior wise I have no strong feeling, we'll fix our userspace (... and documentation) whatever is decided here - let's improve the comment and factor the check anyway. As said above I don't understand why having a mac set matters, if that can be explained I'll be happy to send a patch. Or if we go with the local address version, something like the following?
---- diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 44179f4e807f..240ae86adf08 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -178,6 +178,13 @@ int usbnet_get_ethernet_addr(struct usbnet *dev, int iMACAddress) } EXPORT_SYMBOL_GPL(usbnet_get_ethernet_addr);
+static bool usbnet_dev_is_two_host (struct usbnet *dev, struct net_device *net) +{ + /* device is marked point-to-point with a local mac address */ + return (dev->driver_info->flags & FLAG_POINTTOPOINT) != 0 && + is_local_ether_addr(net->dev_addr); +} + static void intr_complete (struct urb *urb) { struct usbnet *dev = urb->context; @@ -1762,13 +1769,10 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) if (status < 0) goto out1;
- // heuristic: "usb%d" for links we know are two-host, - // else "eth%d" when there's reasonable doubt. userspace - // can rename the link if it knows better. + /* heuristic: rename to "eth%d" if we are not sure this link + * is two-host (these links keep "usb%d") */ if ((dev->driver_info->flags & FLAG_ETHER) != 0 && - ((dev->driver_info->flags & FLAG_POINTTOPOINT) == 0 || - /* somebody touched it*/ - !is_zero_ether_addr(net->dev_addr))) + !usbnet_dev_is_two_host(dev, net)) strscpy(net->name, "eth%d", sizeof(net->name)); /* WLAN devices should always be named "wlan%d" */ if ((dev->driver_info->flags & FLAG_WLAN) != 0) ----
Thanks,