[PATCH 0/4] brcmfmac: Add few features in AP mode

[PATCH 0/4] brcmfmac: Add few features in AP mode

[Wright Feng]

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(-)

-- 
2.25.0

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

[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 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);
+		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:
-- 
2.25.0

[PATCH 2/4] brcmfmac: don't allow arp/nd offload to be enabled if ap mode exists

[Wright Feng]

From: Ting-Ying Li <tingying.li@cypress.com>

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.

Signed-off-by: Ting-Ying Li <tingying.li@cypress.com>
---
 .../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(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 7ef93cd66b2c..d6972420d426 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -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;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
index 333fdf394f95..8c14d00c6d0e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h
@@ -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 */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index f89010a81ffb..20c510dca601 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -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
-- 
2.25.0

[PATCH 3/4] brcmfmac: support the forwarding packet

[Wright Feng]

From: Jia-Shyr Chuang <joseph.chuang@cypress.com>

Support packet forwarding mechanism for some special usages on PCIE,
and fix BE/VI priority issue when pumping iperf.

Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
Signed-off-by: Ting-Ying Li <tingying.li@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 .../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(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index d6972420d426..8c7941f85715 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -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");
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 20c510dca601..3257b784e019 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -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)
+
 char *brcmf_ifname(struct brcmf_if *ifp)
 {
 	if (!ifp)
@@ -899,7 +907,9 @@ struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bsscfgidx, s32 ifidx,
 
 	init_waitqueue_head(&ifp->pend_8021x_wait);
 	spin_lock_init(&ifp->netif_stop_lock);
-
+	BRCMF_IF_STA_LIST_LOCK_INIT(ifp);
+	 /* Initialize STA info list */
+	INIT_LIST_HEAD(&ifp->sta_list);
 	if (mac_addr != NULL)
 		memcpy(ifp->mac_addr, mac_addr, ETH_ALEN);
 
@@ -1557,3 +1567,91 @@ void __exit brcmf_core_exit(void)
 #endif
 }
 
+/** Find STA with MAC address ea in an interface's STA list. */
+struct brcmf_sta *
+brcmf_find_sta(struct brcmf_if *ifp, const u8 *ea)
+{
+	struct brcmf_sta  *sta;
+	unsigned long flags;
+
+	BRCMF_IF_STA_LIST_LOCK(ifp, flags);
+	list_for_each_entry(sta, &ifp->sta_list, list) {
+		if (!memcmp(sta->ea.octet, ea, ETH_ALEN)) {
+			brcmf_dbg(INFO, "Found STA: 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x into sta list\n",
+				  sta->ea.octet[0], sta->ea.octet[1],
+				  sta->ea.octet[2], sta->ea.octet[3],
+				  sta->ea.octet[4], sta->ea.octet[5]);
+			BRCMF_IF_STA_LIST_UNLOCK(ifp, flags);
+			return sta;
+		}
+	}
+	BRCMF_IF_STA_LIST_UNLOCK(ifp, flags);
+
+	return BRCMF_STA_NULL;
+}
+
+/** Add STA into the interface's STA list. */
+struct brcmf_sta *
+brcmf_add_sta(struct brcmf_if *ifp, const u8 *ea)
+{
+	struct brcmf_sta *sta;
+	unsigned long flags;
+
+	sta =  kzalloc(sizeof(*sta), GFP_KERNEL);
+	if (sta == BRCMF_STA_NULL) {
+		brcmf_err("Alloc failed\n");
+		return BRCMF_STA_NULL;
+	}
+	memcpy(sta->ea.octet, ea, ETH_ALEN);
+	brcmf_dbg(INFO, "Add STA: 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x into sta list\n",
+		  sta->ea.octet[0], sta->ea.octet[1],
+		  sta->ea.octet[2], sta->ea.octet[3],
+		  sta->ea.octet[4], sta->ea.octet[5]);
+
+	/* link the sta and the dhd interface */
+	sta->ifp = ifp;
+	INIT_LIST_HEAD(&sta->list);
+
+	BRCMF_IF_STA_LIST_LOCK(ifp, flags);
+
+	list_add_tail(&sta->list, &ifp->sta_list);
+
+	BRCMF_IF_STA_LIST_UNLOCK(ifp, flags);
+	return sta;
+}
+
+/** Delete STA from the interface's STA list. */
+void
+brcmf_del_sta(struct brcmf_if *ifp, const u8 *ea)
+{
+	struct brcmf_sta *sta, *next;
+	unsigned long flags;
+
+	BRCMF_IF_STA_LIST_LOCK(ifp, flags);
+	list_for_each_entry_safe(sta, next, &ifp->sta_list, list) {
+		if (!memcmp(sta->ea.octet, ea, ETH_ALEN)) {
+			brcmf_dbg(INFO, "del STA: 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x from sta list\n",
+				  ea[0], ea[1], ea[2], ea[3],
+				  ea[4], ea[5]);
+			list_del(&sta->list);
+			kfree(sta);
+		}
+	}
+
+	BRCMF_IF_STA_LIST_UNLOCK(ifp, flags);
+}
+
+/** Add STA if it doesn't exist. Not reentrant. */
+struct brcmf_sta*
+brcmf_findadd_sta(struct brcmf_if *ifp, const u8 *ea)
+{
+	struct brcmf_sta *sta = NULL;
+
+	sta = brcmf_find_sta(ifp, ea);
+
+	if (!sta) {
+		/* Add entry */
+		sta = brcmf_add_sta(ifp, ea);
+	}
+	return sta;
+}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 33b2ab3b54b0..3cf0b8c8d7b1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -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 */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index f1a20db8daab..c53c3cf96f92 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -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);
 }
-- 
2.25.0

[PATCH 4/4] brcmfmac: add a variable for packet forwarding condition

[Wright Feng]

From: Ting-Ying Li <tingying.li@cypress.com>

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 <tingying.li@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
---
 .../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(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 8c7941f85715..ac0095989e43 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -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;
 }
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 3cf0b8c8d7b1..d5745f48e003 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -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 {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index c53c3cf96f92..1c109257aefc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -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);
-- 
2.25.0

Re: [PATCH 3/4] brcmfmac: support the forwarding packet

[Arend Van Spriel]

On 9/1/2020 8:32 AM, Wright Feng wrote:
> From: Jia-Shyr Chuang <joseph.chuang@cypress.com>
> 
> 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

> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com>
> Signed-off-by: Ting-Ying Li <tingying.li@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../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(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index d6972420d426..8c7941f85715 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -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)) {

Given this function I don't think the new struct brcmf_if::isap flag is 
necessary.

> +		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");
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 20c510dca601..3257b784e019 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -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.

Re: [PATCH 3/4] brcmfmac: support the forwarding packet

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

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
Reported-by: kernel test robot <lkp@intel.com>


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@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40269 bytes --]

[RFC PATCH] brcmfmac: brcmf_add_sta can be static

[kernel test robot]

Signed-off-by: kernel test robot <lkp@intel.com>
---
 core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 595795e8cf6ac..c71c07e1f245a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -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;

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

[Kalle Valo]

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.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Arend Van Spriel]

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

Re: [PATCH 4/4] brcmfmac: add a variable for packet forwarding condition

[Kalle Valo]

Wright Feng <wright.feng@cypress.com> writes:

> From: Ting-Ying Li <tingying.li@cypress.com>
>
> 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 <tingying.li@cypress.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>

[...]

> +	/* 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;
}


-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Kalle Valo]

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?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Wright Feng]

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.

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

[Kalle Valo]

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

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[Arend Van Spriel]

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

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

[Wright Feng]

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

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

[Kalle Valo]

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.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches