| Message ID | 20231116123033.26035-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] usbnet: assign unique random MAC | expand |
Oliver Neukum <oneukum@suse.com> writes: > A module parameter to go back to the old behavior > is included. Is this really required? If we add it now then we can never get rid of it. Why not try without, and add this back if/when somebody complains about the new behaviour? I believe there's a fair chance no one will notice or complain. And we have much cleaner code and one module param less. Bjørn
On 16.11.23 13:39, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.com> writes: > >> A module parameter to go back to the old behavior >> is included. > > Is this really required? If we add it now then we can never get rid of > it. Why not try without, and add this back if/when somebody complains > about the new behaviour? > > I believe there's a fair chance no one will notice or complain. And we > have much cleaner code and one module param less. Isn't it a bit evil to change behavior? Do you think I should make a different version for stable with the logic for retaining the old behavior inverted? Regards Oliver
Oliver Neukum <oneukum@suse.com> writes: > On 16.11.23 13:39, Bjørn Mork wrote: >> Oliver Neukum <oneukum@suse.com> writes: >> >>> A module parameter to go back to the old behavior >>> is included. >> Is this really required? If we add it now then we can never get rid >> of >> it. Why not try without, and add this back if/when somebody complains >> about the new behaviour? >> I believe there's a fair chance no one will notice or complain. And >> we >> have much cleaner code and one module param less. > > Isn't it a bit evil to change behavior? Only if someone actually depend on the old behaviour. And I think there's a fair chance no one does. I don't propose forcing this change on anyone. Only to try and see if we can apply if without any force involved. Note that the module parameter solution also will be a breaking change for anyone depending on the current behaviour. If you want to avoid that, then you need to invert the logic. And then you might as well drop the whole change. > Do you think I should make a different version for stable > with the logic for retaining the old behavior inverted? I assumed this was unsuitable for stable backports. Is there any reason to backport it? Bjørn
On 16.11.23 14:21, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.com> writes: > >> On 16.11.23 13:39, Bjørn Mork wrote: >>> Oliver Neukum <oneukum@suse.com> writes: >> Isn't it a bit evil to change behavior? > > Only if someone actually depend on the old behaviour. And I think > there's a fair chance no one does. Very well. I'll take it out. >> Do you think I should make a different version for stable >> with the logic for retaining the old behavior inverted? > > I assumed this was unsuitable for stable backports. Is there any reason > to backport it? You could argue that handing out the same MAC twice violates standards. Regards Oliver
Oliver Neukum <oneukum@suse.com> writes: > You could argue that handing out the same MAC twice > violates standards. You can't argue that to the Sparc crowd. Which has to be considered when sending stuff to netdev :-) Bjørn
On 16.11.23 15:49, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.com> writes: > >> You could argue that handing out the same MAC twice >> violates standards. > > You can't argue that to the Sparc crowd. Which has to be considered > when sending stuff to netdev :-) Should I reword the commit message? Regards Oliver
On Thu, 16 Nov 2023 13:30:24 +0100 Oliver Neukum <oneukum@suse.com> wrote: > module_exit(usbnet_exit); > > +module_param(legacymac, bool, S_IRUGO | S_IWUSR); > +MODULE_PARM_DESC(legacymac, "Use a legacy style common MAC if device need a random MAC"); > MODULE_AUTHOR("David Brownell"); Module params are bad idea, just do the right thing.
On Thu, Nov 16, 2023 at 01:39:59PM +0100, Bjørn Mork wrote: > Oliver Neukum <oneukum@suse.com> writes: > > > A module parameter to go back to the old behavior > > is included. > > Is this really required? If we add it now then we can never get rid of > it. Why not try without, and add this back if/when somebody complains > about the new behaviour? > > I believe there's a fair chance no one will notice or complain. And we > have much cleaner code and one module param less. As Stephen pointed out, module parameters are not really liked in netdev. I suggest not having it. Post this patch for net-next, and don't make the commit message sound like it is a fix, otherwise it might get back ported by the Machine Learning fix spotting bot, which we probably don't want. Andrew --- pw-bot: cr
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 2d14b0d78541..53cb3a8d48c3 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -61,8 +61,10 @@ /*-------------------------------------------------------------------------*/ -// randomly generated ethernet address -static u8 node_id [ETH_ALEN]; +/* for the legacy behavior */ + +u8 legacyrandomid[ETH_ALEN]; +static bool legacymac = false; /* use ethtool to change the level for any given device */ static int msg_level = -1; @@ -1731,7 +1733,6 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) dev->net = net; strscpy(net->name, "usb%d", sizeof(net->name)); - eth_hw_addr_set(net, node_id); /* rx and tx sides can use different message sizes; * bind() should set rx_urb_size in that case. @@ -1805,9 +1806,19 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) goto out4; } - /* let userspace know we have a random address */ - if (ether_addr_equal(net->dev_addr, node_id)) - net->addr_assign_type = NET_ADDR_RANDOM; + /* + * if the device does not come with a MAC + * we ask the network core to generate us one + * and flag the device accordingly + */ + if (!is_valid_ether_addr(net->dev_addr)) { + if (legacymac) { + eth_hw_addr_set(net, legacyrandomid); + net->addr_assign_type = NET_ADDR_RANDOM; + } else { + eth_hw_addr_random(net); + } + } if ((dev->driver_info->flags & FLAG_WLAN) != 0) SET_NETDEV_DEVTYPE(net, &wlan_type); @@ -2217,7 +2228,7 @@ static int __init usbnet_init(void) BUILD_BUG_ON( sizeof_field(struct sk_buff, cb) < sizeof(struct skb_data)); - eth_random_addr(node_id); + eth_random_addr(legacyrandomid); return 0; } module_init(usbnet_init); @@ -2227,6 +2238,8 @@ static void __exit usbnet_exit(void) } module_exit(usbnet_exit); +module_param(legacymac, bool, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(legacymac, "Use a legacy style common MAC if device need a random MAC"); MODULE_AUTHOR("David Brownell"); MODULE_DESCRIPTION("USB network driver framework"); MODULE_LICENSE("GPL");
The old method had the bug of issuing the same random MAC over and over even to every device. This bug is as old as the driver. This new method generates each device whose minidriver does not provide its own MAC its own unique random MAC. A module parameter to go back to the old behavior is included. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/usbnet.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)