[PATCH RFC 1/2] brcmfmac: remove interface before notifying listener

[PATCH RFC 1/2] brcmfmac: remove interface before notifying listener

[Rafał Miłecki]

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(-)

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);
 }
 
 /**
-- 
1.8.4.5

[PATCH RFC 2/2] brcmfmac: support removing AP interfaces with "interface_remove"

[Rafał Miłecki]

New firmwares (e.g. 10.10.69.36 for BCM4366) support "interface_remove"
for removing interfaces. Try to use this method on cfg80211 request. In
case of older firmwares (e.g. 7.35.177.56 for BCM43602 as I tested) this
will just result in firmware rejecting command and this won't change any
behavior.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 37 +++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 502b0a0..aff76c2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -785,12 +785,46 @@ s32 brcmf_notify_escan_complete(struct brcmf_cfg80211_info *cfg,
 	return err;
 }
 
+static int brcmf_cfg80211_del_ap_iface(struct wiphy *wiphy,
+				       struct wireless_dev *wdev)
+{
+	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
+	struct net_device *ndev = wdev->netdev;
+	struct brcmf_if *ifp = netdev_priv(ndev);
+	int ret;
+	int err;
+
+	brcmf_cfg80211_arm_vif_event(cfg, ifp->vif);
+
+	err = brcmf_fil_bsscfg_data_set(ifp, "interface_remove", NULL, 0);
+	if (err) {
+		brcmf_err("interface_remove failed %d\n", err);
+		goto err_unarm;
+	}
+
+	/* wait for firmware event */
+	ret = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_DEL,
+					    BRCMF_VIF_EVENT_TIMEOUT);
+	if (!ret) {
+		brcmf_err("timeout occurred\n");
+		err = -EIO;
+		goto err_unarm;
+	}
+
+err_unarm:
+	brcmf_cfg80211_arm_vif_event(cfg, NULL);
+	return err;
+}
+
 static
 int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 {
 	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
 	struct net_device *ndev = wdev->netdev;
 
+	if (ndev == cfg_to_ndev(cfg))
+		return -ENOTSUPP;
+
 	/* vif event pending in firmware */
 	if (brcmf_cfg80211_vif_event_armed(cfg))
 		return -EBUSY;
@@ -807,12 +841,13 @@ int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
 	case NL80211_IFTYPE_STATION:
-	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_AP_VLAN:
 	case NL80211_IFTYPE_WDS:
 	case NL80211_IFTYPE_MONITOR:
 	case NL80211_IFTYPE_MESH_POINT:
 		return -EOPNOTSUPP;
+	case NL80211_IFTYPE_AP:
+		return brcmf_cfg80211_del_ap_iface(wiphy, wdev);
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_P2P_GO:
 	case NL80211_IFTYPE_P2P_DEVICE:
-- 
1.8.4.5

Re: [PATCH RFC 2/2] brcmfmac: support removing AP interfaces with "interface_remove"

[Arend van Spriel]

On 18-06-16 20:18, Rafał Miłecki wrote:
> New firmwares (e.g. 10.10.69.36 for BCM4366) support "interface_remove"
> for removing interfaces. Try to use this method on cfg80211 request. In
> case of older firmwares (e.g. 7.35.177.56 for BCM43602 as I tested) this
> will just result in firmware rejecting command and this won't change any
> behavior.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 37 +++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 502b0a0..aff76c2 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -785,12 +785,46 @@ s32 brcmf_notify_escan_complete(struct brcmf_cfg80211_info *cfg,
>  	return err;
>  }
>  
> +static int brcmf_cfg80211_del_ap_iface(struct wiphy *wiphy,
> +				       struct wireless_dev *wdev)
> +{
> +	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
> +	struct net_device *ndev = wdev->netdev;
> +	struct brcmf_if *ifp = netdev_priv(ndev);
> +	int ret;
> +	int err;
> +
> +	brcmf_cfg80211_arm_vif_event(cfg, ifp->vif);
> +
> +	err = brcmf_fil_bsscfg_data_set(ifp, "interface_remove", NULL, 0);
> +	if (err) {
> +		brcmf_err("interface_remove failed %d\n", err);
> +		goto err_unarm;
> +	}
> +
> +	/* wait for firmware event */
> +	ret = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_DEL,
> +					    BRCMF_VIF_EVENT_TIMEOUT);
> +	if (!ret) {
> +		brcmf_err("timeout occurred\n");
> +		err = -EIO;
> +		goto err_unarm;
> +	}
> +
> +err_unarm:
> +	brcmf_cfg80211_arm_vif_event(cfg, NULL);
> +	return err;
> +}
> +
>  static
>  int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
>  {
>  	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
>  	struct net_device *ndev = wdev->netdev;
>  
> +	if (ndev == cfg_to_ndev(cfg))

ndev can be NULL, ie. for IFTYPE_P2P_DEVICE.

Regards,
Arend

> +		return -ENOTSUPP;
> +
>  	/* vif event pending in firmware */
>  	if (brcmf_cfg80211_vif_event_armed(cfg))
>  		return -EBUSY;
> @@ -807,12 +841,13 @@ int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
>  	switch (wdev->iftype) {
>  	case NL80211_IFTYPE_ADHOC:
>  	case NL80211_IFTYPE_STATION:
> -	case NL80211_IFTYPE_AP:
>  	case NL80211_IFTYPE_AP_VLAN:
>  	case NL80211_IFTYPE_WDS:
>  	case NL80211_IFTYPE_MONITOR:
>  	case NL80211_IFTYPE_MESH_POINT:
>  		return -EOPNOTSUPP;
> +	case NL80211_IFTYPE_AP:
> +		return brcmf_cfg80211_del_ap_iface(wiphy, wdev);
>  	case NL80211_IFTYPE_P2P_CLIENT:
>  	case NL80211_IFTYPE_P2P_GO:
>  	case NL80211_IFTYPE_P2P_DEVICE:
> 

Re: [PATCH RFC 1/2] brcmfmac: remove interface before notifying listener

[Arend van Spriel]

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

Re: [PATCH RFC 1/2] brcmfmac: remove interface before notifying listener

[Rafał Miłecki]

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ł

Re: [PATCH RFC 1/2] brcmfmac: remove interface before notifying listener

[Rafał Miłecki]

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).

[PATCH V2 RFC 1/2] brcmfmac: delete interface directly in code that sent fw request

[Rafał Miłecki]

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 <zajec5@gmail.com>
---
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

[PATCH V2 RFC 2/2] brcmfmac: support removing AP interfaces with "interface_remove"

[Rafał Miłecki]

New firmwares (e.g. 10.10.69.36 for BCM4366) support "interface_remove"
for removing interfaces. Try to use this method on cfg80211 request. In
case of older firmwares (e.g. 7.35.177.56 for BCM43602 as I tested) this
will just result in firmware rejecting command and this won't change any
behavior.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Add extra chcek for ndev. Thanks Arend.
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 39 +++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 502b0a0..1e411dc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -785,12 +785,48 @@ s32 brcmf_notify_escan_complete(struct brcmf_cfg80211_info *cfg,
 	return err;
 }
 
+static int brcmf_cfg80211_del_ap_iface(struct wiphy *wiphy,
+				       struct wireless_dev *wdev)
+{
+	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
+	struct net_device *ndev = wdev->netdev;
+	struct brcmf_if *ifp = netdev_priv(ndev);
+	int ret;
+	int err;
+
+	brcmf_cfg80211_arm_vif_event(cfg, ifp->vif);
+
+	err = brcmf_fil_bsscfg_data_set(ifp, "interface_remove", NULL, 0);
+	if (err) {
+		brcmf_err("interface_remove failed %d\n", err);
+		goto err_unarm;
+	}
+
+	/* wait for firmware event */
+	ret = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_DEL,
+					    BRCMF_VIF_EVENT_TIMEOUT);
+	if (!ret) {
+		brcmf_err("timeout occurred\n");
+		err = -EIO;
+		goto err_unarm;
+	}
+
+	brcmf_remove_interface(ifp, true);
+
+err_unarm:
+	brcmf_cfg80211_arm_vif_event(cfg, NULL);
+	return err;
+}
+
 static
 int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 {
 	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
 	struct net_device *ndev = wdev->netdev;
 
+	if (ndev && ndev == cfg_to_ndev(cfg))
+		return -ENOTSUPP;
+
 	/* vif event pending in firmware */
 	if (brcmf_cfg80211_vif_event_armed(cfg))
 		return -EBUSY;
@@ -807,12 +843,13 @@ int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
 	case NL80211_IFTYPE_STATION:
-	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_AP_VLAN:
 	case NL80211_IFTYPE_WDS:
 	case NL80211_IFTYPE_MONITOR:
 	case NL80211_IFTYPE_MESH_POINT:
 		return -EOPNOTSUPP;
+	case NL80211_IFTYPE_AP:
+		return brcmf_cfg80211_del_ap_iface(wiphy, wdev);
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_P2P_GO:
 	case NL80211_IFTYPE_P2P_DEVICE:
-- 
1.8.4.5

[PATCH 1/2] brcmfmac: delete interface directly in code that sent fw request

[Rafał Miłecki]

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 <zajec5@gmail.com>
---
 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

Re: [1/2] brcmfmac: delete interface directly in code that sent fw request

[Kalle Valo]

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 <zajec5@gmail.com>

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/

[PATCH 2/2] brcmfmac: support removing AP interfaces with "interface_remove"

[Rafał Miłecki]

New firmwares (e.g. 10.10.69.36 for BCM4366) support "interface_remove"
for removing interfaces. Try to use this method on cfg80211 request. In
case of older firmwares (e.g. 7.35.177.56 for BCM43602 as I tested) this
will just result in firmware rejecting command and this won't change any
behavior.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 39 +++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index a5db953..6e6066a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -785,12 +785,48 @@ s32 brcmf_notify_escan_complete(struct brcmf_cfg80211_info *cfg,
 	return err;
 }
 
+static int brcmf_cfg80211_del_ap_iface(struct wiphy *wiphy,
+				       struct wireless_dev *wdev)
+{
+	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
+	struct net_device *ndev = wdev->netdev;
+	struct brcmf_if *ifp = netdev_priv(ndev);
+	int ret;
+	int err;
+
+	brcmf_cfg80211_arm_vif_event(cfg, ifp->vif);
+
+	err = brcmf_fil_bsscfg_data_set(ifp, "interface_remove", NULL, 0);
+	if (err) {
+		brcmf_err("interface_remove failed %d\n", err);
+		goto err_unarm;
+	}
+
+	/* wait for firmware event */
+	ret = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_DEL,
+					    BRCMF_VIF_EVENT_TIMEOUT);
+	if (!ret) {
+		brcmf_err("timeout occurred\n");
+		err = -EIO;
+		goto err_unarm;
+	}
+
+	brcmf_remove_interface(ifp, true);
+
+err_unarm:
+	brcmf_cfg80211_arm_vif_event(cfg, NULL);
+	return err;
+}
+
 static
 int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 {
 	struct brcmf_cfg80211_info *cfg = wiphy_priv(wiphy);
 	struct net_device *ndev = wdev->netdev;
 
+	if (ndev && ndev == cfg_to_ndev(cfg))
+		return -ENOTSUPP;
+
 	/* vif event pending in firmware */
 	if (brcmf_cfg80211_vif_event_armed(cfg))
 		return -EBUSY;
@@ -807,12 +843,13 @@ int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 	switch (wdev->iftype) {
 	case NL80211_IFTYPE_ADHOC:
 	case NL80211_IFTYPE_STATION:
-	case NL80211_IFTYPE_AP:
 	case NL80211_IFTYPE_AP_VLAN:
 	case NL80211_IFTYPE_WDS:
 	case NL80211_IFTYPE_MONITOR:
 	case NL80211_IFTYPE_MESH_POINT:
 		return -EOPNOTSUPP;
+	case NL80211_IFTYPE_AP:
+		return brcmf_cfg80211_del_ap_iface(wiphy, wdev);
 	case NL80211_IFTYPE_P2P_CLIENT:
 	case NL80211_IFTYPE_P2P_GO:
 	case NL80211_IFTYPE_P2P_DEVICE:
-- 
1.8.4.5

Re: [PATCH 0/2] brcmfmac: support removing AP interfaces with new fw

[Rafał Miłecki]

On 29 June 2016 at 21:54, Rafał Miłecki <zajec5@gmail.com> wrote:
> This is the latest patchset needed to get brcmfmac working reasonably well
> with BCM4366.
>
> Both patches were already sent as V2 RFC (10 days ago), there were no more
> comments since then and this is the same code as in V2 RFC. I was mostly waiting
> for accepting
> brcmfmac: fix lockup when removing P2P interface after event timeout
> before sending these 2 (because of dependency).
>
> Rafał Miłecki (2):
>   brcmfmac: delete interface directly in code that sent fw request
>   brcmfmac: support removing AP interfaces with "interface_remove"
>
>  .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 39 +++++++++++++++++++++-
>  .../wireless/broadcom/brcm80211/brcmfmac/fweh.c    | 10 ++++--
>  .../net/wireless/broadcom/brcm80211/brcmfmac/p2p.c |  3 +-
>  3 files changed, 47 insertions(+), 5 deletions(-)

Oops, I just realized get_maintainer.pl didn't pick any ppl/ML for
this cover letter. Added them now.

-- 
Rafał