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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 13 Jun 2016 21:30:31 +0200
From:	Arend van Spriel <arend.vanspriel@...adcom.com>
To:	Rafał Miłecki <zajec5@...il.com>,
	Kalle Valo <kvalo@...eaurora.org>
Cc:	Brett Rudley <brudley@...adcom.com>,
	Arend van Spriel <arend@...adcom.com>,
	"Franky (Zhenhui) Lin" <frankyl@...adcom.com>,
	Hante Meuleman <meuleman@...adcom.com>,
	Pieter-Paul Giesberts <pieterpg@...adcom.com>,
	Franky Lin <franky.lin@...adcom.com>,
	"open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
	<linux-wireless@...r.kernel.org>,
	"open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
	<brcm80211-dev-list@...adcom.com>,
	"open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] brcmfmac: rework function picking free BSS index



On 09-06-16 21:16, Arend van Spriel wrote:
> On 26-05-16 01:44, Rafał Miłecki wrote:
>> The old implementation was overcomplicated and slightly bugged in some
>> corner cases.
>>

[...]

>> 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@...il.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.

I tested STA on bsscfgidx=0, AP on bsscfgidx=1, and P2P_DEV on
bsscfgidx=2. P2P discovery on P2P_DEV interface works as expected so we
can indeed drop the 'bsscfgidx 1 is reserved' statement. I want to
verify on an older device before creating a patch.

Regards,
Arend

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