| Message ID | 20220708133712.102179-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4af4c0b93c15ece3732dee5e5dbcf0a01904413a |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2] brcmfmac: Add brcmf_c_set_cur_etheraddr() helper | expand |
On 7/8/2022 3:37 PM, Hans de Goede wrote: > On some boards there is no eeprom to hold the nvram, in this case instead > a board specific nvram is loaded from /lib/firmware. On most boards the > macaddr=... setting in the /lib/firmware nvram file is ignored because > the wifi/bt chip has a unique MAC programmed into the chip itself. > > But in some cases the actual MAC from the /lib/firmware nvram file gets > used, leading to MAC conflicts. > > The MAC addresses in the troublesome nvram files seem to all come from > the same nvram file template, so we can detect this by checking for > the template nvram file MAC. > > Detect that the default MAC address is being used and replace it > with a random MAC address to avoid MAC address conflicts. > > Note that udev will detect this is a random MAC based on > /sys/class/net/wlan0/addr_assign_type and then replace this with > a MAC based on hashing the netdev-name + the machine-id. So that > the MAC address is both guaranteed to be unique per machine while > it is still the same/persistent at each boot (assuming the > default Link.MACAddressPolicy=persistent udev setting). Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > .../broadcom/brcm80211/brcmfmac/common.c | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > index dccd8f4ca1d0..7485e784be2a 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c [...] > @@ -226,6 +240,15 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) > bphy_err(drvr, "Retrieving cur_etheraddr failed, %d\n", err); > goto done; > } > + > + if (ether_addr_equal_unaligned(ifp->mac_addr, brcmf_default_mac_address)) { Is it necessary to use the unaligned variant? Regards, Arend
Hi, On 7/8/22 20:27, Arend Van Spriel wrote: > On 7/8/2022 3:37 PM, Hans de Goede wrote: >> On some boards there is no eeprom to hold the nvram, in this case instead >> a board specific nvram is loaded from /lib/firmware. On most boards the >> macaddr=... setting in the /lib/firmware nvram file is ignored because >> the wifi/bt chip has a unique MAC programmed into the chip itself. >> >> But in some cases the actual MAC from the /lib/firmware nvram file gets >> used, leading to MAC conflicts. >> >> The MAC addresses in the troublesome nvram files seem to all come from >> the same nvram file template, so we can detect this by checking for >> the template nvram file MAC. >> >> Detect that the default MAC address is being used and replace it >> with a random MAC address to avoid MAC address conflicts. >> >> Note that udev will detect this is a random MAC based on >> /sys/class/net/wlan0/addr_assign_type and then replace this with >> a MAC based on hashing the netdev-name + the machine-id. So that >> the MAC address is both guaranteed to be unique per machine while >> it is still the same/persistent at each boot (assuming the >> default Link.MACAddressPolicy=persistent udev setting). > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> .../broadcom/brcm80211/brcmfmac/common.c | 23 +++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c >> index dccd8f4ca1d0..7485e784be2a 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > > [...] > >> @@ -226,6 +240,15 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) >> bphy_err(drvr, "Retrieving cur_etheraddr failed, %d\n", err); >> goto done; >> } >> + >> + if (ether_addr_equal_unaligned(ifp->mac_addr, brcmf_default_mac_address)) { > > Is it necessary to use the unaligned variant? The type is: u8 mac_addr[ETH_ALEN] so there is no guarantee it will be aligned. Even if it is aligned today, if someone inserts a struct member in front of it it will end up unaligned. Regards, Hans
Arend Van Spriel <aspriel@gmail.com> writes: > On 7/8/2022 3:37 PM, Hans de Goede wrote: > >> On some boards there is no eeprom to hold the nvram, in this case instead >> a board specific nvram is loaded from /lib/firmware. On most boards the >> macaddr=... setting in the /lib/firmware nvram file is ignored because >> the wifi/bt chip has a unique MAC programmed into the chip itself. >> >> But in some cases the actual MAC from the /lib/firmware nvram file gets >> used, leading to MAC conflicts. >> >> The MAC addresses in the troublesome nvram files seem to all come from >> the same nvram file template, so we can detect this by checking for >> the template nvram file MAC. >> >> Detect that the default MAC address is being used and replace it >> with a random MAC address to avoid MAC address conflicts. >> >> Note that udev will detect this is a random MAC based on >> /sys/class/net/wlan0/addr_assign_type and then replace this with >> a MAC based on hashing the netdev-name + the machine-id. So that >> the MAC address is both guaranteed to be unique per machine while >> it is still the same/persistent at each boot (assuming the >> default Link.MACAddressPolicy=persistent udev setting). > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> This is nitpicking but as you are the maintainer could you use Acked-by instead? That way I can immediately see in my patchwork script that a maintainer has acked these and I can take them. Just trying to optimise my workflow :)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index dccd8f4ca1d0..7485e784be2a 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -201,6 +201,20 @@ int brcmf_c_set_cur_etheraddr(struct brcmf_if *ifp, const u8 *addr) return err; } +/* On some boards there is no eeprom to hold the nvram, in this case instead + * a board specific nvram is loaded from /lib/firmware. On most boards the + * macaddr setting in the /lib/firmware nvram file is ignored because the + * wifibt chip has a unique MAC programmed into the chip itself. + * But in some cases the actual MAC from the /lib/firmware nvram file gets + * used, leading to MAC conflicts. + * The MAC addresses in the troublesome nvram files seem to all come from + * the same nvram file template, so we only need to check for 1 known + * address to detect this. + */ +static const u8 brcmf_default_mac_address[ETH_ALEN] = { + 0x00, 0x90, 0x4c, 0xc5, 0x12, 0x38 +}; + int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) { struct brcmf_pub *drvr = ifp->drvr; @@ -226,6 +240,15 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) bphy_err(drvr, "Retrieving cur_etheraddr failed, %d\n", err); goto done; } + + if (ether_addr_equal_unaligned(ifp->mac_addr, brcmf_default_mac_address)) { + bphy_err(drvr, "Default MAC is used, replacing with random MAC to avoid conflicts\n"); + eth_random_addr(ifp->mac_addr); + ifp->ndev->addr_assign_type = NET_ADDR_RANDOM; + err = brcmf_c_set_cur_etheraddr(ifp, ifp->mac_addr); + if (err < 0) + goto done; + } } memcpy(ifp->drvr->mac, ifp->mac_addr, sizeof(ifp->drvr->mac));
On some boards there is no eeprom to hold the nvram, in this case instead a board specific nvram is loaded from /lib/firmware. On most boards the macaddr=... setting in the /lib/firmware nvram file is ignored because the wifi/bt chip has a unique MAC programmed into the chip itself. But in some cases the actual MAC from the /lib/firmware nvram file gets used, leading to MAC conflicts. The MAC addresses in the troublesome nvram files seem to all come from the same nvram file template, so we can detect this by checking for the template nvram file MAC. Detect that the default MAC address is being used and replace it with a random MAC address to avoid MAC address conflicts. Note that udev will detect this is a random MAC based on /sys/class/net/wlan0/addr_assign_type and then replace this with a MAC based on hashing the netdev-name + the machine-id. So that the MAC address is both guaranteed to be unique per machine while it is still the same/persistent at each boot (assuming the default Link.MACAddressPolicy=persistent udev setting). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../broadcom/brcm80211/brcmfmac/common.c | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+)