Re: brcmfmac MAC address change delay and 500ms down delay
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.vanspr...@broadcom.com
Re: brcmfmac MAC address change delay and 500ms down delay
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
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
brcmfmac MAC address change delay and 500ms down delay
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