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

From: Wright Feng <Wright.Feng@cypress.com>
To: Arend Van Spriel <arend.vanspriel@broadcom.com>,
	Kalle Valo <kvalo@codeaurora.org>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"brcm80211-dev-list@broadcom.com"
	<brcm80211-dev-list@broadcom.com>,
	brcm80211-dev-list <brcm80211-dev-list@cypress.com>,
	Franky Lin <franky.lin@broadcom.com>,
	Hante Meuleman <hante.meuleman@broadcom.com>,
	Chi-Hsien Lin <Chi-Hsien.Lin@cypress.com>
Subject: Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Date: Tue, 8 Sep 2020 02:13:20 +0000	[thread overview]
Message-ID: <6cc3759f-9dc5-f505-4878-33700e41fb73@cypress.com> (raw)
In-Reply-To: <1746948ecc8.279b.9b12b7fc0a3841636cfb5e919b41b954@broadcom.com>



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


