lkml.org 
[lkml]   [2016]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH RFC 1/2] brcmfmac: remove interface before notifying listener
On 18 June 2016 at 21:26, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> On 18-06-16 20:18, Rafał Miłecki wrote:
>> So far when receiving event about in-firmware-interface removal we were
>> notifying our listener and afterwards we were removing Linux interface.
>>
>
> [snip]
>
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>> index 9da7a4c..5fd1886 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
>> @@ -18,6 +18,7 @@
>> #include "brcmu_wifi.h"
>> #include "brcmu_utils.h"
>>
>> +#include "cfg80211.h"
>> #include "core.h"
>> #include "debug.h"
>> #include "tracepoint.h"
>> @@ -180,10 +181,16 @@ static void brcmf_fweh_handle_if_event(struct brcmf_pub *drvr,
>> if (ifp && ifevent->action == BRCMF_E_IF_CHANGE)
>> brcmf_fws_reset_interface(ifp);
>>
>> - err = brcmf_fweh_call_event_handler(ifp, emsg->event_code, emsg, data);
>
> The reason for doing this first is because we are passing the ifp, which
> is netdev_priv(ifp->ndev). In brcmf_remove_interface() we only
> unregister the netdev, which will end up (after scheduling) in
> brcmf_free_netdev() thus freeing the ifp. By moving the event handler
> function ifp may be stale already.

Good catch. What about making brcmf_fweh_call_event_handler work
without ifp? Would that be OK then?


>> + if (ifp && ifevent->action == BRCMF_E_IF_DEL) {
>> + bool rtnl_locked = brcmf_cfg80211_vif_event_armed(drvr->config);
>> +
>> + brcmf_remove_interface(ifp, rtnl_locked);
>
> I guess rtnl_locked here means "rtnl_is_locked() by brcmfmac". It
> actually does not matter who is holding the rtnl_lock. At least when it
> is brcmfmac it is still a different task, ie. hostapd, iw, etc. Also
> when brcmf_cfg80211_vif_event_armed() return false there may still be
> some task holding the rtnl_lock.

It does matter who holds the lock.

If it's e.g. some other driver (ath, intel, ralink, whatever) we still
should call unregister_netdevice. It'll just wait until rtnl lock gets
released.

If it's brcmfmac holding the lock, we can't expect it to be released
as brcmfmac waits for completion event.

--
Rafał

\
 
 \ /
  Last update: 2016-06-19 00:41    [W:0.058 / U:1.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site