| Message ID | 20220708133712.102179-1-hdegoede@redhat.com |
|---|---|
| State | New |
| 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 :)
Hans de Goede <hdegoede@redhat.com> wrote: > Add a little helper to send "cur_etheraddr" commands to the interface > and to handle the error reporting of it in a single place. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Acked-by: Arend van Spriel <aspriel@gmail.com> 2 patches applied to wireless-next.git, thanks. cf1239e5b7bf wifi: brcmfmac: Add brcmf_c_set_cur_etheraddr() helper 4af4c0b93c15 wifi: brcmfmac: Replace default (not configured) MAC with a random MAC
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c index fe01da9e620d..dccd8f4ca1d0 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -190,6 +190,17 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp) return err; } +int brcmf_c_set_cur_etheraddr(struct brcmf_if *ifp, const u8 *addr) +{ + s32 err; + + err = brcmf_fil_iovar_data_set(ifp, "cur_etheraddr", addr, ETH_ALEN); + if (err < 0) + bphy_err(ifp->drvr, "Setting cur_etheraddr failed, %d\n", err); + + return err; +} + int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) { struct brcmf_pub *drvr = ifp->drvr; @@ -204,12 +215,9 @@ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp) if (is_valid_ether_addr(ifp->mac_addr)) { /* set mac address */ - err = brcmf_fil_iovar_data_set(ifp, "cur_etheraddr", ifp->mac_addr, - ETH_ALEN); - if (err < 0) { - bphy_err(ifp->drvr, "Setting cur_etheraddr failed, %d\n", err); + err = brcmf_c_set_cur_etheraddr(ifp, ifp->mac_addr); + if (err < 0) goto done; - } } else { /* retrieve mac address */ err = brcmf_fil_iovar_data_get(ifp, "cur_etheraddr", ifp->mac_addr, diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h index 15accc88d5c0..7329eb751945 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h @@ -65,6 +65,7 @@ void brcmf_release_module_param(struct brcmf_mp_device *module_param); /* Sets dongle media info (drv_version, mac address). */ int brcmf_c_preinit_dcmds(struct brcmf_if *ifp); +int brcmf_c_set_cur_etheraddr(struct brcmf_if *ifp, const u8 *addr); #ifdef CONFIG_DMI void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c index 87aef211b35f..bd164a0821f9 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c @@ -233,16 +233,12 @@ static int brcmf_netdev_set_mac_address(struct net_device *ndev, void *addr) { struct brcmf_if *ifp = netdev_priv(ndev); struct sockaddr *sa = (struct sockaddr *)addr; - struct brcmf_pub *drvr = ifp->drvr; int err; brcmf_dbg(TRACE, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx); - err = brcmf_fil_iovar_data_set(ifp, "cur_etheraddr", sa->sa_data, - ETH_ALEN); - if (err < 0) { - bphy_err(drvr, "Setting cur_etheraddr failed, %d\n", err); - } else { + err = brcmf_c_set_cur_etheraddr(ifp, sa->sa_data); + if (err >= 0) { brcmf_dbg(TRACE, "updated to %pM\n", sa->sa_data); memcpy(ifp->mac_addr, sa->sa_data, ETH_ALEN); eth_hw_addr_set(ifp->ndev, ifp->mac_addr);
Add a little helper to send "cur_etheraddr" commands to the interface and to handle the error reporting of it in a single place. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../broadcom/brcm80211/brcmfmac/common.c | 18 +++++++++++++----- .../broadcom/brcm80211/brcmfmac/common.h | 1 + .../broadcom/brcm80211/brcmfmac/core.c | 8 ++------ 3 files changed, 16 insertions(+), 11 deletions(-)