| Message ID | 20200901063237.15549-1-wright.feng@cypress.com |
|---|---|
| Headers | show |
| Series | brcmfmac: Add few features in AP mode | expand |
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 於 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.
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
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