| Message ID | 1466273932-11554-1-git-send-email-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
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. > + 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. Regards, Arend -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On 18 June 2016 at 23:58, Rafał Miłecki <zajec5@gmail.com> wrote: > 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? Maybe I have even better idea. What about handling Linux interface removal in the code waiting for BRCMF_E_IF_DEL? We already do something like that in case of BRCMF_E_IF_ADD (it is brcmf_ap_add_vif that calls brcmf_net_attach). -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + if (ifp && ifevent->action == BRCMF_E_IF_DEL) { + bool rtnl_locked = brcmf_cfg80211_vif_event_armed(drvr->config); + + brcmf_remove_interface(ifp, rtnl_locked); + } - if (ifp && ifevent->action == BRCMF_E_IF_DEL) - brcmf_remove_interface(ifp, false); + err = brcmf_fweh_call_event_handler(ifp, emsg->event_code, emsg, data); + if (err) + brcmf_err("event %d handler failed (%d)\n", emsg->event_code, + err); } /**
So far when receiving event about in-firmware-interface removal we were notifying our listener and afterwards we were removing Linux interface. This order was most likely a try to avoid a lockup. Removing in-firmware interface could be requested by a driver code holding rtnl lock. Such code waits for a corresponding event (still holding rtnl lock) which prevents us from calling unregister_netdev. Notifying listener first allowed it to release rtnl lock and removing interface succesfully. Please note we couldn't switch to unregister_netdevice as interface removal event could also occur in other (unpredictable) moments. Unfortunately above workaround doesn't work in some corner cases. Focus on the time slot between calling event handler and removing Linux interface. During that time original caller may leave (unlocking rtnl semaphore) *and* another call to the same code may be done (locking it again). If that happens our event handler will stuck at removing Linux interface, it won't handle another event and will block process holding rtnl lock. So the real solution is to remove interface before notifying listener. We just need to know if rtnl is hold by a caller that triggered our event. Moreover this changes makes sure we call unregister_netdevice before del_virtual_intf leaves which isn't critical but it makes a bit more of sense to handle it this way. Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)