Re: [PATCH] brcmfmac: rework function picking free BSS index

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 26-05-16 01:44, Rafał Miłecki wrote:
> The old implementation was overcomplicated and slightly bugged in some
> corner cases.
> 
> Consider following state of BSS-es (limited to 6 for simplification):
> drvr->iflist[0]: { bsscfgidx:0, ndev->name:wlan1, }
> drvr->iflist[1]:  (null)
> drvr->iflist[2]: { bsscfgidx:2, ndev->name:wlan1-1, }
> drvr->iflist[3]: { bsscfgidx:3, ndev->name:wlan1-2, }
> drvr->iflist[4]:  (null)
> drvr->iflist[5]:  (null)
> In such case the next AP interface should bsscfgidx 4 (we don't use 1 as
> it's reserved for P2P).
> 
> With old code the loop iterations were following:
> [ifidx = 0] [bsscfgidx = 2] [highest = 2]
> [ifidx = 1] [bsscfgidx = 2] [highest = 2] available = true
> [ifidx = 2] [bsscfgidx = 2] [highest = 2] bsscfgidx = highest + 1
> [ifidx = 3] [bsscfgidx = 3] [highest = 2] bsscfgidx = highest + 1
> [ifidx = 4] [bsscfgidx = 3] [highest = 2] available = true
> [ifidx = 5] [bsscfgidx = 3] [highest = 2] available = true
> There were 2 obvious problems:
> 1) Having empty BSS at index 1 was resulting in available being always
>    set to true, even if we would run out of BSS-es.
> 2) Calculated bsscfgidx was invalid (3 instead of 4) resulting in driver
>    not being able to create the 4th AP interface.
> 
> New code is simpler, placed in file where it's really used, handles
> running out of free BSS-es and allows using 4 interfaces at the same
> time. It also looks for the first free BSS instead of one after the last
> in use. It works well with current driver (which doesn't allow deleting
> interfaces) and should be future proof (if we ever allow deleting).
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 17 ++++++++++++++-
>  .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 24 ----------------------
>  .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  1 -
>  3 files changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 3d09d23..d00eef8 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -541,6 +541,21 @@ brcmf_cfg80211_update_proto_addr_mode(struct wireless_dev *wdev)
>  						ADDR_INDIRECT);
>  }
>  
> +static int brcmf_get_first_free_bsscfgidx(struct brcmf_pub *drvr)
> +{
> +	int bsscfgidx;
> +
> +	for (bsscfgidx = 0; bsscfgidx < BRCMF_MAX_IFS; bsscfgidx++) {
> +		/* bsscfgidx 1 is reserved for legacy P2P */

Hi Rafał,

A bit late as the patch is already applied, but this reserved index is
no longer needed as we removed all trickery that was build on the
assumption that the P2P_DEVICE interface was always in bsscfgidx 1.
Hence this could be removed.

Regards,
Arend

> +		if (bsscfgidx == 1)
> +			continue;
> +		if (!drvr->iflist[bsscfgidx])
> +			return bsscfgidx;
> +	}
> +
> +	return -ENOMEM;
> +}
> +
>  static int brcmf_cfg80211_request_ap_if(struct brcmf_if *ifp)
>  {
>  	struct brcmf_mbss_ssid_le mbss_ssid_le;
> @@ -548,7 +563,7 @@ static int brcmf_cfg80211_request_ap_if(struct brcmf_if *ifp)
>  	int err;
>  
>  	memset(&mbss_ssid_le, 0, sizeof(mbss_ssid_le));
> -	bsscfgidx = brcmf_get_next_free_bsscfgidx(ifp->drvr);
> +	bsscfgidx = brcmf_get_first_free_bsscfgidx(ifp->drvr);
>  	if (bsscfgidx < 0)
>  		return bsscfgidx;
>  
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index b590499..6a76480 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -753,30 +753,6 @@ void brcmf_remove_interface(struct brcmf_if *ifp)
>  	brcmf_del_if(ifp->drvr, ifp->bsscfgidx);
>  }
>  
> -int brcmf_get_next_free_bsscfgidx(struct brcmf_pub *drvr)
> -{
> -	int ifidx;
> -	int bsscfgidx;
> -	bool available;
> -	int highest;
> -
> -	available = false;
> -	bsscfgidx = 2;
> -	highest = 2;
> -	for (ifidx = 0; ifidx < BRCMF_MAX_IFS; ifidx++) {
> -		if (drvr->iflist[ifidx]) {
> -			if (drvr->iflist[ifidx]->bsscfgidx == bsscfgidx)
> -				bsscfgidx = highest + 1;
> -			else if (drvr->iflist[ifidx]->bsscfgidx > highest)
> -				highest = drvr->iflist[ifidx]->bsscfgidx;
> -		} else {
> -			available = true;
> -		}
> -	}
> -
> -	return available ? bsscfgidx : -ENOMEM;
> -}
> -
>  #ifdef CONFIG_INET
>  #define ARPOL_MAX_ENTRIES	8
>  static int brcmf_inetaddr_changed(struct notifier_block *nb,
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> index 647d3cc..2a075c5 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
> @@ -217,7 +217,6 @@ 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);
>  void brcmf_remove_interface(struct brcmf_if *ifp);
> -int brcmf_get_next_free_bsscfgidx(struct brcmf_pub *drvr);
>  void brcmf_txflowblock_if(struct brcmf_if *ifp,
>  			  enum brcmf_netif_stop_reason reason, bool state);
>  void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
>