diff mbox series

[02/12] wifi: mwifiex: fix MAC address handling

Message ID 20240826-mwifiex-cleanup-1-v1-2-56e6f8e056ec@pengutronix.de
State New
Headers show
Series mwifiex: two fixes and cleanup | expand

Commit Message

Sascha Hauer Aug. 26, 2024, 11:01 a.m. UTC
The mwifiex driver tries to derive the MAC addresses of the virtual
interfaces from the permanent address by adding the bss_num of the
particular interface used. It does so each time the virtual interface
is changed from AP to station or the other way round. This means that
the devices MAC address changes during a change_virtual_intf call which
is pretty unexpected by userspace.

Furthermore the driver doesn't use the permanent address to add the
bss_num to, but instead the current MAC address increases each time
we do a change_virtual_intf.

Fix this by initializing the MAC address once from the permanent MAC
address during creation of the virtual interface and never touch it
again. This also means that userspace can set a different MAC address
which then stays like this forever and is not unexpectedly changed
by the driver.

It is not clear how many (if any) MAC addresses after the permanent MAC
address are reserved for a device, so set the locally admistered
bit for all MAC addresses modified from the permanent address.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: stable@vger.kernel.org
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c |  4 +-
 drivers/net/wireless/marvell/mwifiex/init.c     |  1 -
 drivers/net/wireless/marvell/mwifiex/main.c     | 54 ++++++++++++-------------
 drivers/net/wireless/marvell/mwifiex/main.h     |  5 ++-
 4 files changed, 30 insertions(+), 34 deletions(-)

Comments

Francesco Dolcini Sept. 6, 2024, 2:40 p.m. UTC | #1
On Mon, Aug 26, 2024 at 01:01:23PM +0200, Sascha Hauer wrote:
> The mwifiex driver tries to derive the MAC addresses of the virtual
> interfaces from the permanent address by adding the bss_num of the
> particular interface used. It does so each time the virtual interface
> is changed from AP to station or the other way round. This means that
> the devices MAC address changes during a change_virtual_intf call which
> is pretty unexpected by userspace.

Is this the only reason for this patch or there are other reasons?
I'd like to understand the whole impact, to be sure the backport to
stable is what we want.

> Furthermore the driver doesn't use the permanent address to add the
> bss_num to, but instead the current MAC address increases each time
> we do a change_virtual_intf.
> 
> Fix this by initializing the MAC address once from the permanent MAC
> address during creation of the virtual interface and never touch it
> again. This also means that userspace can set a different MAC address
> which then stays like this forever and is not unexpectedly changed
> by the driver.
> 
> It is not clear how many (if any) MAC addresses after the permanent MAC
> address are reserved for a device, so set the locally admistered
> bit for all MAC addresses modified from the permanent address.

I wonder if we should not just use the same permanent mac address whatever
the virtual interface is. Do we have something similar in other wireless
drivers?

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c |  4 +-
>  drivers/net/wireless/marvell/mwifiex/init.c     |  1 -
>  drivers/net/wireless/marvell/mwifiex/main.c     | 54 ++++++++++++-------------
>  drivers/net/wireless/marvell/mwifiex/main.h     |  5 ++-
>  4 files changed, 30 insertions(+), 34 deletions(-)
> 
...

> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 96d1f6039fbca..46acddd03ffd1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -971,34 +971,16 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  }
>  
>  int mwifiex_set_mac_address(struct mwifiex_private *priv,
> -			    struct net_device *dev, bool external,
> -			    u8 *new_mac)
> +			    struct net_device *dev, u8 *new_mac)
>  {
>  	int ret;
> -	u64 mac_addr, old_mac_addr;
> +	u64 old_mac_addr;
>  
> -	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
> +	netdev_info(dev, "%s: old: %pM new: %pM\n", __func__, priv->curr_addr, new_mac);
>  
> -	if (external) {
> -		mac_addr = ether_addr_to_u64(new_mac);
> -	} else {
> -		/* Internal mac address change */
> -		if (priv->bss_type == MWIFIEX_BSS_TYPE_ANY)
> -			return -EOPNOTSUPP;
this was the only usage of MWIFIEX_BSS_TYPE_ANY, correct? Did it had any
reason before?

> -
> -		mac_addr = old_mac_addr;
> -
> -		if (priv->bss_type == MWIFIEX_BSS_TYPE_P2P) {
> -			mac_addr |= BIT_ULL(MWIFIEX_MAC_LOCAL_ADMIN_BIT);
> -			mac_addr += priv->bss_num;
> -		} else if (priv->adapter->priv[0] != priv) {
> -			/* Set mac address based on bss_type/bss_num */
> -			mac_addr ^= BIT_ULL(priv->bss_type + 8);
> -			mac_addr += priv->bss_num;
> -		}
> -	}
> +	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
>  
> -	u64_to_ether_addr(mac_addr, priv->curr_addr);
> +	ether_addr_copy(priv->curr_addr, new_mac);
>  
>  	/* Send request to firmware */
>  	ret = mwifiex_send_cmd(priv, HostCmd_CMD_802_11_MAC_ADDRESS,
> @@ -1015,6 +997,26 @@ int mwifiex_set_mac_address(struct mwifiex_private *priv,
>  	return 0;
>  }
>  
> +int mwifiex_set_default_mac_address(struct mwifiex_private *priv,
> +				    struct net_device *dev)
> +{
> +	int priv_num;
> +	u8 mac[ETH_ALEN];
> +
> +	ether_addr_copy(mac, priv->adapter->perm_addr);
> +
> +	for (priv_num = 0; priv_num < priv->adapter->priv_num; priv_num++)
> +		if (priv == priv->adapter->priv[priv_num])
> +			break;
> +
> +	if (priv_num) {
> +		eth_addr_add(mac, priv_num);
> +		mac[0] |= 0x2;
> +	}

Please see my concern on this in the beginning of the email.

> @@ -1364,10 +1366,6 @@ void mwifiex_init_priv_params(struct mwifiex_private *priv,
>  	priv->assocresp_idx = MWIFIEX_AUTO_IDX_MASK;
>  	priv->gen_idx = MWIFIEX_AUTO_IDX_MASK;
>  	priv->num_tx_timeout = 0;
> -	if (is_valid_ether_addr(dev->dev_addr))
> -		ether_addr_copy(priv->curr_addr, dev->dev_addr);
> -	else
> -		ether_addr_copy(priv->curr_addr, priv->adapter->perm_addr);

With this change, when mfg_mode is true, priv->curr_addr will be not
initialized. Wanted? 

Francesco
Sascha Hauer Sept. 9, 2024, 8:09 a.m. UTC | #2
On Fri, Sep 06, 2024 at 04:40:36PM +0200, Francesco Dolcini wrote:
> On Mon, Aug 26, 2024 at 01:01:23PM +0200, Sascha Hauer wrote:
> > The mwifiex driver tries to derive the MAC addresses of the virtual
> > interfaces from the permanent address by adding the bss_num of the
> > particular interface used. It does so each time the virtual interface
> > is changed from AP to station or the other way round. This means that
> > the devices MAC address changes during a change_virtual_intf call which
> > is pretty unexpected by userspace.
> 
> Is this the only reason for this patch or there are other reasons?
> I'd like to understand the whole impact, to be sure the backport to
> stable is what we want.
> 
> > Furthermore the driver doesn't use the permanent address to add the
> > bss_num to, but instead the current MAC address increases each time
> > we do a change_virtual_intf.
> > 
> > Fix this by initializing the MAC address once from the permanent MAC
> > address during creation of the virtual interface and never touch it
> > again. This also means that userspace can set a different MAC address
> > which then stays like this forever and is not unexpectedly changed
> > by the driver.
> > 
> > It is not clear how many (if any) MAC addresses after the permanent MAC
> > address are reserved for a device, so set the locally admistered
> > bit for all MAC addresses modified from the permanent address.
> 
> I wonder if we should not just use the same permanent mac address whatever
> the virtual interface is. Do we have something similar in other wireless
> drivers?

Yes, there are at least four driver that generate different MAC
addresses for different vifs:

drivers/net/wireless/ath/ath6kl/cfg80211.c:3816
drivers/net/wireless/ath/wil6210/cfg80211.c:732
drivers/net/wireless/microchip/wilc1000/netdev.c:983
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:606

(line numbers match 6.11-rc6):

For the mac80211 based drivers there are also tricks played to use
unique MAC addresses in ieee80211_assign_perm_addr().

For reference in mwifiex setting different MAC addresses for different
interfaces goes down to:

| commit 864164683678e27c931b5909c72a001b1b943f36
| Author: Xinming Hu <huxm@marvell.com>
| Date:   Tue Feb 13 14:10:15 2018 +0800
| 
|     mwifiex: set different mac address for interfaces with same bss type
| 
|     Multiple interfaces with same bss type could affect each other if
|     they are sharing the same mac address. In this patch, different
|     mac address is assigned to new interface which have same bss type
|     with exist interfaces.
| 
|     Signed-off-by: Xinming Hu <huxm@marvell.com>
|     Signed-off-by: Kalle Valo <kvalo@codeaurora.org>


> 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/net/wireless/marvell/mwifiex/cfg80211.c |  4 +-
> >  drivers/net/wireless/marvell/mwifiex/init.c     |  1 -
> >  drivers/net/wireless/marvell/mwifiex/main.c     | 54 ++++++++++++-------------
> >  drivers/net/wireless/marvell/mwifiex/main.h     |  5 ++-
> >  4 files changed, 30 insertions(+), 34 deletions(-)
> > 
> ...
> 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 96d1f6039fbca..46acddd03ffd1 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -971,34 +971,16 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  }
> >  
> >  int mwifiex_set_mac_address(struct mwifiex_private *priv,
> > -			    struct net_device *dev, bool external,
> > -			    u8 *new_mac)
> > +			    struct net_device *dev, u8 *new_mac)
> >  {
> >  	int ret;
> > -	u64 mac_addr, old_mac_addr;
> > +	u64 old_mac_addr;
> >  
> > -	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
> > +	netdev_info(dev, "%s: old: %pM new: %pM\n", __func__, priv->curr_addr, new_mac);
> >  
> > -	if (external) {
> > -		mac_addr = ether_addr_to_u64(new_mac);
> > -	} else {
> > -		/* Internal mac address change */
> > -		if (priv->bss_type == MWIFIEX_BSS_TYPE_ANY)
> > -			return -EOPNOTSUPP;
> this was the only usage of MWIFIEX_BSS_TYPE_ANY, correct? Did it had any
> reason before?

I haven't found a path to get here with priv->bss_type ==
MWIFIEX_BSS_TYPE_ANY. This function is called from
mwifiex_init_new_priv_params() and mwifiex_add_virtual_intf(). Both
functions set priv->bss_type to something else or bail out with an error
before calling mwifiex_set_mac_address(). It's also called from the
ndo_set_mac_address method, but for a registered net device the bss_type
should also be set to something meaningful.

> 
> > -
> > -		mac_addr = old_mac_addr;
> > -
> > -		if (priv->bss_type == MWIFIEX_BSS_TYPE_P2P) {
> > -			mac_addr |= BIT_ULL(MWIFIEX_MAC_LOCAL_ADMIN_BIT);
> > -			mac_addr += priv->bss_num;
> > -		} else if (priv->adapter->priv[0] != priv) {
> > -			/* Set mac address based on bss_type/bss_num */
> > -			mac_addr ^= BIT_ULL(priv->bss_type + 8);
> > -			mac_addr += priv->bss_num;
> > -		}
> > -	}
> > +	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
> >  
> > -	u64_to_ether_addr(mac_addr, priv->curr_addr);
> > +	ether_addr_copy(priv->curr_addr, new_mac);
> >  
> >  	/* Send request to firmware */
> >  	ret = mwifiex_send_cmd(priv, HostCmd_CMD_802_11_MAC_ADDRESS,
> > @@ -1015,6 +997,26 @@ int mwifiex_set_mac_address(struct mwifiex_private *priv,
> >  	return 0;
> >  }
> >  
> > +int mwifiex_set_default_mac_address(struct mwifiex_private *priv,
> > +				    struct net_device *dev)
> > +{
> > +	int priv_num;
> > +	u8 mac[ETH_ALEN];
> > +
> > +	ether_addr_copy(mac, priv->adapter->perm_addr);
> > +
> > +	for (priv_num = 0; priv_num < priv->adapter->priv_num; priv_num++)
> > +		if (priv == priv->adapter->priv[priv_num])
> > +			break;
> > +
> > +	if (priv_num) {
> > +		eth_addr_add(mac, priv_num);
> > +		mac[0] |= 0x2;
> > +	}
> 
> Please see my concern on this in the beginning of the email.
> 
> > @@ -1364,10 +1366,6 @@ void mwifiex_init_priv_params(struct mwifiex_private *priv,
> >  	priv->assocresp_idx = MWIFIEX_AUTO_IDX_MASK;
> >  	priv->gen_idx = MWIFIEX_AUTO_IDX_MASK;
> >  	priv->num_tx_timeout = 0;
> > -	if (is_valid_ether_addr(dev->dev_addr))
> > -		ether_addr_copy(priv->curr_addr, dev->dev_addr);
> > -	else
> > -		ether_addr_copy(priv->curr_addr, priv->adapter->perm_addr);
> 
> With this change, when mfg_mode is true, priv->curr_addr will be not
> initialized. Wanted?

Not wanted, just me being ignorant. Let's have a look:

priv->adapter->perm_addr is initialized in the response handling of the
HostCmd_CMD_GET_HW_SPEC command. This command is only issued when
mfg_mode is false, so in mfg mode priv->adapter->perm_addr will be the
zero address.

The only documentation we have for mfg_mode is:

manufacturing mode enable:1, disable:0

I don't know what this really is about, but I could imagine that this
is for initial factory bringup when the chip is not parametrized and thus
doesn't have a permanent MAC address yet.

Sascha
Francesco Dolcini Sept. 9, 2024, 4:42 p.m. UTC | #3
On Mon, Sep 09, 2024 at 10:09:33AM +0200, Sascha Hauer wrote:
> On Fri, Sep 06, 2024 at 04:40:36PM +0200, Francesco Dolcini wrote:
> > On Mon, Aug 26, 2024 at 01:01:23PM +0200, Sascha Hauer wrote:
> > > The mwifiex driver tries to derive the MAC addresses of the virtual
> > > interfaces from the permanent address by adding the bss_num of the
> > > particular interface used. It does so each time the virtual interface
> > > is changed from AP to station or the other way round. This means that
> > > the devices MAC address changes during a change_virtual_intf call which
> > > is pretty unexpected by userspace.
> > 
> > Is this the only reason for this patch or there are other reasons?
> > I'd like to understand the whole impact, to be sure the backport to
> > stable is what we want.
> > 
> > > Furthermore the driver doesn't use the permanent address to add the
> > > bss_num to, but instead the current MAC address increases each time
> > > we do a change_virtual_intf.
> > > 
> > > Fix this by initializing the MAC address once from the permanent MAC
> > > address during creation of the virtual interface and never touch it
> > > again. This also means that userspace can set a different MAC address
> > > which then stays like this forever and is not unexpectedly changed
> > > by the driver.
> > > 
> > > It is not clear how many (if any) MAC addresses after the permanent MAC
> > > address are reserved for a device, so set the locally admistered
> > > bit for all MAC addresses modified from the permanent address.
> > 
> > I wonder if we should not just use the same permanent mac address whatever
> > the virtual interface is. Do we have something similar in other wireless
> > drivers?
> 
> Yes, there are at least four driver that generate different MAC
> addresses for different vifs:

Ok, fine for me. It seems like there is some real use case requiring to have
different MAC addresses for each virtual interface, and given that mwifiex is
already like that, we should keep it that way.

It would be interesting to know from NXP if they do provide some guidance on
this topic to whoever is using their chips or the reality is what you
implemented here that we cannot assume anything on how many MAC addresses are
available is just the way it is.

David?

> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/net/wireless/marvell/mwifiex/cfg80211.c |  4 +-
> > >  drivers/net/wireless/marvell/mwifiex/init.c     |  1 -
> > >  drivers/net/wireless/marvell/mwifiex/main.c     | 54 ++++++++++++-------------
> > >  drivers/net/wireless/marvell/mwifiex/main.h     |  5 ++-
> > >  4 files changed, 30 insertions(+), 34 deletions(-)
> > > 
> > ...
> > 
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> > > index 96d1f6039fbca..46acddd03ffd1 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > > @@ -971,34 +971,16 @@ mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > >  }
> > >  
> > >  int mwifiex_set_mac_address(struct mwifiex_private *priv,
> > > -			    struct net_device *dev, bool external,
> > > -			    u8 *new_mac)
> > > +			    struct net_device *dev, u8 *new_mac)
> > >  {
> > >  	int ret;
> > > -	u64 mac_addr, old_mac_addr;
> > > +	u64 old_mac_addr;
> > >  
> > > -	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
> > > +	netdev_info(dev, "%s: old: %pM new: %pM\n", __func__, priv->curr_addr, new_mac);
> > >  
> > > -	if (external) {
> > > -		mac_addr = ether_addr_to_u64(new_mac);
> > > -	} else {
> > > -		/* Internal mac address change */
> > > -		if (priv->bss_type == MWIFIEX_BSS_TYPE_ANY)
> > > -			return -EOPNOTSUPP;
> > this was the only usage of MWIFIEX_BSS_TYPE_ANY, correct? Did it had any
> > reason before?
> 
> I haven't found a path to get here with priv->bss_type ==
> MWIFIEX_BSS_TYPE_ANY. This function is called from

Ok, so maybe we can kill the MWIFIEX_BSS_TYPE_ANY in this patch also?

> > > @@ -1364,10 +1366,6 @@ void mwifiex_init_priv_params(struct mwifiex_private *priv,
> > >  	priv->assocresp_idx = MWIFIEX_AUTO_IDX_MASK;
> > >  	priv->gen_idx = MWIFIEX_AUTO_IDX_MASK;
> > >  	priv->num_tx_timeout = 0;
> > > -	if (is_valid_ether_addr(dev->dev_addr))
> > > -		ether_addr_copy(priv->curr_addr, dev->dev_addr);
> > > -	else
> > > -		ether_addr_copy(priv->curr_addr, priv->adapter->perm_addr);
> > 
> > With this change, when mfg_mode is true, priv->curr_addr will be not
> > initialized. Wanted?
> 
> Not wanted, just me being ignorant. Let's have a look:
> 
> priv->adapter->perm_addr is initialized in the response handling of the
> HostCmd_CMD_GET_HW_SPEC command. This command is only issued when
> mfg_mode is false, so in mfg mode priv->adapter->perm_addr will be the
> zero address.
> 
> The only documentation we have for mfg_mode is:
> 
> manufacturing mode enable:1, disable:0
> 
> I don't know what this really is about, but I could imagine that this
> is for initial factory bringup when the chip is not parametrized and thus
> doesn't have a permanent MAC address yet.

Not sure even myself, but I would advise to not break it.

Francesco
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 5697a02e6b8d3..d3e1424bea390 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -962,8 +962,6 @@  mwifiex_init_new_priv_params(struct mwifiex_private *priv,
 	adapter->rx_locked = false;
 	spin_unlock_bh(&adapter->rx_proc_lock);
 
-	mwifiex_set_mac_address(priv, dev, false, NULL);
-
 	return 0;
 }
 
@@ -3115,7 +3113,7 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 	priv->netdev = dev;
 
 	if (!adapter->mfg_mode) {
-		mwifiex_set_mac_address(priv, dev, false, NULL);
+		mwifiex_set_default_mac_address(priv, dev);
 
 		ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
 				       HostCmd_ACT_GEN_SET, 0, NULL, true);
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 8b61e45cd6678..0259c9f88486b 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -71,7 +71,6 @@  int mwifiex_init_priv(struct mwifiex_private *priv)
 	u32 i;
 
 	priv->media_connected = false;
-	eth_broadcast_addr(priv->curr_addr);
 	priv->port_open = false;
 	priv->usb_port = MWIFIEX_USB_EP_DATA;
 	priv->pkt_tx_ctrl = 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 96d1f6039fbca..46acddd03ffd1 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -971,34 +971,16 @@  mwifiex_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 }
 
 int mwifiex_set_mac_address(struct mwifiex_private *priv,
-			    struct net_device *dev, bool external,
-			    u8 *new_mac)
+			    struct net_device *dev, u8 *new_mac)
 {
 	int ret;
-	u64 mac_addr, old_mac_addr;
+	u64 old_mac_addr;
 
-	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
+	netdev_info(dev, "%s: old: %pM new: %pM\n", __func__, priv->curr_addr, new_mac);
 
-	if (external) {
-		mac_addr = ether_addr_to_u64(new_mac);
-	} else {
-		/* Internal mac address change */
-		if (priv->bss_type == MWIFIEX_BSS_TYPE_ANY)
-			return -EOPNOTSUPP;
-
-		mac_addr = old_mac_addr;
-
-		if (priv->bss_type == MWIFIEX_BSS_TYPE_P2P) {
-			mac_addr |= BIT_ULL(MWIFIEX_MAC_LOCAL_ADMIN_BIT);
-			mac_addr += priv->bss_num;
-		} else if (priv->adapter->priv[0] != priv) {
-			/* Set mac address based on bss_type/bss_num */
-			mac_addr ^= BIT_ULL(priv->bss_type + 8);
-			mac_addr += priv->bss_num;
-		}
-	}
+	old_mac_addr = ether_addr_to_u64(priv->curr_addr);
 
-	u64_to_ether_addr(mac_addr, priv->curr_addr);
+	ether_addr_copy(priv->curr_addr, new_mac);
 
 	/* Send request to firmware */
 	ret = mwifiex_send_cmd(priv, HostCmd_CMD_802_11_MAC_ADDRESS,
@@ -1015,6 +997,26 @@  int mwifiex_set_mac_address(struct mwifiex_private *priv,
 	return 0;
 }
 
+int mwifiex_set_default_mac_address(struct mwifiex_private *priv,
+				    struct net_device *dev)
+{
+	int priv_num;
+	u8 mac[ETH_ALEN];
+
+	ether_addr_copy(mac, priv->adapter->perm_addr);
+
+	for (priv_num = 0; priv_num < priv->adapter->priv_num; priv_num++)
+		if (priv == priv->adapter->priv[priv_num])
+			break;
+
+	if (priv_num) {
+		eth_addr_add(mac, priv_num);
+		mac[0] |= 0x2;
+	}
+
+	return mwifiex_set_mac_address(priv, dev, mac);
+}
+
 /* CFG802.11 network device handler for setting MAC address.
  */
 static int
@@ -1023,7 +1025,7 @@  mwifiex_ndo_set_mac_address(struct net_device *dev, void *addr)
 	struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
 	struct sockaddr *hw_addr = addr;
 
-	return mwifiex_set_mac_address(priv, dev, true, hw_addr->sa_data);
+	return mwifiex_set_mac_address(priv, dev, hw_addr->sa_data);
 }
 
 /*
@@ -1364,10 +1366,6 @@  void mwifiex_init_priv_params(struct mwifiex_private *priv,
 	priv->assocresp_idx = MWIFIEX_AUTO_IDX_MASK;
 	priv->gen_idx = MWIFIEX_AUTO_IDX_MASK;
 	priv->num_tx_timeout = 0;
-	if (is_valid_ether_addr(dev->dev_addr))
-		ether_addr_copy(priv->curr_addr, dev->dev_addr);
-	else
-		ether_addr_copy(priv->curr_addr, priv->adapter->perm_addr);
 
 	if (GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_STA ||
 	    GET_BSS_ROLE(priv) == MWIFIEX_BSS_ROLE_UAP) {
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 529863edd7a25..dc07eb11f7752 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1694,8 +1694,9 @@  void mwifiex_process_multi_chan_event(struct mwifiex_private *priv,
 				      struct sk_buff *event_skb);
 void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter);
 int mwifiex_set_mac_address(struct mwifiex_private *priv,
-			    struct net_device *dev,
-			    bool external, u8 *new_mac);
+			    struct net_device *dev, u8 *new_mac);
+int mwifiex_set_default_mac_address(struct mwifiex_private *priv,
+				    struct net_device *dev);
 void mwifiex_devdump_tmo_func(unsigned long function_context);
 
 #ifdef CONFIG_DEBUG_FS