[PATCH] brcmfmac: rework function picking free BSS index

[PATCH] brcmfmac: rework function picking free BSS index

[Rafał Miłecki]

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

Re: brcmfmac: rework function picking free BSS index

[Kalle Valo]

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>

Thanks, 1 patch applied to wireless-drivers-next.git:

d02fb8f14b2d brcmfmac: rework function picking free BSS index

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9136421/

Re: brcmfmac: rework function picking free BSS index

[Kalle Valo]

Kalle Valo <kvalo@codeaurora.org> writes:

> 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>
>
> Thanks, 1 patch applied to wireless-drivers-next.git:
>
> d02fb8f14b2d brcmfmac: rework function picking free BSS index

Now that patchwork.kernel.org is finally updated and I fixed my python
script, this is the first time I applied a patch using UTF-8 charset
directly from patchwork. As it's not exactly easy to get UTF-8 support
right in python, I would very much like to hear if there any issues
either with the commit below or the email above.

https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=d02fb8f14b2d

-- 
Kalle Valo

Re: brcmfmac: rework function picking free BSS index

[Kalle Valo]

Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:

> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Thanks, 1 patch applied to wireless-drivers-next.git:
>
> d02fb8f14b2d brcmfmac: rework function picking free BSS index

Now that patchwork.kernel.org is finally updated and I fixed my python
script, this is the first time I applied a patch using UTF-8 charset
directly from patchwork. As it's not exactly easy to get UTF-8 support
right in python, I would very much like to hear if there any issues
either with the commit below or the email above.

https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=d02fb8f14b2d

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: brcmfmac: rework function picking free BSS index

[Rafał Miłecki]

On 9 June 2016 at 18:30, Kalle Valo <kvalo@codeaurora.org> wrote:
> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> 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>
>>
>> Thanks, 1 patch applied to wireless-drivers-next.git:
>>
>> d02fb8f14b2d brcmfmac: rework function picking free BSS index
>
> Now that patchwork.kernel.org is finally updated and I fixed my python
> script, this is the first time I applied a patch using UTF-8 charset
> directly from patchwork. As it's not exactly easy to get UTF-8 support
> right in python, I would very much like to hear if there any issues
> either with the commit below or the email above.
>
> https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=d02fb8f14b2d

Look fine, thanks!

-- 
Rafał

Re: brcmfmac: rework function picking free BSS index

[Rafał Miłecki]

On 9 June 2016 at 18:30, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> writes:
>
>> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Thanks, 1 patch applied to wireless-drivers-next.git:
>>
>> d02fb8f14b2d brcmfmac: rework function picking free BSS index
>
> Now that patchwork.kernel.org is finally updated and I fixed my python
> script, this is the first time I applied a patch using UTF-8 charset
> directly from patchwork. As it's not exactly easy to get UTF-8 support
> right in python, I would very much like to hear if there any issues
> either with the commit below or the email above.
>
> https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=d02fb8f14b2d

Look fine, thanks!

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arend van Spriel]

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

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

[Arend van Spriel]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../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);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arend van Spriel]

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

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

[Arend van Spriel]

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

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

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

[Arend van Spriel]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../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);
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arend van Spriel]

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

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

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

[Rafał Miłecki]

On 13 June 2016 at 21:30, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> 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@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.
>
> 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.

Thanks for taking look at this.

-- 
Rafał

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

[Rafał Miłecki]

On 13 June 2016 at 21:30, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> 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@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.
>
> 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.

Thanks for taking look at this.

-- 
Rafał