[PATCH 1/2] brcmfmac: delete interface directly in code that sent fw request
So far when receiving event about in-firmware-interface removal our
event worker was notifying listener and afterwards it was removing Linux
interface.
First of all it was resulting in slightly unexpected order. The listener
(del_virtual_intf callback) was (usually) returning with success before
we even called unregister_netdev(ice).
Please note this couldn't be simply fixed by changing order of calls in
brcmf_fweh_handle_if_event as unregistering interface earlier could free
struct brcmf_if.
Another problem of current implementation are possible lockups. 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.
This can be simply solved by unregistering interface in a proper
callback, right after receiving confirmation event from firmware. This
only required modifying worker to don't unregister on its own if there
is someone waiting for the event.
Signed-off-by: Rafał Miłecki
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 10 --
drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 9da7a4c..79c081f 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"
@@ -182,8 +183,13 @@ static void brcmf_fweh_handle_if_event(struct brcmf_pub
*drvr,
err = brcmf_fweh_call_event_handler(ifp, emsg->event_code, emsg, data);
- if (ifp && ifevent->action == BRCMF_E_IF_DEL)
- brcmf_remove_interface(ifp, false);
+ if (ifp && ifevent->action == BRCMF_E_IF_DEL) {
+ bool armed = brcmf_cfg80211_vif_event_armed(drvr->config);
+
+ /* Default handling in case no-one waits for this event */
+ if (!armed)
+ brcmf_remove_interface(ifp, false);
+ }
}
/**
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index f6241fd..66f942f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -2288,8 +2288,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct
wireless_dev *wdev)
else
err = 0;
}
- if (err)
- brcmf_remove_interface(vif->ifp, true);
+ brcmf_remove_interface(vif->ifp, true);
brcmf_cfg80211_arm_vif_event(cfg, NULL);
if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE)
--
1.8.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [1/2] brcmfmac: delete interface directly in code that sent fw request
Rafał Miłecki wrote: > So far when receiving event about in-firmware-interface removal our > event worker was notifying listener and afterwards it was removing Linux > interface. > > First of all it was resulting in slightly unexpected order. The listener > (del_virtual_intf callback) was (usually) returning with success before > we even called unregister_netdev(ice). > > Please note this couldn't be simply fixed by changing order of calls in > brcmf_fweh_handle_if_event as unregistering interface earlier could free > struct brcmf_if. > > Another problem of current implementation are possible lockups. 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. > > This can be simply solved by unregistering interface in a proper > callback, right after receiving confirmation event from firmware. This > only required modifying worker to don't unregister on its own if there > is someone waiting for the event. > > Signed-off-by: Rafał Miłecki Thanks, 2 patches applied to wireless-drivers-next.git: a63b09872c1d brcmfmac: delete interface directly in code that sent fw request dba8fbc67ecd brcmfmac: support removing AP interfaces with "interface_remove" -- Sent by pwcli https://patchwork.kernel.org/patch/9206157/ -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2 RFC 1/2] brcmfmac: delete interface directly in code that sent fw request
So far when receiving event about in-firmware-interface removal our
event worker was notifying listener and afterwards it was removing Linux
interface.
First of all it was resulting in slightly unexpected order. The listener
(del_virtual_intf callback) was (usually) returning with success before
we even called unregister_netdev(ice).
Please note this couldn't be simply fixed by changing order of calls in
brcmf_fweh_handle_if_event as unregistering interface earlier could free
struct brcmf_if.
Another problem of current implementation are possible lockups. 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.
This can be simply solved by unregistering interface in a proper
callback, right after receiving confirmation event from firmware. This
only required modifying worker to don't unregister on its own if there
is someone waiting for the event.
Signed-off-by: Rafał Miłecki
---
V2: Modification of brcmf_fweh_handle_if_event done in V1 was a wrong
idea as it could result in use-after-free regarding struct brcmf_if.
Thanks Arend for noticing that!
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 10 --
drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 9da7a4c..79c081f 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"
@@ -182,8 +183,13 @@ static void brcmf_fweh_handle_if_event(struct brcmf_pub
*drvr,
err = brcmf_fweh_call_event_handler(ifp, emsg->event_code, emsg, data);
- if (ifp && ifevent->action == BRCMF_E_IF_DEL)
- brcmf_remove_interface(ifp, false);
+ if (ifp && ifevent->action == BRCMF_E_IF_DEL) {
+ bool armed = brcmf_cfg80211_vif_event_armed(drvr->config);
+
+ /* Default handling in case no-one waits for this event */
+ if (!armed)
+ brcmf_remove_interface(ifp, false);
+ }
}
/**
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index f6241fd..66f942f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -2288,8 +2288,7 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct
wireless_dev *wdev)
else
err = 0;
}
- if (err)
- brcmf_remove_interface(vif->ifp, true);
+ brcmf_remove_interface(vif->ifp, true);
brcmf_cfg80211_arm_vif_event(cfg, NULL);
if (vif->wdev.iftype != NL80211_IFTYPE_P2P_DEVICE)
--
1.8.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
