brcmfmac MAC address change delay and 500ms down delay

brcmfmac MAC address change delay and 500ms down delay

[Dan Williams]

Hi,

While refining NetworkManager's MAC address randomization behavior we
came across two issues with brcmfmac:

1) when changing the MAC address, the driver schedules work for the new
change and returns success, but doesn't actually change the MAC until
the work is scheduled.  Because it returns 0 from the
ndo_set_mac_address hook the net core will generate a NETDEV_CHANGEADDR
event and rtnetlink will send out an RTM_NEWLINK with the old MAC
address.  No event for the new address will be sent.  So it's pretty
hard to figure out when the address actually changed, and when its safe
to associate, without polling the device's MAC address.  Ugly.

2) when closing the device (eg, set !IFF_UP) the driver unconditionally
blocks for 500ms in __brcmf_cfg80211_down():

	if (check_vif_up(ifp->vif)) {
		brcmf_link_down(ifp->vif, WLAN_REASON_UNSPECIFIED);

		/* Make sure WPA_Supplicant receives all the event
		   generated due to DISASSOC call to the fw to keep
		   the state fw and WPA_Supplicant state consistent
		 */
		brcmf_delay(500);
	}

Should I dump these into kernel bugzilla, or is there some internal bug
tracker they could get stuffed into?

Thanks!
Dan

Re: brcmfmac MAC address change delay and 500ms down delay

[Arend Van Spriel]

On 15-9-2016 16:42, Dan Williams wrote:
> Hi,
> 
> While refining NetworkManager's MAC address randomization behavior we
> came across two issues with brcmfmac:
> 
> 1) when changing the MAC address, the driver schedules work for the new
> change and returns success, but doesn't actually change the MAC until
> the work is scheduled.  Because it returns 0 from the
> ndo_set_mac_address hook the net core will generate a NETDEV_CHANGEADDR
> event and rtnetlink will send out an RTM_NEWLINK with the old MAC
> address.  No event for the new address will be sent.  So it's pretty
> hard to figure out when the address actually changed, and when its safe
> to associate, without polling the device's MAC address.  Ugly.

And apparently unnecessary. I recalled we had this as the
ndo_set_mac_address callback could be called in atomic context. So we
are using a worker because we are grabbing a mutex upon sending the
control info to the device. Looking into the core network code it seems
the callback is not called in atomic context so it seems we can get rid
of the worker here. I made a patch

> 2) when closing the device (eg, set !IFF_UP) the driver unconditionally
> blocks for 500ms in __brcmf_cfg80211_down():
> 
> 	if (check_vif_up(ifp->vif)) {
> 		brcmf_link_down(ifp->vif, WLAN_REASON_UNSPECIFIED);
> 
> 		/* Make sure WPA_Supplicant receives all the event
> 		   generated due to DISASSOC call to the fw to keep
> 		   the state fw and WPA_Supplicant state consistent
> 		 */
> 		brcmf_delay(500);
> 	}

This is actually a bogus delay as we are under an RTNL lock here so I
think the events will not go out until after the delay has finished. I
did submit a patch long ago removing this delay, but the change was not
accepted. Let me revisit that.

> Should I dump these into kernel bugzilla, or is there some internal bug
> tracker they could get stuffed into?

Kernel bugzilla is fine although I check it rather infrequently.

Regards,
Arend

Re: brcmfmac MAC address change delay and 500ms down delay

[Dan Williams]

On Fri, 2016-09-16 at 11:58 +0200, Arend Van Spriel wrote:
> On 15-9-2016 16:42, Dan Williams wrote:
> > 
> > Hi,
> > 
> > While refining NetworkManager's MAC address randomization behavior
> > we
> > came across two issues with brcmfmac:
> > 
> > 1) when changing the MAC address, the driver schedules work for the
> > new
> > change and returns success, but doesn't actually change the MAC
> > until
> > the work is scheduled.  Because it returns 0 from the
> > ndo_set_mac_address hook the net core will generate a
> > NETDEV_CHANGEADDR
> > event and rtnetlink will send out an RTM_NEWLINK with the old MAC
> > address.  No event for the new address will be sent.  So it's
> > pretty
> > hard to figure out when the address actually changed, and when its
> > safe
> > to associate, without polling the device's MAC address.  Ugly.
> And apparently unnecessary. I recalled we had this as the
> ndo_set_mac_address callback could be called in atomic context. So we
> are using a worker because we are grabbing a mutex upon sending the
> control info to the device. Looking into the core network code it
> seems
> the callback is not called in atomic context so it seems we can get
> rid
> of the worker here. I made a patch
> 
> > 
> > 2) when closing the device (eg, set !IFF_UP) the driver
> > unconditionally
> > blocks for 500ms in __brcmf_cfg80211_down():
> > 
> > 	if (check_vif_up(ifp->vif)) {
> > 		brcmf_link_down(ifp->vif, WLAN_REASON_UNSPECIFIED);
> > 
> > 		/* Make sure WPA_Supplicant receives all the event
> > 		   generated due to DISASSOC call to the fw to keep
> > 		   the state fw and WPA_Supplicant state consistent
> > 		 */
> > 		brcmf_delay(500);
> > 	}
> This is actually a bogus delay as we are under an RTNL lock here so I
> think the events will not go out until after the delay has finished.
> I
> did submit a patch long ago removing this delay, but the change was
> not
> accepted. Let me revisit that.
> 
> > 
> > Should I dump these into kernel bugzilla, or is there some internal
> > bug
> > tracker they could get stuffed into?
> Kernel bugzilla is fine although I check it rather infrequently.

Thanks for taking another look at these.  Should I still file in
bugzilla, or are the patches going through the process already?

Dan

Re: brcmfmac MAC address change delay and 500ms down delay

[Arend Van Spriel]

On 19-9-2016 16:48, Dan Williams wrote:
> On Fri, 2016-09-16 at 11:58 +0200, Arend Van Spriel wrote:
>> On 15-9-2016 16:42, Dan Williams wrote:
>>>
>>> Hi,
>>>
>>> While refining NetworkManager's MAC address randomization behavior
>>> we
>>> came across two issues with brcmfmac:
>>>
>>> 1) when changing the MAC address, the driver schedules work for the
>>> new
>>> change and returns success, but doesn't actually change the MAC
>>> until
>>> the work is scheduled.  Because it returns 0 from the
>>> ndo_set_mac_address hook the net core will generate a
>>> NETDEV_CHANGEADDR
>>> event and rtnetlink will send out an RTM_NEWLINK with the old MAC
>>> address.  No event for the new address will be sent.  So it's
>>> pretty
>>> hard to figure out when the address actually changed, and when its
>>> safe
>>> to associate, without polling the device's MAC address.  Ugly.
>> And apparently unnecessary. I recalled we had this as the
>> ndo_set_mac_address callback could be called in atomic context. So we
>> are using a worker because we are grabbing a mutex upon sending the
>> control info to the device. Looking into the core network code it
>> seems
>> the callback is not called in atomic context so it seems we can get
>> rid
>> of the worker here. I made a patch
>>
>>>
>>> 2) when closing the device (eg, set !IFF_UP) the driver
>>> unconditionally
>>> blocks for 500ms in __brcmf_cfg80211_down():
>>>
>>> 	if (check_vif_up(ifp->vif)) {
>>> 		brcmf_link_down(ifp->vif, WLAN_REASON_UNSPECIFIED);
>>>
>>> 		/* Make sure WPA_Supplicant receives all the event
>>> 		   generated due to DISASSOC call to the fw to keep
>>> 		   the state fw and WPA_Supplicant state consistent
>>> 		 */
>>> 		brcmf_delay(500);
>>> 	}
>> This is actually a bogus delay as we are under an RTNL lock here so I
>> think the events will not go out until after the delay has finished.
>> I
>> did submit a patch long ago removing this delay, but the change was
>> not
>> accepted. Let me revisit that.
>>
>>>
>>> Should I dump these into kernel bugzilla, or is there some internal
>>> bug
>>> tracker they could get stuffed into?
>> Kernel bugzilla is fine although I check it rather infrequently.
> 
> Thanks for taking another look at these.  Should I still file in
> bugzilla, or are the patches going through the process already?

For the mac address delay I let this [1] one fly today. A pity
git-send-email does not add the 'Reported-by:' email to the Cc:

The other one is a bit more tricky. The 500ms delay can be removed, but
I need to fix a related scenario. Working on it.

Regards,
Arend

[1] 1474283399-14385-6-git-send-email-arend.vanspriel@broadcom.com