| Message ID | 20200901063237.15549-2-wright.feng@cypress.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: Add few features in AP mode | expand |
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; Is the cast to u32 really necessary? Please avoid casts as much as possible.
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
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?
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.
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
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
Wright Feng <Wright.Feng@cypress.com> 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.
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: