Hi Brian,
It's been a while, but I'd like to get this forward now.
On Fri, Oct 04, 2024 at 04:15:32PM -0700, Brian Norris wrote:
I think I'm generally supportive of the direction this changes things, but I'm a bit hesitant about two things:
- the potential user-visible changes and
- the linux-stable backport (Cc stable below)
For 1: MAC addresses are important in some contexts, and this might significantly change the addresses that devices get in practice. Such users might not really care about the weird details of when the address incremented; but they *probably* care that a certain sequence of "boot device; run hostapd with <foo> config file" produces the same address.
Also, I'm not sure I know enough of the implications of potential over-use of the locally administered bit. Are there significant downsides to it (aside from the simple fact that it's a different address)?
Not that I know of, but that doesn't mean much.
And I see that you rightly don't know how many addresses are actually reserved, but I have an educated guess that it's not just 1.
Even if there are more addresses reserved, we don't know which these are, see below.
For one, this driver used to default-create 3 interfaces: 1211c961170c mwifiex: do not create AP and P2P interfaces upon driver loading
and when we stopped doing that, we still kept support for a module parameter for the old way: 0013c7cebed6 mwifiex: module load parameter for interface creation
Perhaps these "initial" interfaces should at least be allowed permanent addresses?
I started up a board with the downstream driver. It comes up with these MAC addresses:
wlp1s0 Link encap:Ethernet HWaddr 34:6F:24:4E:E0:3D uap0 Link encap:Ethernet HWaddr 36:6F:24:4E:E1:3D wfd0 Link encap:Ethernet HWaddr 36:6F:24:4E:E0:3D
The permanent address from EEPROM is 34:6F:24:4E:E0:3D which is used for wlp1s0. For the other addresses the locally admistered bit is set (34 -> 36). Here's the corresponding code:
if (priv->bss_type == MLAN_BSS_TYPE_WIFIDIRECT) { if (priv->bss_virtual) { ... } else { priv->current_addr[0] |= 0x02; } }
if (priv->bss_type != MLAN_BSS_TYPE_WIFIDIRECT) { if (priv->bss_index) { priv->current_addr[0] |= 0x02; priv->current_addr[4] += priv->bss_index; } }
See https://github.com/nxp-imx/mwifiex/blob/lf-6.6.3_1.0.0/mxm_wifiex/wlan_src/m...
Note this behaviour was changed in the driver in a0835444f1 ("mxm_wifiex: update to mxm5x17344.p1 release"). Before that the driver has just done a priv->current_addr[4] += priv->bss_index without setting the locally admistered bit. Of course the commit message says nothing about the reasons for this change.
The downstream driver puts the bss_num (or bss_index) into different bits than the upstream driver does. It uses current_addr[4] whereas the upstream driver uses current_addr[5]. So even when there's more than one MAC address reserved for one chip, both drivers disagree on which addresses these are.
Given that, I think our safest bet is to always set the locally admistered bit for derived MAC addresses.
So anyway, I don't really know for sure the right answer, but I want to log my concerns, in case you had more thoughts on backward compatibility.
And given all the uncertainty above, I'm extra hesitant about backporting likely-user-visible changes to stable (#2).
I can remove the stable tag if makes you feel more comfortable.
Sascha