From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751847AbcFITQx (ORCPT ); Thu, 9 Jun 2016 15:16:53 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:38588 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751259AbcFITQu (ORCPT ); Thu, 9 Jun 2016 15:16:50 -0400 Subject: Re: [PATCH] brcmfmac: rework function picking free BSS index To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Kalle Valo References: <1464219876-13776-1-git-send-email-zajec5@gmail.com> Cc: Brett Rudley , Arend van Spriel , "Franky (Zhenhui) Lin" , Hante Meuleman , Pieter-Paul Giesberts , Franky Lin , "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" , "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" , "open list:NETWORKING DRIVERS" , open list From: Arend van Spriel Message-ID: <5759C09F.7020704@broadcom.com> Date: Thu, 9 Jun 2016 21:16:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1464219876-13776-1-git-send-email-zajec5@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > .../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); >