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

[Wright Feng <Wright.Feng@cypress.com>]

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.