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