Arend Van Spriel 於 9/7/2020 11:57 PM 寫道:
> On September 7, 2020 5:29:14 PM Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote:
>
>> Wright Feng <Wright.Feng@xxxxxxxxxxx> 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
