[PATCH RFC] brcmfmac: support deleting MBSS AP interfaces

[PATCH RFC] brcmfmac: support deleting MBSS AP interfaces

[Rafał Miłecki]

We already support adding extra (AP) interfaces so it also makes an
obvious sense to allow deleting them.

Adding a new interface is implemented by sending request to firmware for
creating a new BSS and waiting for a proper event. Ideally deleting
interface should be handled in a similar way. There should be a request
to firmware for deleting BSS and firmware should respond with an event.

Unfortunately it doesn't seem to work with recent firmwares. They never
seem to delete BSS and never send BRCMF_E_IF_DEL. As a workaround this
patch deletes Linux interface while keeping a track of BSSes present in
a firmware. If there is request for adding a new interface this code is
capable of reusing existing BSS-es.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 84 +++++++++++++++-------
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    |  6 +-
 .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  4 +-
 .../wireless/broadcom/brcm80211/brcmfmac/fweh.c    |  4 +-
 4 files changed, 71 insertions(+), 27 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index d23b95e..ce35ada 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -35,6 +35,7 @@
 #include "cfg80211.h"
 #include "feature.h"
 #include "fwil.h"
+#include "fwsignal.h"
 #include "proto.h"
 #include "vendor.h"
 #include "bus.h"
@@ -556,10 +557,10 @@ static int brcmf_get_first_free_bsscfgidx(struct brcmf_pub *drvr)
 	return -ENOMEM;
 }
 
-static int brcmf_cfg80211_request_ap_if(struct brcmf_if *ifp)
+static int brcmf_cfg80211_request_ap_if(struct brcmf_cfg80211_info *cfg, int bsscfgidx)
 {
+	struct brcmf_if *ifp = netdev_priv(cfg_to_ndev(cfg));
 	struct brcmf_mbss_ssid_le mbss_ssid_le;
-	int bsscfgidx;
 	int err;
 
 	memset(&mbss_ssid_le, 0, sizeof(mbss_ssid_le));
@@ -594,6 +595,7 @@ struct wireless_dev *brcmf_ap_add_vif(struct wiphy *wiphy, const char *name,
 	struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(wiphy);
 	struct brcmf_if *ifp = netdev_priv(cfg_to_ndev(cfg));
 	struct brcmf_cfg80211_vif *vif;
+	int bsscfgidx;
 	int err;
 
 	if (brcmf_cfg80211_vif_event_armed(cfg))
@@ -605,30 +607,49 @@ struct wireless_dev *brcmf_ap_add_vif(struct wiphy *wiphy, const char *name,
 	if (IS_ERR(vif))
 		return (struct wireless_dev *)vif;
 
-	brcmf_cfg80211_arm_vif_event(cfg, vif);
+	bsscfgidx = brcmf_get_first_free_bsscfgidx(cfg->pub);
+	brcmf_err("bsscfgidx:%d cfg->pub->bss2if[bsscfgidx]:%d\n", bsscfgidx, cfg->pub->bss2if[bsscfgidx]);
+	if (cfg->pub->bss2if[bsscfgidx] != BRCMF_IDX_INVALID) {
+		ifp = brcmf_add_if(cfg->pub, bsscfgidx, cfg->pub->bss2if[bsscfgidx], false, "wlan%d", params->macaddr);
+		if (IS_ERR(ifp)) {
+			err = PTR_ERR(ifp);
+			goto fail;
+		}
+		brcmf_fws_add_interface(ifp);
 
-	err = brcmf_cfg80211_request_ap_if(ifp);
-	if (err) {
-		brcmf_cfg80211_arm_vif_event(cfg, NULL);
-		goto fail;
-	}
+		ifp->vif = vif;
+		vif->ifp = ifp;
+		if (ifp->ndev) {
+			vif->wdev.netdev = ifp->ndev;
+			ifp->ndev->ieee80211_ptr = &vif->wdev;
+			SET_NETDEV_DEV(ifp->ndev, wiphy_dev(cfg->wiphy));
+		}
+	} else {
+		brcmf_cfg80211_arm_vif_event(cfg, vif);
 
-	/* wait for firmware event */
-	err = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_ADD,
-					    BRCMF_VIF_EVENT_TIMEOUT);
-	brcmf_cfg80211_arm_vif_event(cfg, NULL);
-	if (!err) {
-		brcmf_err("timeout occurred\n");
-		err = -EIO;
-		goto fail;
-	}
+		err = brcmf_cfg80211_request_ap_if(cfg, bsscfgidx);
+		if (err) {
+			brcmf_cfg80211_arm_vif_event(cfg, NULL);
+			goto fail;
+		}
 
-	/* interface created in firmware */
-	ifp = vif->ifp;
-	if (!ifp) {
-		brcmf_err("no if pointer provided\n");
-		err = -ENOENT;
-		goto fail;
+		/* wait for firmware event */
+		err = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_ADD,
+						BRCMF_VIF_EVENT_TIMEOUT);
+		brcmf_cfg80211_arm_vif_event(cfg, NULL);
+		if (!err) {
+			brcmf_err("timeout occurred\n");
+			err = -EIO;
+			goto fail;
+		}
+
+		/* interface created in firmware */
+		ifp = vif->ifp;
+		if (!ifp) {
+			brcmf_err("no if pointer provided\n");
+			err = -ENOENT;
+			goto fail;
+		}
 	}
 
 	strncpy(ifp->ndev->name, name, sizeof(ifp->ndev->name) - 1);
@@ -781,12 +802,26 @@ 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 net_device *ndev = wdev->netdev;
+	struct brcmf_if *ifp = netdev_priv(ndev);
+
+	brcmf_remove_interface(ifp);
+
+	return 0;
+}
+
 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;
@@ -803,12 +838,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:
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 6a76480..1208c21 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -634,7 +634,7 @@ fail:
 }
 
 struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
-			      bool is_p2pdev, char *name, u8 *mac_addr)
+			      bool is_p2pdev, const char *name, u8 *mac_addr)
 {
 	struct brcmf_if *ifp;
 	struct net_device *ndev;
@@ -680,6 +680,8 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
 		/* store mapping ifidx to bsscfgidx */
 		if (drvr->if2bss[ifidx] == BRCMF_BSSIDX_INVALID)
 			drvr->if2bss[ifidx] = bsscfgidx;
+		if (drvr->bss2if[bsscfgidx] == BRCMF_IDX_INVALID)
+			drvr->bss2if[bsscfgidx] = ifidx;
 	}
 
 	ifp->drvr = drvr;
@@ -910,6 +912,8 @@ int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings)
 
 	for (i = 0; i < ARRAY_SIZE(drvr->if2bss); i++)
 		drvr->if2bss[i] = BRCMF_BSSIDX_INVALID;
+	for (i = 0; i < ARRAY_SIZE(drvr->bss2if); i++)
+		drvr->bss2if[i] = BRCMF_IDX_INVALID;
 
 	mutex_init(&drvr->proto_block);
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 2a075c5..edd322a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -29,6 +29,7 @@
 
 /* For supporting multiple interfaces */
 #define BRCMF_MAX_IFS	16
+#define BRCMF_IDX_INVALID	-1
 
 /* Small, medium and maximum buffer size for dcmd
  */
@@ -125,6 +126,7 @@ struct brcmf_pub {
 
 	struct brcmf_if *iflist[BRCMF_MAX_IFS];
 	s32 if2bss[BRCMF_MAX_IFS];
+	s32 bss2if[BRCMF_MAX_IFS];
 
 	struct mutex proto_block;
 	unsigned char proto_buf[BRCMF_DCMD_MAXLEN];
@@ -215,7 +217,7 @@ char *brcmf_ifname(struct brcmf_if *ifp);
 struct brcmf_if *brcmf_get_ifp(struct brcmf_pub *drvr, int ifidx);
 int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked);
 struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
-			      bool is_p2pdev, char *name, u8 *mac_addr);
+			      bool is_p2pdev, const char *name, u8 *mac_addr);
 void brcmf_remove_interface(struct brcmf_if *ifp);
 void brcmf_txflowblock_if(struct brcmf_if *ifp,
 			  enum brcmf_netif_stop_reason reason, bool state);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index b390561..5bde857 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -182,8 +182,10 @@ 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)
+	if (ifp && ifevent->action == BRCMF_E_IF_DEL) {
 		brcmf_remove_interface(ifp);
+		drvr->bss2if[ifp->bsscfgidx] = BRCMF_IDX_INVALID;
+	}
 }
 
 /**
-- 
1.8.4.5

Re: [PATCH RFC] brcmfmac: support deleting MBSS AP interfaces

[Arend van Spriel]

On 01-06-16 16:36, Rafał Miłecki wrote:
> We already support adding extra (AP) interfaces so it also makes an
> obvious sense to allow deleting them.
> 
> Adding a new interface is implemented by sending request to firmware for
> creating a new BSS and waiting for a proper event. Ideally deleting
> interface should be handled in a similar way. There should be a request
> to firmware for deleting BSS and firmware should respond with an event.
> 
> Unfortunately it doesn't seem to work with recent firmwares. They never
> seem to delete BSS and never send BRCMF_E_IF_DEL. As a workaround this
> patch deletes Linux interface while keeping a track of BSSes present in
> a firmware. If there is request for adding a new interface this code is
> capable of reusing existing BSS-es.

It is not so much an issue of recent firmware. Actually, on recent
firmware 7.x.y.z and higher there are other command to create *and*
delete additional interfaces. On the other hand we aim to support a
large number of devices going back to bcm4329 so we have to come up with
a scheme to use the new commands or fallback to old api. Let's hope we
can reuse much of this effort you put in.

> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 84 +++++++++++++++-------
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c    |  6 +-
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  4 +-
>  .../wireless/broadcom/brcm80211/brcmfmac/fweh.c    |  4 +-
>  4 files changed, 71 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index d23b95e..ce35ada 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -35,6 +35,7 @@
>  #include "cfg80211.h"
>  #include "feature.h"
>  #include "fwil.h"
> +#include "fwsignal.h"
>  #include "proto.h"
>  #include "vendor.h"
>  #include "bus.h"
> @@ -556,10 +557,10 @@ static int brcmf_get_first_free_bsscfgidx(struct brcmf_pub *drvr)
>  	return -ENOMEM;
>  }
>  
> -static int brcmf_cfg80211_request_ap_if(struct brcmf_if *ifp)
> +static int brcmf_cfg80211_request_ap_if(struct brcmf_cfg80211_info *cfg, int bsscfgidx)
>  {
> +	struct brcmf_if *ifp = netdev_priv(cfg_to_ndev(cfg));
>  	struct brcmf_mbss_ssid_le mbss_ssid_le;
> -	int bsscfgidx;
>  	int err;
>  
>  	memset(&mbss_ssid_le, 0, sizeof(mbss_ssid_le));
> @@ -594,6 +595,7 @@ struct wireless_dev *brcmf_ap_add_vif(struct wiphy *wiphy, const char *name,
>  	struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(wiphy);
>  	struct brcmf_if *ifp = netdev_priv(cfg_to_ndev(cfg));
>  	struct brcmf_cfg80211_vif *vif;
> +	int bsscfgidx;
>  	int err;
>  
>  	if (brcmf_cfg80211_vif_event_armed(cfg))
> @@ -605,30 +607,49 @@ struct wireless_dev *brcmf_ap_add_vif(struct wiphy *wiphy, const char *name,
>  	if (IS_ERR(vif))
>  		return (struct wireless_dev *)vif;
>  
> -	brcmf_cfg80211_arm_vif_event(cfg, vif);
> +	bsscfgidx = brcmf_get_first_free_bsscfgidx(cfg->pub);
> +	brcmf_err("bsscfgidx:%d cfg->pub->bss2if[bsscfgidx]:%d\n", bsscfgidx, cfg->pub->bss2if[bsscfgidx]);

Looks like debugging code. If you want to keep it, make it
brcmf_dbg(INFO, ...).

> +	if (cfg->pub->bss2if[bsscfgidx] != BRCMF_IDX_INVALID) {
> +		ifp = brcmf_add_if(cfg->pub, bsscfgidx, cfg->pub->bss2if[bsscfgidx], false, "wlan%d", params->macaddr);
> +		if (IS_ERR(ifp)) {
> +			err = PTR_ERR(ifp);
> +			goto fail;
> +		}
> +		brcmf_fws_add_interface(ifp);
>  
> -	err = brcmf_cfg80211_request_ap_if(ifp);
> -	if (err) {
> -		brcmf_cfg80211_arm_vif_event(cfg, NULL);
> -		goto fail;
> -	}
> +		ifp->vif = vif;
> +		vif->ifp = ifp;
> +		if (ifp->ndev) {
> +			vif->wdev.netdev = ifp->ndev;
> +			ifp->ndev->ieee80211_ptr = &vif->wdev;
> +			SET_NETDEV_DEV(ifp->ndev, wiphy_dev(cfg->wiphy));
> +		}
> +	} else {
> +		brcmf_cfg80211_arm_vif_event(cfg, vif);
>  
> -	/* wait for firmware event */
> -	err = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_ADD,
> -					    BRCMF_VIF_EVENT_TIMEOUT);
> -	brcmf_cfg80211_arm_vif_event(cfg, NULL);
> -	if (!err) {
> -		brcmf_err("timeout occurred\n");
> -		err = -EIO;
> -		goto fail;
> -	}
> +		err = brcmf_cfg80211_request_ap_if(cfg, bsscfgidx);
> +		if (err) {
> +			brcmf_cfg80211_arm_vif_event(cfg, NULL);
> +			goto fail;
> +		}
>  
> -	/* interface created in firmware */
> -	ifp = vif->ifp;
> -	if (!ifp) {
> -		brcmf_err("no if pointer provided\n");
> -		err = -ENOENT;
> -		goto fail;
> +		/* wait for firmware event */
> +		err = brcmf_cfg80211_wait_vif_event(cfg, BRCMF_E_IF_ADD,
> +						BRCMF_VIF_EVENT_TIMEOUT);
> +		brcmf_cfg80211_arm_vif_event(cfg, NULL);
> +		if (!err) {
> +			brcmf_err("timeout occurred\n");
> +			err = -EIO;
> +			goto fail;
> +		}
> +
> +		/* interface created in firmware */
> +		ifp = vif->ifp;
> +		if (!ifp) {
> +			brcmf_err("no if pointer provided\n");
> +			err = -ENOENT;
> +			goto fail;
> +		}
>  	}
>  
>  	strncpy(ifp->ndev->name, name, sizeof(ifp->ndev->name) - 1);
> @@ -781,12 +802,26 @@ 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 net_device *ndev = wdev->netdev;
> +	struct brcmf_if *ifp = netdev_priv(ndev);
> +
> +	brcmf_remove_interface(ifp);
> +
> +	return 0;
> +}
> +
>  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;
> @@ -803,12 +838,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:
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 6a76480..1208c21 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -634,7 +634,7 @@ fail:
>  }
>  
>  struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
> -			      bool is_p2pdev, char *name, u8 *mac_addr)
> +			      bool is_p2pdev, const char *name, u8 *mac_addr)

Seems like an unrelated change.

>  {
>  	struct brcmf_if *ifp;
>  	struct net_device *ndev;
> @@ -680,6 +680,8 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
>  		/* store mapping ifidx to bsscfgidx */
>  		if (drvr->if2bss[ifidx] == BRCMF_BSSIDX_INVALID)
>  			drvr->if2bss[ifidx] = bsscfgidx;
> +		if (drvr->bss2if[bsscfgidx] == BRCMF_IDX_INVALID)
> +			drvr->bss2if[bsscfgidx] = ifidx;
>  	}
>  
>  	ifp->drvr = drvr;
> @@ -910,6 +912,8 @@ int brcmf_attach(struct device *dev, struct brcmf_mp_device *settings)
>  
>  	for (i = 0; i < ARRAY_SIZE(drvr->if2bss); i++)
>  		drvr->if2bss[i] = BRCMF_BSSIDX_INVALID;
> +	for (i = 0; i < ARRAY_SIZE(drvr->bss2if); i++)
> +		drvr->bss2if[i] = BRCMF_IDX_INVALID;

both have size BRCMF_MAX_IFS so might stick with one loop as it is
unlikely the size of these two arrays will ever differ.

>  	mutex_init(&drvr->proto_block);
>  
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> index 2a075c5..edd322a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> @@ -29,6 +29,7 @@
>  
>  /* For supporting multiple interfaces */
>  #define BRCMF_MAX_IFS	16
> +#define BRCMF_IDX_INVALID	-1
>  
>  /* Small, medium and maximum buffer size for dcmd
>   */
> @@ -125,6 +126,7 @@ struct brcmf_pub {
>  
>  	struct brcmf_if *iflist[BRCMF_MAX_IFS];
>  	s32 if2bss[BRCMF_MAX_IFS];
> +	s32 bss2if[BRCMF_MAX_IFS];
>  
>  	struct mutex proto_block;
>  	unsigned char proto_buf[BRCMF_DCMD_MAXLEN];
> @@ -215,7 +217,7 @@ char *brcmf_ifname(struct brcmf_if *ifp);
>  struct brcmf_if *brcmf_get_ifp(struct brcmf_pub *drvr, int ifidx);
>  int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked);
>  struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
> -			      bool is_p2pdev, char *name, u8 *mac_addr);
> +			      bool is_p2pdev, const char *name, u8 *mac_addr);
>  void brcmf_remove_interface(struct brcmf_if *ifp);
>  void brcmf_txflowblock_if(struct brcmf_if *ifp,
>  			  enum brcmf_netif_stop_reason reason, bool state);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> index b390561..5bde857 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> @@ -182,8 +182,10 @@ 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)
> +	if (ifp && ifevent->action == BRCMF_E_IF_DEL) {
>  		brcmf_remove_interface(ifp);
> +		drvr->bss2if[ifp->bsscfgidx] = BRCMF_IDX_INVALID;

Why not do this in brcmf_remove_interface(). Seems more clean to me.

Regards,
Arend

Re: [PATCH RFC] brcmfmac: support deleting MBSS AP interfaces

[Rafał Miłecki]

On 1 June 2016 at 21:00, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> On 01-06-16 16:36, Rafał Miłecki wrote:
>> We already support adding extra (AP) interfaces so it also makes an
>> obvious sense to allow deleting them.
>>
>> Adding a new interface is implemented by sending request to firmware for
>> creating a new BSS and waiting for a proper event. Ideally deleting
>> interface should be handled in a similar way. There should be a request
>> to firmware for deleting BSS and firmware should respond with an event.
>>
>> Unfortunately it doesn't seem to work with recent firmwares. They never
>> seem to delete BSS and never send BRCMF_E_IF_DEL. As a workaround this
>> patch deletes Linux interface while keeping a track of BSSes present in
>> a firmware. If there is request for adding a new interface this code is
>> capable of reusing existing BSS-es.
>
> It is not so much an issue of recent firmware. Actually, on recent
> firmware 7.x.y.z and higher there are other command to create *and*
> delete additional interfaces. On the other hand we aim to support a
> large number of devices going back to bcm4329 so we have to come up with
> a scheme to use the new commands or fallback to old api. Let's hope we
> can reuse much of this effort you put in.

You gave me a complex puzzle there :D It took me a while to find out
what API you meant.

Finally I found an interesting wlioctl.h in SDK 9.10.178.27 that gave
me some clue. I got this SDK from ASUS RT-AC1200G+ open souce tarball.
There are 2 interesting structs:

typedef struct wl_interface_create {
        uint16 ver; /* version of this struct */
        uint32  flags; /* flags that defines the operation */
        struct ether_addr   mac_addr; /* Optional Mac address */
        uint32  wlc_index; /* Optional wlc index */
} wl_interface_create_t;

typedef struct wl_interface_info {
        uint16 ver; /* version of this struct */
        struct ether_addr    mac_addr; /* MAC address of the interface */
        char ifname[BCM_MSG_IFNAME_MAX]; /* name of interface */
        uint8 bsscfgidx; /* source bsscfg index */
} wl_interface_info_t;

I couldn't find any corresponding WLC_* in wlioctl_defs.h, so I guess
I should use WLC_SET_VAR (or WLC_SET_VAR as you prefer) with some
string. Any tip what would it be? Something like
"wl_interface_create"? Can you reveal such a small secret?

Also can you share any tip on removing interface? I don't see any
struct for that. Is that handled by some special value in
"wl_interface_create_t" or by separated string used with WLC_SET_VAR?

Re: [PATCH RFC] brcmfmac: support deleting MBSS AP interfaces

[Arend van Spriel]

On 17-06-16 14:30, Rafał Miłecki wrote:
> On 1 June 2016 at 21:00, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
>> On 01-06-16 16:36, Rafał Miłecki wrote:
>>> We already support adding extra (AP) interfaces so it also makes an
>>> obvious sense to allow deleting them.
>>>
>>> Adding a new interface is implemented by sending request to firmware for
>>> creating a new BSS and waiting for a proper event. Ideally deleting
>>> interface should be handled in a similar way. There should be a request
>>> to firmware for deleting BSS and firmware should respond with an event.
>>>
>>> Unfortunately it doesn't seem to work with recent firmwares. They never
>>> seem to delete BSS and never send BRCMF_E_IF_DEL. As a workaround this
>>> patch deletes Linux interface while keeping a track of BSSes present in
>>> a firmware. If there is request for adding a new interface this code is
>>> capable of reusing existing BSS-es.
>>
>> It is not so much an issue of recent firmware. Actually, on recent
>> firmware 7.x.y.z and higher there are other command to create *and*
>> delete additional interfaces. On the other hand we aim to support a
>> large number of devices going back to bcm4329 so we have to come up with
>> a scheme to use the new commands or fallback to old api. Let's hope we
>> can reuse much of this effort you put in.
> 
> You gave me a complex puzzle there :D It took me a while to find out
> what API you meant.

Actually, the puzzle was supposed to be for me, but I like your
curiosity and persistence in digging up the (partial) info.

> Finally I found an interesting wlioctl.h in SDK 9.10.178.27 that gave
> me some clue. I got this SDK from ASUS RT-AC1200G+ open souce tarball.
> There are 2 interesting structs:
> 
> typedef struct wl_interface_create {
>         uint16 ver; /* version of this struct */
>         uint32  flags; /* flags that defines the operation */
>         struct ether_addr   mac_addr; /* Optional Mac address */
>         uint32  wlc_index; /* Optional wlc index */
> } wl_interface_create_t;
> 
> typedef struct wl_interface_info {
>         uint16 ver; /* version of this struct */
>         struct ether_addr    mac_addr; /* MAC address of the interface */
>         char ifname[BCM_MSG_IFNAME_MAX]; /* name of interface */
>         uint8 bsscfgidx; /* source bsscfg index */
> } wl_interface_info_t;
> 
> I couldn't find any corresponding WLC_* in wlioctl_defs.h, so I guess
> I should use WLC_SET_VAR (or WLC_SET_VAR as you prefer) with some

(huh)? anyway the api indeed uses what we call an iovar, ie.
string-based ioctl.

> string. Any tip what would it be? Something like
> "wl_interface_create"? Can you reveal such a small secret?

It is "interface_create" and "interface_remove".

Regards,
Arend

Re: [PATCH RFC] brcmfmac: support deleting MBSS AP interfaces

[Rafał Miłecki]

On 17 June 2016 at 21:00, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> On 17-06-16 14:30, Rafał Miłecki wrote:
>> On 1 June 2016 at 21:00, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
>>> On 01-06-16 16:36, Rafał Miłecki wrote:
>>>> We already support adding extra (AP) interfaces so it also makes an
>>>> obvious sense to allow deleting them.
>>>>
>>>> Adding a new interface is implemented by sending request to firmware for
>>>> creating a new BSS and waiting for a proper event. Ideally deleting
>>>> interface should be handled in a similar way. There should be a request
>>>> to firmware for deleting BSS and firmware should respond with an event.
>>>>
>>>> Unfortunately it doesn't seem to work with recent firmwares. They never
>>>> seem to delete BSS and never send BRCMF_E_IF_DEL. As a workaround this
>>>> patch deletes Linux interface while keeping a track of BSSes present in
>>>> a firmware. If there is request for adding a new interface this code is
>>>> capable of reusing existing BSS-es.
>>>
>>> It is not so much an issue of recent firmware. Actually, on recent
>>> firmware 7.x.y.z and higher there are other command to create *and*
>>> delete additional interfaces. On the other hand we aim to support a
>>> large number of devices going back to bcm4329 so we have to come up with
>>> a scheme to use the new commands or fallback to old api. Let's hope we
>>> can reuse much of this effort you put in.
>>
>> You gave me a complex puzzle there :D It took me a while to find out
>> what API you meant.
>
> Actually, the puzzle was supposed to be for me, but I like your
> curiosity and persistence in digging up the (partial) info.
>
>> Finally I found an interesting wlioctl.h in SDK 9.10.178.27 that gave
>> me some clue. I got this SDK from ASUS RT-AC1200G+ open souce tarball.
>> There are 2 interesting structs:
>>
>> typedef struct wl_interface_create {
>>         uint16 ver; /* version of this struct */
>>         uint32  flags; /* flags that defines the operation */
>>         struct ether_addr   mac_addr; /* Optional Mac address */
>>         uint32  wlc_index; /* Optional wlc index */
>> } wl_interface_create_t;
>>
>> typedef struct wl_interface_info {
>>         uint16 ver; /* version of this struct */
>>         struct ether_addr    mac_addr; /* MAC address of the interface */
>>         char ifname[BCM_MSG_IFNAME_MAX]; /* name of interface */
>>         uint8 bsscfgidx; /* source bsscfg index */
>> } wl_interface_info_t;
>>
>> I couldn't find any corresponding WLC_* in wlioctl_defs.h, so I guess
>> I should use WLC_SET_VAR (or WLC_SET_VAR as you prefer) with some
>
> (huh)? anyway the api indeed uses what we call an iovar, ie.
> string-based ioctl.

That's what I meant, sorry for wrong naming :)


>> string. Any tip what would it be? Something like
>> "wl_interface_create"? Can you reveal such a small secret?
>
> It is "interface_create" and "interface_remove".

It (almost) works, thanks! I just hit some bug in brcmfmac in handling
events. It can be exposed by deleting 2 interfaces quickly, one by
one, it seems. I'll debug this.

-- 
Rafał