diff mbox

brcmfmac: remove setting IBSS mode when stopping AP

Message ID 1495099339-7626-1-git-send-email-wright.feng@cypress.com (mailing list archive)
State Accepted
Commit 9029679f66d976f8c720eb03c4898274803c9923
Delegated to: Kalle Valo
Headers show

Commit Message

Wright Feng May 18, 2017, 9:22 a.m. UTC
From: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>

Setting IBSS mode when stopping AP may affect functionality of INFRA
mode on another interface. For instance, if user creates and stops
hostap daemon on virtual interface, then association cannot work on
primary interface because default BSS has been set to IBSS mode in
firmware side. The IBSS mode should be set when cfg80211 changes the
interface.

Reviewed-by: Wright Feng <wright.feng@cypress.com>
Signed-off-by: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Kalle Valo May 19, 2017, 4:49 a.m. UTC | #1
Wright Feng <wright.feng@cypress.com> writes:

> From: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
>
> Setting IBSS mode when stopping AP may affect functionality of INFRA
> mode on another interface. For instance, if user creates and stops
> hostap daemon on virtual interface, then association cannot work on
> primary interface because default BSS has been set to IBSS mode in
> firmware side. The IBSS mode should be set when cfg80211 changes the
> interface.
>
> Reviewed-by: Wright Feng <wright.feng@cypress.com>
> Signed-off-by: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 944b83c..ce2a5b6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -4658,9 +4658,6 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev)
>  		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
>  		if (err < 0)
>  			brcmf_err("setting AP mode failed %d\n", err);
> -		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_INFRA, 0);
> -		if (err < 0)
> -			brcmf_err("setting INFRA mode failed %d\n", err);

The title talks about setting IBSS mode but the code looks to be about
setting INFRA mode. Is this correct?
Arend van Spriel May 19, 2017, 7:49 a.m. UTC | #2
On 5/19/2017 6:49 AM, Kalle Valo wrote:
> Wright Feng <wright.feng@cypress.com> writes:
> 
>> From: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
>>
>> Setting IBSS mode when stopping AP may affect functionality of INFRA
>> mode on another interface. For instance, if user creates and stops
>> hostap daemon on virtual interface, then association cannot work on
>> primary interface because default BSS has been set to IBSS mode in
>> firmware side. The IBSS mode should be set when cfg80211 changes the
>> interface.
>>
>> Reviewed-by: Wright Feng <wright.feng@cypress.com>
>> Signed-off-by: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index 944b83c..ce2a5b6 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -4658,9 +4658,6 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev)
>>   		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
>>   		if (err < 0)
>>   			brcmf_err("setting AP mode failed %d\n", err);
>> -		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_INFRA, 0);
>> -		if (err < 0)
>> -			brcmf_err("setting INFRA mode failed %d\n", err);
> 
> The title talks about setting IBSS mode but the code looks to be about
> setting INFRA mode. Is this correct?

Both are correct :-p Have to admit this is confusing. The command 
SET_INFRA with zero value means "disable INFRA; thus enable IBSS".

I would rephrase the commit message indeed explaining what the code 
currently does, ie.:

Upon stopping an AP interface the driver disable INFRA mode effectively 
setting the interface in IBSS mode. However, this may affect other 
interfaces running in INFRA mode. For instance, ....

Regards,
Arend
Kalle Valo May 19, 2017, 7:54 a.m. UTC | #3
Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 5/19/2017 6:49 AM, Kalle Valo wrote:
>> Wright Feng <wright.feng@cypress.com> writes:
>>
>>> From: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
>>>
>>> Setting IBSS mode when stopping AP may affect functionality of INFRA
>>> mode on another interface. For instance, if user creates and stops
>>> hostap daemon on virtual interface, then association cannot work on
>>> primary interface because default BSS has been set to IBSS mode in
>>> firmware side. The IBSS mode should be set when cfg80211 changes the
>>> interface.
>>>
>>> Reviewed-by: Wright Feng <wright.feng@cypress.com>
>>> Signed-off-by: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>>> ---
>>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> index 944b83c..ce2a5b6 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> @@ -4658,9 +4658,6 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev)
>>>   		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
>>>   		if (err < 0)
>>>   			brcmf_err("setting AP mode failed %d\n", err);
>>> -		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_INFRA, 0);
>>> -		if (err < 0)
>>> -			brcmf_err("setting INFRA mode failed %d\n", err);
>>
>> The title talks about setting IBSS mode but the code looks to be about
>> setting INFRA mode. Is this correct?
>
> Both are correct :-p Have to admit this is confusing. The command
> SET_INFRA with zero value means "disable INFRA; thus enable IBSS".

Ah, that IS confusing :)

> I would rephrase the commit message indeed explaining what the code
> currently does, ie.:
>
> Upon stopping an AP interface the driver disable INFRA mode
> effectively setting the interface in IBSS mode. However, this may
> affect other interfaces running in INFRA mode. For instance, ....

I can fix that during commit. Thanks.
Kalle Valo May 22, 2017, 3:21 p.m. UTC | #4
Wright Feng <wright.feng@cypress.com> wrote:
> From: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
> 
> Upon stopping an AP interface the driver disable INFRA mode effectively 
> setting the interface in IBSS mode. However, this may affect other 
> interfaces running in INFRA mode. For instance, if user creates and stops
> hostap daemon on virtual interface, then association cannot work on
> primary interface because default BSS has been set to IBSS mode in
> firmware side. The IBSS mode should be set when cfg80211 changes the
> interface.
> 
> Reviewed-by: Wright Feng <wright.feng@cypress.com>
> Signed-off-by: Chi-hsien Lin <Chi-Hsien.Lin@cypress.com>
> [kvalo@codeaurora.org: rephased commit log based on discussion]
> Signed-off-by: Wright Feng <wright.feng@cypress.com>

Patch applied to wireless-drivers-next.git, thanks.

9029679f66d9 brcmfmac: remove setting IBSS mode when stopping AP
diff mbox

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 944b83c..ce2a5b6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -4658,9 +4658,6 @@  static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev)
 		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_AP, 0);
 		if (err < 0)
 			brcmf_err("setting AP mode failed %d\n", err);
-		err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_INFRA, 0);
-		if (err < 0)
-			brcmf_err("setting INFRA mode failed %d\n", err);
 		if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS))
 			brcmf_fil_iovar_int_set(ifp, "mbss", 0);
 		brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_REGULATORY,