Jakub Kicinski wrote on Mon, Dec 02, 2024 at 06:29:35PM -0800:
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")
We moved where the random address is assigned, we used to assign random (local) addr at init, now we assign it after calling ->bind().
Previously we checked "if local" as a shorthand for checking "if driver updated". This check should really have been "if addr == node_id".
Ok, so a zero address here means a driver didn't set it, because the ex-"node_id" address was no longer set at this point, and these would fail the 0x2 check that worked previously...
The third time's a charm, the ordering part of the message just clicked for me, thank you for putting up with me.
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.
Let the compiler discover that, this is control path code, so write it for the human reader... The condition we want is "driver did not initialize the MAC address, or it initialized it to a local MAC address".
(I was reading that '&& !' wrong here, having the check negated in the helper is much more clear and it's required to keep the two anyway...)
Or if we go with the local address version, something like the following?
[...]
Up to you if you want to send this.
Thank you; after thinking it through today I think it won't hurt further to send so I did. Almost everyone involved is in Cc, but for follow-up it is here: https://lore.kernel.org/r/20241203130457.904325-1-asmadeus@codewreck.org
Thanks,