brcmfmac: Add few features in AP mode
cover brcmfmac: Add few features in AP mode
This patch series add support for AP isolation and forwarding mechanism in AP mode.
Jia-Shyr Chuang (1):
brcmfmac: support the forwarding packet
Ting-Ying Li (2):
brcmfmac: don't allow arp/nd offload to be enabled if ap mode exists
brcmfmac: add a variable for packet forwarding condition
Wright Feng (1):
brcmfmac: add change_bss to support AP isolation
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 64 ++++++++++- .../broadcom/brcm80211/brcmfmac/cfg80211.h | 1 + .../broadcom/brcm80211/brcmfmac/core.c | 105 +++++++++++++++++- .../broadcom/brcm80211/brcmfmac/core.h | 19 +++- .../broadcom/brcm80211/brcmfmac/feature.c | 1 + .../broadcom/brcm80211/brcmfmac/feature.h | 3 +- .../broadcom/brcm80211/brcmfmac/msgbuf.c | 31 +++++- 7 files changed, 217 insertions(+), 7 deletions(-)
Signature · 1 line
2.25.0
PATCH 1/4 brcmfmac: add change_bss to support AP isolation
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.
--- .../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(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +23 -0
@@ -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);
drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c +1 -0
@@ -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,
drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h +2 -1
@@ -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:
Signature · 1 line
2.25.0
Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Quoted 23 lines · Wright Feng
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 methodchange_bssto support setting AP isolation if firmware hasap_isolatefeature.Signed-off-by Wright Feng <[email protected]>Signed-off-by Chi-hsien Lin <[email protected]>--- .../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(-)drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +2
-0@@ -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,
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev, · line 5439
Quoted 10 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +10
-0+ 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.
Signature · 1 line
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Arend Van Spriel
maintainerRe: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Quoted 23 lines · Kalle Valo
Quoted 21 lines · Wright Feng
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 methodchange_bssto support setting AP isolation if firmware hasap_isolatefeature.Signed-off-by Wright Feng <[email protected]>Signed-off-by Chi-hsien Lin <[email protected]>--- .../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(-)drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +0
-0@@ -5425,6 +5425,26 @@ static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev,return brcmf_set_pmk (ifp,NULL ,0 ); }
Quoted 16 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev, · line 5440
Quoted 13 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +13
-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
Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Quoted 40 lines · Arend Van Spriel
Quoted 30 lines · Kalle Valo
Quoted 28 lines · Wright Feng
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 methodchange_bssto support setting AP isolation if firmware hasap_isolatefeature.Signed-off-by Wright Feng <[email protected]>Signed-off-by Chi-hsien Lin <[email protected]>--- .../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(-)drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +7
-0@@ -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; +Quoted 8 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev, · line 5440
Quoted 5 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +5
-0+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.
Quoted 3 lines
It seems to me. struct
bss_parameters::ap_isolateis typed as int. It is passed tobrcmf_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?
Signature · 1 line
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Kalle Valo 於 9/7/2020 5:49 PM 寫道:
Quoted 42 lines
Quoted 40 lines · Arend Van Spriel
Quoted 38 lines · Kalle Valo
Quoted 23 lines · Wright Feng
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 methodchange_bssto support setting AP isolation if firmware hasap_isolatefeature.Signed-off-by Wright Feng <[email protected]>Signed-off-by Chi-hsien Lin <[email protected]>--- .../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(-)drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +2
-0@@ -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)drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev, · line 5439
Quoted 10 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +10
-0+{ +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.
Quoted 6 lines
Quoted 3 lines
It seems to me. struct
bss_parameters::ap_isolateis typed as int. It is passed tobrcmf_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.
Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Quoted 20 lines · Wright Feng
Quoted 20 lines
Quoted 19 lines
Quoted 7 lines
Quoted 7 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +7
-0+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; +Quoted 8 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c static int brcmf_cfg80211_del_pmk(struct wiphy *wiphy, struct net_device *dev, · line 5440
Quoted 5 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +5
-0+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_isolateis typed as int. It is passed tobrcmf_fil_iovar_int_set()which requires a u32 (maybe function name is causing the confusion).
Quoted 7 lines
Quoted 2 lines
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 gettingparams->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
Signature · 1 line
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
A message that is not in the archive (sent off-list or not yet received): 01010174692f7c3f-4b7369b2-0665-4324-b1c8-57bd22ac9ce7-000000@us-west-2.amazonses.com
Arend Van Spriel
maintainerRe: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Quoted 30 lines · Kalle Valo
Quoted 20 lines · Wright Feng
Quoted 20 lines
Quoted 19 lines
Quoted 7 lines
Quoted 7 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +7
-0+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; +Quoted 8 lines
Quoted 5 lines
+
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_isolateis typed as int. It is passed tobrcmf_fil_iovar_int_set()which requires a u32 (maybe function name is causing the confusion).Quoted 7 lines
Quoted 2 lines
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 gettingparams->ap_isolate-1 and removes explicit type casting. Thanks for the comment.
Quoted 4 lines
Oh, I didn't realise
ap_isolatecan be -1 as structbss_parametersdidn'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
Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Arend Van Spriel 於 9/7/2020 11:57 PM 寫道:
Quoted 37 lines · Kalle Valo
Quoted 36 lines
Quoted 21 lines · Wright Feng
Quoted 21 lines
Quoted 20 lines
Quoted 8 lines
Quoted 8 lines
+brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct
net_device*dev,
+ structbss_parameters*params)(quoted diff) +5
-0+{ + struct brcmf_if *ifp; + int ret = 0; + u32 ap_isolate; +Quoted 8 lines
Quoted 5 lines
+
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_isolateis typed as int. It is passed tobrcmf_fil_iovar_int_set()which requires a u32 (maybe function name is causing the confusion).Quoted 8 lines
Quoted 3 lines
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 gettingparams->ap_isolate-1 and removes explicit type casting. Thanks for the comment.Oh, I didn't realise
ap_isolatecan be -1 as structbss_parametersdidn't document that. Can someone submit a patch to fix that?
Quoted 7 lines
Quoted 1 line
* @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.
---
include/net/cfg80211.h +1 -0
@@ -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
Re: [PATCH 1/4] brcmfmac: add change_bss to support AP isolation
Quoted 7 lines · Wright Feng
Quoted 7 lines
Quoted 4 lines
Oh, I didn't realise
ap_isolatecan be -1 as structbss_parametersdidn'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.
Quoted 8 lines
Quoted 2 lines
Regards,
ArendThe
ap_isolate-1 value innl80211_set_bssmeans not to changed.I intend to add a check of "params->ap_isolate>= 0" likeath/wil6210/cfg80211.cdoes inbrcmf_cfg80211_change_bss. And I will submit another patch to revise the comment incfg80211.has below. Let me know if you concern about it.
Great, thanks.
Signature · 1 line
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
PATCH 2/4 brcmfmac: don't allow arp/nd offload to be enabled if ap mode exists
In reply to Wright FengFrom: Ting-Ying Li <[email protected]>
Add a condition to determine whether arp/nd offload enabling request is allowed. If there is any interface acts as ap mode and is operating, then reject the request of arp oflload enabling from cfg80211.
--- .../broadcom/brcm80211/brcmfmac/cfg80211.c | 17 ++++++++++++++++- .../broadcom/brcm80211/brcmfmac/cfg80211.h | 1 + .../wireless/broadcom/brcm80211/brcmfmac/core.c | 5 +++++ 3 files changed, 22 insertions(+), 1 deletion(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +16 -1
@@ -766,6 +766,21 @@ void brcmf_set_mpc(struct brcmf_if *ifp, int mpc) } } +bool brcmf_is_apmode_operating (struct wiphy *wiphy) +{ +struct brcmf_cfg80211_info *cfg =wiphy_to_cfg (wiphy); +struct brcmf_cfg80211_vif *vif; +bool ret = false; + +list_for_each_entry (vif, &cfg->vif_list, list) { +if (brcmf_is_apmode (vif) && +test_bit (BRCMF_VIF_STATUS_AP_CREATED , &vif->sme_state)) + ret = true; + } + +return ret; +} +s32 brcmf_notify_escan_complete (struct brcmf_cfg80211_info *cfg,struct brcmf_if *ifp,bool aborted,bool fw_abort)
@@ -4949,8 +4964,8 @@ static int brcmf_cfg80211_stop_ap(struct wiphy *wiphy, struct net_device *ndev)bphy_err (drvr,"bss_enable config failed %d\n" , err); }brcmf_set_mpc (ifp,1 ); -brcmf_configure_arp_nd_offload (ifp, true);clear_bit (BRCMF_VIF_STATUS_AP_CREATED , &ifp->vif->sme_state); +brcmf_configure_arp_nd_offload (ifp, true);brcmf_net_setcarrier (ifp, false);return err;
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h +1 -0
@@ -447,5 +447,6 @@ s32 brcmf_notify_escan_complete(struct brcmf_cfg80211_info *cfg,void brcmf_set_mpc (struct brcmf_if *ndev,int mpc);void brcmf_abort_scanning (struct brcmf_cfg80211_info *cfg);void brcmf_cfg80211_free_netdev (struct net_device *ndev); +bool brcmf_is_apmode_operating (struct wiphy *wiphy);#endif /* BRCMFMAC_CFG80211_H */
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +5 -0
@@ -96,6 +96,11 @@ void brcmf_configure_arp_nd_offload(struct brcmf_if *ifp, bool enable)s32 err;u32 mode; +if (enable &&brcmf_is_apmode_operating (ifp->drvr->wiphy)) { +brcmf_dbg (TRACE ,"Skip ARP/ND offload enable when soft AP is running\n" ); +return ; + } +if (enable) mode =BRCMF_ARP_OL_AGENT |BRCMF_ARP_OL_PEER_AUTO_REPLY ;else
Signature · 1 line
2.25.0
PATCH 3/4 brcmfmac: support the forwarding packet
In reply to Wright FengFrom: Jia-Shyr Chuang <[email protected]>
Support packet forwarding mechanism for some special usages on PCIE, and fix BE/VI priority issue when pumping iperf.
--- .../broadcom/brcm80211/brcmfmac/cfg80211.c | 13 ++- .../broadcom/brcm80211/brcmfmac/core.c | 100 +++++++++++++++++- .../broadcom/brcm80211/brcmfmac/core.h | 18 +++- .../broadcom/brcm80211/brcmfmac/msgbuf.c | 31 +++++- 4 files changed, 157 insertions(+), 5 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +11 -2
@@ -4799,7 +4799,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, err = -EINVAL ; goto exit; } - + ifp->isap = false;/* Interface specific setup */ if (dev_role ==NL80211_IFTYPE_AP ) {if ((brcmf_feat_is_enabled (ifp,BRCMF_FEAT_MBSS )) && (!mbss))
@@ -4860,7 +4860,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, err); goto exit; } - + ifp->isap = true;brcmf_dbg (TRACE ,"AP mode configuration complete\n" ); }else if (dev_role ==NL80211_IFTYPE_P2P_GO ) { err =brcmf_fil_iovar_int_set (ifp,"chanspec" , chanspec);
@@ -4892,6 +4892,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, goto exit; } + ifp->isap = true;brcmf_dbg (TRACE ,"GO mode configuration complete\n" ); }else {WARN_ON (1 );
@@ -6045,6 +6046,14 @@ brcmf_notify_connect_status(struct brcmf_if *ifp, }if (brcmf_is_apmode (ifp->vif)) { +if (e->event_code ==BRCMF_E_ASSOC_IND || + e->event_code ==BRCMF_E_REASSOC_IND ) { +brcmf_findadd_sta (ifp, e->addr); + }else if ((e->event_code ==BRCMF_E_DISASSOC_IND ) || + (e->event_code ==BRCMF_E_DEAUTH_IND ) || + (e->event_code ==BRCMF_E_DEAUTH )) { +brcmf_del_sta (ifp, e->addr); + } err =brcmf_notify_connect_status_ap (cfg, ndev, e, data); }else if (brcmf_is_linkup (ifp->vif, e)) {brcmf_dbg (CONN ,"Linkup\n" );
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +99 -1
115 lines not on this page · raw message
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h +17 -1
@@ -185,6 +185,7 @@ struct brcmf_if {struct brcmf_fws_mac_descriptor *fws_desc;int ifidx;s32 bsscfgidx; +bool isap;u8 mac_addr[ETH_ALEN ];u8 netif_stop;spinlock_t netif_stop_lock;
@@ -193,6 +194,19 @@ struct brcmf_if {struct in6_addr ipv6_addr_tbl[NDOL_MAX_ENTRIES ];u8 ipv6addr_idx;bool fwil_fwerr; +struct list_head sta_list;/* sll of associated stations */ +spinlock_t sta_list_lock; +}; + +struct ether_addr { +u8 octet[ETH_ALEN ]; +}; + +/** Per STA params. A list of dhd_sta objects are managed in dhd_if */ +struct brcmf_sta { +void *ifp;/* associated brcm_if */ +struct ether_addr ea;/* stations ethernet mac address */ +struct list_head list;/* link into brcmf_if::sta_list */ };int brcmf_netdev_wait_pend8021x (struct brcmf_if *ifp);
@@ -215,5 +229,7 @@ int brcmf_net_mon_attach(struct brcmf_if *ifp);void brcmf_net_setcarrier (struct brcmf_if *ifp,bool on);int __init brcmf_core_init (void ); void __exit brcmf_core_exit(void); - +void brcmf_del_sta (struct brcmf_if *ifp,const u8 *ea); +struct brcmf_sta *brcmf_find_sta (struct brcmf_if *ifp,const u8 *ea); +struct brcmf_sta *brcmf_findadd_sta (struct brcmf_if *ifp,const u8 *ea);#endif /* BRCMFMAC_CORE_H */
drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c +30 -1
@@ -1140,7 +1140,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf) {struct brcmf_pub *drvr = msgbuf->drvr;struct msgbuf_rx_complete *rx_complete; -struct sk_buff *skb; +struct sk_buff *skb, *cpskb =NULL ; +struct ethhdr *eh;u16 data_offset;u16 buflen;u16 flags;
@@ -1189,6 +1190,34 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)return ; } + eh = (struct ethhdr *)(skb->data); +if (ifp->isap) { +skb_set_network_header (skb,sizeof (struct ethhdr )); + skb->protocol = eh->h_proto; + skb->priority =cfg80211_classify8021d (skb,NULL ); +if (is_unicast_ether_addr (eh->h_dest)) { +if (brcmf_find_sta (ifp, eh->h_dest)) { +/* determine the priority */ +if (skb->priority ==0 || skb->priority >7 ) { + skb->priority = +cfg80211_classify8021d (skb, +NULL ); + } +brcmf_proto_tx_queue_data (ifp->drvr, + ifp->ifidx, skb); +return ; + } + }else { + cpskb =pskb_copy (skb,GFP_ATOMIC ); +if (cpskb) { +brcmf_proto_tx_queue_data (ifp->drvr, + ifp->ifidx, + cpskb); + }else { +brcmf_err ("Unable to do skb copy\n" ); + } + } + } skb->protocol =eth_type_trans (skb, ifp->ndev);brcmf_netif_rx (ifp, skb); }
Signature · 1 line
2.25.0
Arend Van Spriel
maintainerRe: [PATCH 3/4] brcmfmac: support the forwarding packet
Quoted 4 lines · Wright Feng
From: Jia-Shyr Chuang <[email protected]>
Support packet forwarding mechanism for some special usages on PCIE, and fix BE/VI priority issue when pumping iperf.
This is a bit to brief for what is being introduced here. Basically, your are introducing a shortcut if a packet from an associated STA is destined for another associated STA taking the linux network stack and its overhead out of the equation. Not sure if this is a desired feature for upstream driver as it likely is interfering with features like TSQ. So there should be something done to inform the network stack about this packet going into transmit path.
I also don't understand what is PCIe specific about this and what BE/VI priority issue is being fixed. Please spend more words on the commit message.
Some more specific comment below.
Regards,
Arend
Quoted 41 lines
Signed-off-by Jia-Shyr Chuang <[email protected]>Signed-off-by Ting-Ying Li <[email protected]>Signed-off-by Chi-hsien Lin <[email protected]>--- .../broadcom/brcm80211/brcmfmac/cfg80211.c | 13 ++- .../broadcom/brcm80211/brcmfmac/core.c | 100 +++++++++++++++++- .../broadcom/brcm80211/brcmfmac/core.h | 18 +++- .../broadcom/brcm80211/brcmfmac/msgbuf.c | 31 +++++- 4 files changed, 157 insertions(+), 5 deletions(-)drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +3
-2@@ -4799,7 +4799,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, err = -EINVAL ; goto exit; } - + ifp->isap = false;/* Interface specific setup */ if (dev_role ==NL80211_IFTYPE_AP ) {if ((brcmf_feat_is_enabled (ifp,BRCMF_FEAT_MBSS )) && (!mbss))@@ -4860,7 +4860,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, err); goto exit; } - + ifp->isap = true;brcmf_dbg (TRACE ,"AP mode configuration complete\n" ); }else if (dev_role ==NL80211_IFTYPE_P2P_GO ) { err =brcmf_fil_iovar_int_set (ifp,"chanspec" , chanspec);@@ -4892,6 +4892,7 @@ brcmf_cfg80211_start_ap(struct wiphy *wiphy, struct net_device *ndev, goto exit; } + ifp->isap = true;brcmf_dbg (TRACE ,"GO mode configuration complete\n" ); }else {WARN_ON (1 );
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c brcmf_notify_connect_status(struct brcmf_if *ifp, · line 6048
Quoted 3 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +0
-0@@ -6045,6 +6046,14 @@ brcmf_notify_connect_status(struct brcmf_if *ifp, }if (brcmf_is_apmode (ifp->vif)) {
Given this function I don't think the new struct brcmf_if::isap flag is necessary.
Quoted 16 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +8
-0+if (e->event_code ==BRCMF_E_ASSOC_IND || + e->event_code ==BRCMF_E_REASSOC_IND ) { +brcmf_findadd_sta (ifp, e->addr); + }else if ((e->event_code ==BRCMF_E_DISASSOC_IND ) || + (e->event_code ==BRCMF_E_DEAUTH_IND ) || + (e->event_code ==BRCMF_E_DEAUTH )) { +brcmf_del_sta (ifp, e->addr); + } err =brcmf_notify_connect_status_ap (cfg, ndev, e, data); }else if (brcmf_is_linkup (ifp->vif, e)) {brcmf_dbg (CONN ,"Linkup\n" );drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +0
-0
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c struct wlc_d11rxhdr { · line 71
Quoted 10 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +7
-0@@ -62,6 +62,14 @@ struct wlc_d11rxhdr {s8 rxpwr[4 ]; } __packed; +#define BRCMF_IF_STA_LIST_LOCK_INIT (ifp) spin_lock_init(&(ifp)->sta_list_lock) +#define BRCMF_IF_STA_LIST_LOCK (ifp, flags) \ + spin_lock_irqsave(&(ifp)->sta_list_lock, (flags)) +#define BRCMF_IF_STA_LIST_UNLOCK (ifp, flags) \ + spin_unlock_irqrestore(&(ifp)->sta_list_lock, (flags)) + +#define BRCMF_STA_NULL ((struct brcmf_sta *)NULL)
Don't hide actual functions with macros like these.
kernel test robot
build botRe: [PATCH 3/4] brcmfmac: support the forwarding packet
In reply to Wright FengThank you for the patch! Perhaps something to improve:
Hi Wright,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on wireless-drivers/master v5.9-rc3 next-20200902] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Wright-Feng/brcmfmac-Add-few-features-in-AP-mode/20200901-143506 base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master config: i386-randconfig-s002-20200902 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.2-191-g10164920-dirty # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
sparse warnings: (new ones prefixed by >>)
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:1594:18: sparse: sparse: symbol 'brcmf_add_sta' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
kernel test robot
build bot[RFC PATCH] brcmfmac: brcmf_add_sta can be static
In reply to Wright FengSigned-off-by: kernel test robot <[email protected]>
--- core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +1 -1
@@ -1591,7 +1591,7 @@ brcmf_find_sta(struct brcmf_if *ifp, const u8 *ea) }/** Add STA into the interface's STA list. */ -struct brcmf_sta * +static struct brcmf_sta *brcmf_add_sta (struct brcmf_if *ifp,const u8 *ea) {struct brcmf_sta *sta;
PATCH 4/4 brcmfmac: add a variable for packet forwarding condition
In reply to Wright FengFrom: Ting-Ying Li <[email protected]>
When the "ap_isolate" function is not set by the host, host-based packet forwarding will be enabled if the packet forwarding mechanism is not offloaded to the lower layer.
--- .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 13 ++++++++++++- .../net/wireless/broadcom/brcm80211/brcmfmac/core.h | 1 + .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 4 ++-- 3 files changed, 15 insertions(+), 3 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +12 -1
@@ -5447,7 +5447,7 @@ brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev, {struct brcmf_if *ifp;int ret =0 ; -u32 ap_isolate; +u32 ap_isolate, val;brcmf_dbg (TRACE ,"Enter\n" ); ifp =netdev_priv (dev);
@@ -5458,6 +5458,17 @@ brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev,brcmf_err ("ap_isolate iovar failed: ret=%d\n" , ret); } +/* Get ap_isolate value from firmware to detemine whether fmac */ +/* driver supports packet forwarding. */ +if (brcmf_fil_iovar_int_get (ifp,"ap_isolate" , &val) ==0 ) { + ifp->fmac_pkt_fwd_en = + ((params->ap_isolate ==0 ) && (val ==1 )) ? + true : false; + }else { +brcmf_err ("get ap_isolate iovar failed: ret=%d\n" , ret); + ifp->fmac_pkt_fwd_en = false; + } +return ret; }
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h +1 -0
@@ -196,6 +196,7 @@ struct brcmf_if {bool fwil_fwerr;struct list_head sta_list;/* sll of associated stations */ spinlock_t sta_list_lock; +bool fmac_pkt_fwd_en; };struct ether_addr {
drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c +2 -2
@@ -1190,8 +1190,8 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)return ; } - eh = (struct ethhdr *)(skb->data); -if (ifp->isap) { +if (ifp->isap && ifp->fmac_pkt_fwd_en) { + eh = (struct ethhdr *)(skb->data);skb_set_network_header (skb,sizeof (struct ethhdr )); skb->protocol = eh->h_proto; skb->priority =cfg80211_classify8021d (skb,NULL );
Signature · 1 line
2.25.0
Re: [PATCH 4/4] brcmfmac: add a variable for packet forwarding condition
Quoted 8 lines · Wright Feng
From: Ting-Ying Li <[email protected]>
When the "
ap_isolate" function is not set by the host, host-based packet forwarding will be enabled if the packet forwarding mechanism is not offloaded to the lower layer.Signed-off-by Ting-Ying Li <[email protected]>Signed-off-by Chi-hsien Lin <[email protected]>
[...]
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c brcmf_cfg80211_change_bss(struct wiphy *wiphy, struct net_device *dev, · line 5470
Quoted 10 lines
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +10
-0+/* Get ap_isolate value from firmware to detemine whether fmac */ +/* driver supports packet forwarding. */ +if (brcmf_fil_iovar_int_get (ifp,"ap_isolate" , &val) ==0 ) { + ifp->fmac_pkt_fwd_en = + ((params->ap_isolate ==0 ) && (val ==1 )) ? + true : false; + }else { +brcmf_err ("get ap_isolate iovar failed: ret=%d\n" , ret); + ifp->fmac_pkt_fwd_en = false; + }
This is hard to read, you can simplify it to:
if (brcmf_fil_iovar_int_get(ifp, "ap_isolate", &val) == 0 &&
params->ap_isolate == 0 &&
val == 1) {
ifp->fmac_pkt_fwd_en = true;
} else {
brcmf_err("get ap_isolate iovar failed: ret=%d\n", ret);
ifp->fmac_pkt_fwd_en = false;
}Signature · 1 line
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches