diff mbox series

[1/4] brcmfmac: add change_bss to support AP isolation

Message ID 20200901063237.15549-2-wright.feng@cypress.com
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series brcmfmac: Add few features in AP mode | expand

Commit Message

Wright Feng Sept. 1, 2020, 6:32 a.m. UTC
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(-)

Comments

Kalle Valo Sept. 7, 2020, 9:04 a.m. UTC | #1
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;

Is the cast to u32 really necessary? Please avoid casts as much as
possible.
Arend van Spriel Sept. 7, 2020, 9:21 a.m. UTC | #2
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
Kalle Valo Sept. 7, 2020, 9:49 a.m. UTC | #3
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?
Wright Feng Sept. 7, 2020, 10:09 a.m. UTC | #4
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.
Kalle Valo Sept. 7, 2020, 3:29 p.m. UTC | #5
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
Arend van Spriel Sept. 7, 2020, 3:57 p.m. UTC | #6
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 | #7
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
Kalle Valo Sept. 8, 2020, 4:29 a.m. UTC | #8
Wright Feng <Wright.Feng@cypress.com> writes:

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

Great, thanks.
diff mbox series

Patch

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);
+		if (ret < 0)
+			brcmf_err("ap_isolate iovar failed: ret=%d\n", ret);
+	}
+
+	return ret;
+}
+
 static struct cfg80211_ops brcmf_cfg80211_ops = {
 	.add_virtual_intf = brcmf_cfg80211_add_iface,
 	.del_virtual_intf = brcmf_cfg80211_del_iface,
@@ -7492,6 +7512,9 @@  struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_WOWL_GTK))
 		ops->set_rekey_data = brcmf_cfg80211_set_rekey_data;
 #endif
+	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_AP_ISOLATE))
+		ops->change_bss = brcmf_cfg80211_change_bss;
+
 	err = wiphy_register(wiphy);
 	if (err < 0) {
 		bphy_err(drvr, "Could not register wiphy device (%d)\n", err);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
index 0dcefbd0c000..1ffa9742612d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
@@ -278,6 +278,7 @@  void brcmf_feat_attach(struct brcmf_pub *drvr)
 	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_RSDB, "rsdb_mode");
 	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_TDLS, "tdls_enable");
 	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_MFP, "mfp");
+	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_AP_ISOLATE, "ap_isolate");
 
 	pfn_mac.version = BRCMF_PFN_MACADDR_CFG_VER;
 	err = brcmf_fil_iovar_data_get(ifp, "pfn_macaddr", &pfn_mac,
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
index cda3fc1bab7f..243d9afddb8c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
@@ -49,7 +49,8 @@ 
 	BRCMF_FEAT_DEF(MONITOR_FMT_RADIOTAP) \
 	BRCMF_FEAT_DEF(MONITOR_FMT_HW_RX_HDR) \
 	BRCMF_FEAT_DEF(DOT11H) \
-	BRCMF_FEAT_DEF(SAE)
+	BRCMF_FEAT_DEF(SAE) \
+	BRCMF_FEAT_DEF(AP_ISOLATE)
 
 /*
  * Quirks: