mbox series

[0/4] brcmfmac: Add few features in AP mode

Message ID 20200901063237.15549-1-wright.feng@cypress.com
Headers show
Series brcmfmac: Add few features in AP mode | expand

Message

Wright Feng Sept. 1, 2020, 6:32 a.m. UTC
This patch series add support for AP isolation and forwarding mechanism
in AP mode.

Jia-Shyr Chuang (1):
  brcmfmac: support the forwarding packet

Ting-Ying Li (2):
  brcmfmac: don't allow arp/nd offload to be enabled if ap mode exists
  brcmfmac: add a variable for packet forwarding condition

Wright Feng (1):
  brcmfmac: add change_bss to support AP isolation

 .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  64 ++++++++++-
 .../broadcom/brcm80211/brcmfmac/cfg80211.h    |   1 +
 .../broadcom/brcm80211/brcmfmac/core.c        | 105 +++++++++++++++++-
 .../broadcom/brcm80211/brcmfmac/core.h        |  19 +++-
 .../broadcom/brcm80211/brcmfmac/feature.c     |   1 +
 .../broadcom/brcm80211/brcmfmac/feature.h     |   3 +-
 .../broadcom/brcm80211/brcmfmac/msgbuf.c      |  31 +++++-
 7 files changed, 217 insertions(+), 7 deletions(-)

Comments

Arend van Spriel Sept. 7, 2020, 9:21 a.m. UTC | #1
On 9/7/2020 11:04 AM, Kalle Valo wrote:
> Wright Feng <wright.feng@cypress.com> writes:
> 
>> Hostap has a parameter "ap_isolate" which is used to prevent low-level
>> bridging of frames between associated stations in the BSS.
>> Regarding driver side, we add cfg80211 ops method change_bss to support
>> setting AP isolation if firmware has ap_isolate feature.
>>
>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>> ---
>>   .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 23 +++++++++++++++++++
>>   .../broadcom/brcm80211/brcmfmac/feature.c     |  1 +
>>   .../broadcom/brcm80211/brcmfmac/feature.h     |  3 ++-
>>   3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index 5d99771c3f64..7ef93cd66b2c 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -5425,6 +5425,26 @@ static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev,
>>   	return brcmf_set_pmk(ifp, NULL, 0);
>>   }
>>   
>> +static int
>> +brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
>> +			  struct bss_parameters *params)
>> +{
>> +	struct brcmf_if *ifp;
>> +	int ret = 0;
>> +	u32 ap_isolate;
>> +
>> +	brcmf_dbg(TRACE, "Enter\n");
>> +	ifp = netdev_priv(dev);
>> +	if (params->ap_isolate >= 0) {
>> +		ap_isolate = (u32)params->ap_isolate;
 >> +		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", ap_isolate);
> 
> Is the cast to u32 really necessary? Please avoid casts as much as
> possible.

It seems to me. struct bss_parameters::ap_isolate is typed as int. It is 
passed to brcmf_fil_iovar_int_set() which requires a u32 (maybe function 
name is causing the confusion).

Regards,
Arend
Wright Feng Sept. 7, 2020, 10:09 a.m. UTC | #2
Kalle Valo 於 9/7/2020 5:49 PM 寫道:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> 

>> On 9/7/2020 11:04 AM, Kalle Valo wrote:

>>> Wright Feng <wright.feng@cypress.com> writes:

>>>

>>>> Hostap has a parameter "ap_isolate" which is used to prevent low-level

>>>> bridging of frames between associated stations in the BSS.

>>>> Regarding driver side, we add cfg80211 ops method change_bss to support

>>>> setting AP isolation if firmware has ap_isolate feature.

>>>>

>>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>

>>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>

>>>> ---

>>>>    .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 23 +++++++++++++++++++

>>>>    .../broadcom/brcm80211/brcmfmac/feature.c     |  1 +

>>>>    .../broadcom/brcm80211/brcmfmac/feature.h     |  3 ++-

>>>>    3 files changed, 26 insertions(+), 1 deletion(-)

>>>>

>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c

>>>> index 5d99771c3f64..7ef93cd66b2c 100644

>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c

>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c

>>>> @@ -5425,6 +5425,26 @@ static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev,

>>>>    	return brcmf_set_pmk(ifp, NULL, 0);

>>>>    }

>>>>    +static int

>>>> +brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,

>>>> +			  struct bss_parameters *params)

>>>> +{

>>>> +	struct brcmf_if *ifp;

>>>> +	int ret = 0;

>>>> +	u32 ap_isolate;

>>>> +

>>>> +	brcmf_dbg(TRACE, "Enter\n");

>>>> +	ifp = netdev_priv(dev);

>>>> +	if (params->ap_isolate >= 0) {

>>>> +		ap_isolate = (u32)params->ap_isolate;

>>>> +		ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", ap_isolate);

>>>

>>> Is the cast to u32 really necessary? Please avoid casts as much as

>>> possible.

>>

>> It seems to me. struct bss_parameters::ap_isolate is typed as int. It

>> is passed to brcmf_fil_iovar_int_set() which requires a u32 (maybe

>> function name is causing the confusion).

> 

> What extra value does this explicit type casting bring here? I don't see

> it. Implicit type casting would work the same, no?

The value will be -1, 0 or 1.
I will submit v2 patch that ignores doing iovar if getting 
params->ap_isolate -1 and removes explicit type casting. Thanks for the 
comment.
Arend van Spriel Sept. 7, 2020, 3:57 p.m. UTC | #3
On September 7, 2020 5:29:14 PM Kalle Valo <kvalo@codeaurora.org> wrote:

> Wright Feng <Wright.Feng@cypress.com> writes:
>
>>>>>> +brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,
>>>>>> +  struct bss_parameters *params)
>>>>>> +{
>>>>>> + struct brcmf_if *ifp;
>>>>>> + int ret = 0;
>>>>>> + u32 ap_isolate;
>>>>>> +
>>>>>> + brcmf_dbg(TRACE, "Enter\n");
>>>>>> + ifp = netdev_priv(dev);
>>>>>> + if (params->ap_isolate >= 0) {
>>>>>> + ap_isolate = (u32)params->ap_isolate;
>>>>>> + ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", ap_isolate);
>>>>>
>>>>> Is the cast to u32 really necessary? Please avoid casts as much as
>>>>> possible.
>>>>
>>>> It seems to me. struct bss_parameters::ap_isolate is typed as int. It
>>>> is passed to brcmf_fil_iovar_int_set() which requires a u32 (maybe
>>>> function name is causing the confusion).
>>>
>>> What extra value does this explicit type casting bring here? I don't see
>>> it. Implicit type casting would work the same, no?
>>
>> The value will be -1, 0 or 1.
>> I will submit v2 patch that ignores doing iovar if getting
>> params->ap_isolate -1 and removes explicit type casting. Thanks for the
>> comment.
>
> Oh, I didn't realise ap_isolate can be -1 as struct bss_parameters
> didn't document that. Can someone submit a patch to fix that?
>
> * @ap_isolate: do not forward packets between connected stations

Me too. I assumed it was a boolean reading that description.

Regards,
Arend
Wright Feng Sept. 8, 2020, 2:13 a.m. UTC | #4
Arend Van Spriel 於 9/7/2020 11:57 PM 寫道:
> On September 7, 2020 5:29:14 PM Kalle Valo <kvalo@codeaurora.org> wrote:

> 

>> Wright Feng <Wright.Feng@cypress.com> writes:

>>

>>>>>>> +brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device 

>>>>>>> *dev,

>>>>>>> +  struct bss_parameters *params)

>>>>>>> +{

>>>>>>> + struct brcmf_if *ifp;

>>>>>>> + int ret = 0;

>>>>>>> + u32 ap_isolate;

>>>>>>> +

>>>>>>> + brcmf_dbg(TRACE, "Enter\n");

>>>>>>> + ifp = netdev_priv(dev);

>>>>>>> + if (params->ap_isolate >= 0) {

>>>>>>> + ap_isolate = (u32)params->ap_isolate;

>>>>>>> + ret = brcmf_fil_iovar_int_set(ifp, "ap_isolate", ap_isolate);

>>>>>>

>>>>>> Is the cast to u32 really necessary? Please avoid casts as much as

>>>>>> possible.

>>>>>

>>>>> It seems to me. struct bss_parameters::ap_isolate is typed as int. It

>>>>> is passed to brcmf_fil_iovar_int_set() which requires a u32 (maybe

>>>>> function name is causing the confusion).

>>>>

>>>> What extra value does this explicit type casting bring here? I don't 

>>>> see

>>>> it. Implicit type casting would work the same, no?

>>>

>>> The value will be -1, 0 or 1.

>>> I will submit v2 patch that ignores doing iovar if getting

>>> params->ap_isolate -1 and removes explicit type casting. Thanks for the

>>> comment.

>>

>> Oh, I didn't realise ap_isolate can be -1 as struct bss_parameters

>> didn't document that. Can someone submit a patch to fix that?

>>

>> * @ap_isolate: do not forward packets between connected stations

> 

> Me too. I assumed it was a boolean reading that description.

> 

> Regards,

> Arend

> 

The ap_isolate -1 value in nl80211_set_bss means not to changed.I intend 
to add a check of "params->ap_isolate >= 0" like
ath/wil6210/cfg80211.c does in brcmf_cfg80211_change_bss.
And I will submit another patch to revise the comment in cfg80211.h as 
below. Let me know
if you concern about it.

---
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fc7e8807838d..f60281c866dc 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1764,6 +1764,7 @@ struct mpath_info {
    *     (or NULL for no change)
    * @basic_rates_len: number of basic rates
    * @ap_isolate: do not forward packets between connected stations
+ *     (0 = no, 1 = yes, -1 = do not change)
    * @ht_opmode: HT Operation mode
    *     (u16 = opmode, -1 = do not change)
    * @p2p_ctwindow: P2P CT Window (-1 = no change)
---

Regards,
Wright