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:
> 1. the potential user-visible changes and
> 2. 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/mlinux/moal_main.c#L8383
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
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
