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 <[email protected]>
Signed-off-by: Chi-hsien Lin <[email protected]>
---
.../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);
+ 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:
--
2.25.0
Wright Feng <[email protected]> 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 <[email protected]>
> Signed-off-by: Chi-hsien Lin <[email protected]>
> ---
> .../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.
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 9/7/2020 11:04 AM, Kalle Valo wrote:
> Wright Feng <[email protected]> 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 <[email protected]>
>> Signed-off-by: Chi-hsien Lin <[email protected]>
>> ---
>> .../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
Arend Van Spriel <[email protected]> writes:
> On 9/7/2020 11:04 AM, Kalle Valo wrote:
>> Wright Feng <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Chi-hsien Lin <[email protected]>
>>> ---
>>> .../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?
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo 於 9/7/2020 5:49 PM 寫道:
> Arend Van Spriel <[email protected]> writes:
>
>> On 9/7/2020 11:04 AM, Kalle Valo wrote:
>>> Wright Feng <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Chi-hsien Lin <[email protected]>
>>>> ---
>>>> .../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.
Wright Feng <[email protected]> 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
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On September 7, 2020 5:29:14 PM Kalle Valo <[email protected]> wrote:
> Wright Feng <[email protected]> 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
Arend Van Spriel 於 9/7/2020 11:57 PM 寫道:
> On September 7, 2020 5:29:14 PM Kalle Valo <[email protected]> wrote:
>
>> Wright Feng <[email protected]> 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
Wright Feng <[email protected]> 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.
--
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches