[PATCH 1/2] brcmfmac: Add brcmf_c_set_cur_etheraddr() helper

[PATCH 1/2] brcmfmac: Add brcmf_c_set_cur_etheraddr() helper

[Hans de Goede]

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(-)

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);
-- 
2.36.0

[PATCH 2/2] brcmfmac: Replace default (not configured) MAC with a random MAC

[Hans de Goede]

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(+)

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));
-- 
2.36.0

Re: [PATCH 2/2] brcmfmac: Replace default (not configured) MAC with a random MAC

[Arend Van Spriel]

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

Re: [PATCH 1/2] brcmfmac: Add brcmf_c_set_cur_etheraddr() helper

[Arend Van Spriel]

On 7/8/2022 3:37 PM, Hans de Goede 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.

Acked-by: Arend van Spriel <aspriel@gmail.com>
> 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(-)

Re: [PATCH 2/2] brcmfmac: Replace default (not configured) MAC with a random MAC

[Hans de Goede]

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

Re: [PATCH 2/2] brcmfmac: Replace default (not configured) MAC with a random MAC

[Kalle Valo]

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 :)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [1/2] wifi: brcmfmac: Add brcmf_c_set_cur_etheraddr() helper

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220708133712.102179-1-hdegoede@redhat.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH 2/2] brcmfmac: Replace default (not configured) MAC with a random MAC

[Hans de Goede]

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(+)

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));
-- 
2.36.0