[PATCH 1/3] brcmfmac: allow specifying features per firmware version

[PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Some features supported by firmware aren't advertised and there is no
way for a driver to query them. This includes e.g. monitor mode details.
Some firmwares support tagging monitor frames, some build radiotap
header but there is no way to detect it.

This commit adds table that will allow specifying features like:
{ "01-abcdef01", BIT(BRCMF_FEAT_FOO) }

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
index 876731c57bf5..1194d31d3902 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
@@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
 }
 #endif /* DEBUG */
 
+struct brcmf_feat_fwfeat {
+	const char * const fwid;
+	u32 flags;
+};
+
+static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
+};
+
+static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
+{
+	const struct brcmf_feat_fwfeat *e;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(brcmf_feat_fwfeat_map); i++) {
+		e = &brcmf_feat_fwfeat_map[i];
+		if (!strcmp(e->fwid, pub->fwver)) {
+			pub->feat_flags |= e->flags;
+			break;
+		}
+	}
+}
+
 /**
  * brcmf_feat_iovar_int_get() - determine feature through iovar query.
  *
@@ -216,6 +238,8 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
 	}
 	brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_FWSUP, "sup_wpa");
 
+	brcmf_feat_firmware_features(drvr);
+
 	/* set chip related quirks */
 	switch (drvr->bus_if->chip) {
 	case BRCM_CC_43236_CHIP_ID:
-- 
2.13.6

[PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

New Broadcom firmwares mark monitor mode packets using a newly defined
bit in the flags field. Use it to filter them out and pass to the
monitor interface. These defines were found in bcmmsgbuf.h from SDK.

As not every firmware generates radiotap header this commit introduces
BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
and prepends it with an empty radiotap header.

It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
will require some extra research.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 24 ++++++++++++++++++++++
 .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  2 ++
 .../wireless/broadcom/brcm80211/brcmfmac/feature.h |  6 +++++-
 .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 17 +++++++++++++++
 4 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 8d4511eaa9b9..f58d7f7f1359 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -21,6 +21,7 @@
 #include <net/cfg80211.h>
 #include <net/rtnetlink.h>
 #include <net/addrconf.h>
+#include <net/ieee80211_radiotap.h>
 #include <net/ipv6.h>
 #include <brcmu_utils.h>
 #include <brcmu_wifi.h>
@@ -404,6 +405,29 @@ void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
 		netif_rx_ni(skb);
 }
 
+void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb)
+{
+	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MON_FMT_RADIOTAP)) {
+		/* Do nothing */
+	} else {
+		struct ieee80211_radiotap_header *radiotap;
+
+		radiotap = skb_push(skb, sizeof(*radiotap));
+		memset(radiotap, 0, sizeof(*radiotap));
+		radiotap->it_len = sizeof(*radiotap);
+
+		/* TODO: what are these extra 4 bytes? */
+		skb->len -= 4;
+	}
+
+	skb->dev = ifp->ndev;
+	skb_reset_mac_header(skb);
+	skb->pkt_type = PACKET_OTHERHOST;
+	skb->protocol = htons(ETH_P_802_2);
+
+	brcmf_netif_rx(ifp, skb);
+}
+
 static int brcmf_rx_hdrpull(struct brcmf_pub *drvr, struct sk_buff *skb,
 			    struct brcmf_if **ifp)
 {
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index 401f50458686..dcf6e27cc16f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -121,6 +121,7 @@ struct brcmf_pub {
 
 	struct brcmf_if *iflist[BRCMF_MAX_IFS];
 	s32 if2bss[BRCMF_MAX_IFS];
+	struct brcmf_if *mon_if;
 
 	struct mutex proto_block;
 	unsigned char proto_buf[BRCMF_DCMD_MAXLEN];
@@ -216,6 +217,7 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
 			  enum brcmf_netif_stop_reason reason, bool state);
 void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
+void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb);
 void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on);
 int __init brcmf_core_init(void);
 void __exit brcmf_core_exit(void);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
index d1193825e559..6e417d104b7f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
@@ -33,6 +33,8 @@
  * MFP: 802.11w Management Frame Protection.
  * GSCAN: enhanced scan offload feature.
  * FWSUP: Firmware supplicant.
+ * MON_802_11_FLAG: monitor packets flagged as 802.11
+ * MON_FMT_RADIOTAP: monitor packets include radiotap header
  */
 #define BRCMF_FEAT_LIST \
 	BRCMF_FEAT_DEF(MBSS) \
@@ -48,7 +50,9 @@
 	BRCMF_FEAT_DEF(WOWL_ARP_ND) \
 	BRCMF_FEAT_DEF(MFP) \
 	BRCMF_FEAT_DEF(GSCAN) \
-	BRCMF_FEAT_DEF(FWSUP)
+	BRCMF_FEAT_DEF(FWSUP) \
+	BRCMF_FEAT_DEF(MON_802_11_FLAG) \
+	BRCMF_FEAT_DEF(MON_FMT_RADIOTAP)
 
 /*
  * Quirks:
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index 49d37ad96958..47a9318cccb8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -69,6 +69,8 @@
 #define BRCMF_MSGBUF_MAX_EVENTBUF_POST		8
 
 #define BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_3	0x01
+#define BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_11	0x02
+#define BRCMF_MSGBUF_PKT_FLAGS_FRAME_MASK	0x07
 #define BRCMF_MSGBUF_PKT_FLAGS_PRIO_SHIFT	5
 
 #define BRCMF_MSGBUF_TX_FLUSH_CNT1		32
@@ -1128,6 +1130,7 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 	struct sk_buff *skb;
 	u16 data_offset;
 	u16 buflen;
+	u16 flags;
 	u32 idx;
 	struct brcmf_if *ifp;
 
@@ -1137,6 +1140,7 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 	data_offset = le16_to_cpu(rx_complete->data_offset);
 	buflen = le16_to_cpu(rx_complete->data_len);
 	idx = le32_to_cpu(rx_complete->msg.request_id);
+	flags = le16_to_cpu(rx_complete->flags);
 
 	skb = brcmf_msgbuf_get_pktid(msgbuf->drvr->bus_if->dev,
 				     msgbuf->rx_pktids, idx);
@@ -1150,6 +1154,19 @@ brcmf_msgbuf_process_rx_complete(struct brcmf_msgbuf *msgbuf, void *buf)
 
 	skb_trim(skb, buflen);
 
+	if ((flags & BRCMF_MSGBUF_PKT_FLAGS_FRAME_MASK) ==
+	    BRCMF_MSGBUF_PKT_FLAGS_FRAME_802_11) {
+		ifp = msgbuf->drvr->mon_if;
+
+		if (!ifp) {
+			brcmf_err("Received unexpected monitor pkt\n");
+			brcmu_pkt_buf_free_skb(skb);
+		}
+
+		brcmf_netif_mon_rx(ifp, skb);
+		return;
+	}
+
 	ifp = brcmf_get_ifp(msgbuf->drvr, rx_complete->msg.ifidx);
 	if (!ifp || !ifp->ndev) {
 		brcmf_err("Received pkt for invalid ifidx %d\n",
-- 
2.13.6

[PATCH 3/3] brcmfmac: add initial support for monitor mode interface

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Right now it's limited to firmwares that mark monitor interface packets
with a special flag. It's required to distinguish them from other
interface packets as firmware doesn't use any unique ifidx for monitor
interface.

In the future one may also add support for older firmwares without
support for proper packet flags. That will require limiting interface
combos to allow monitor mode *only* and adjusting condition in the
brcmf_msgbuf_process_rx_complete().

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 107 +++++++++++++++++++--
 .../wireless/broadcom/brcm80211/brcmfmac/core.c    |  65 ++++++++++++-
 .../wireless/broadcom/brcm80211/brcmfmac/core.h    |   2 +
 .../wireless/broadcom/brcm80211/brcmfmac/fwil.h    |   2 +
 4 files changed, 168 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index f5b405c98047..bbb4f913eece 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -22,6 +22,7 @@
 #include <linux/vmalloc.h>
 #include <net/cfg80211.h>
 #include <net/netlink.h>
+#include <uapi/linux/if_arp.h>
 
 #include <brcmu_utils.h>
 #include <defs.h>
@@ -608,6 +609,82 @@ static bool brcmf_is_ibssmode(struct brcmf_cfg80211_vif *vif)
 	return vif->wdev.iftype == NL80211_IFTYPE_ADHOC;
 }
 
+/**
+ * brcmf_mon_add_vif() - create monitor mode virtual interface
+ *
+ * @wiphy: wiphy device of new interface.
+ * @name: name of the new interface.
+ */
+static struct wireless_dev *brcmf_mon_add_vif(struct wiphy *wiphy,
+					      const char *name)
+{
+	struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(wiphy);
+	struct brcmf_cfg80211_vif *vif;
+	struct net_device *ndev;
+	struct brcmf_if *ifp;
+	int err;
+
+	if (cfg->pub->mon_if) {
+		err = -EEXIST;
+		goto err_out;
+	}
+
+	vif = brcmf_alloc_vif(cfg, NL80211_IFTYPE_MONITOR);
+	if (IS_ERR(vif)) {
+		err = PTR_ERR(vif);
+		goto err_out;
+	}
+
+	ndev = alloc_netdev(sizeof(*ifp), name, NET_NAME_UNKNOWN, ether_setup);
+	if (!ndev) {
+		err = -ENOMEM;
+		goto err_free_vif;
+	}
+	ndev->type = ARPHRD_IEEE80211_RADIOTAP;
+	ndev->ieee80211_ptr = &vif->wdev;
+	ndev->needs_free_netdev = true;
+	ndev->priv_destructor = brcmf_cfg80211_free_netdev;
+	SET_NETDEV_DEV(ndev, wiphy_dev(cfg->wiphy));
+
+	ifp = netdev_priv(ndev);
+	ifp->vif = vif;
+	ifp->ndev = ndev;
+	ifp->drvr = cfg->pub;
+
+	vif->ifp = ifp;
+	vif->wdev.netdev = ndev;
+
+	err = brcmf_net_mon_attach(ifp);
+	if (err) {
+		brcmf_err("Failed to attach %s device\n", ndev->name);
+		free_netdev(ndev);
+		goto err_free_vif;
+	}
+
+	cfg->pub->mon_if = ifp;
+
+	return &vif->wdev;
+
+err_free_vif:
+	brcmf_free_vif(vif);
+err_out:
+	return ERR_PTR(err);
+}
+
+static int brcmf_mon_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
+{
+	struct brcmf_cfg80211_info *cfg = wiphy_to_cfg(wiphy);
+	struct net_device *ndev = wdev->netdev;
+
+	ndev->netdev_ops->ndo_stop(ndev);
+
+	brcmf_net_detach(ndev, true);
+
+	cfg->pub->mon_if = NULL;
+
+	return 0;
+}
+
 static struct wireless_dev *brcmf_cfg80211_add_iface(struct wiphy *wiphy,
 						     const char *name,
 						     unsigned char name_assign_type,
@@ -628,9 +705,10 @@ static struct wireless_dev *brcmf_cfg80211_add_iface(struct wiphy *wiphy,
 	case NL80211_IFTYPE_STATION:
 	case NL80211_IFTYPE_AP_VLAN:
 	case NL80211_IFTYPE_WDS:
-	case NL80211_IFTYPE_MONITOR:
 	case NL80211_IFTYPE_MESH_POINT:
 		return ERR_PTR(-EOPNOTSUPP);
+	case NL80211_IFTYPE_MONITOR:
+		return brcmf_mon_add_vif(wiphy, name);
 	case NL80211_IFTYPE_AP:
 		wdev = brcmf_ap_add_vif(wiphy, name, params);
 		break;
@@ -810,9 +888,10 @@ int brcmf_cfg80211_del_iface(struct wiphy *wiphy, struct wireless_dev *wdev)
 	case NL80211_IFTYPE_STATION:
 	case NL80211_IFTYPE_AP_VLAN:
 	case NL80211_IFTYPE_WDS:
-	case NL80211_IFTYPE_MONITOR:
 	case NL80211_IFTYPE_MESH_POINT:
 		return -EOPNOTSUPP;
+	case NL80211_IFTYPE_MONITOR:
+		return brcmf_mon_del_vif(wiphy, wdev);
 	case NL80211_IFTYPE_AP:
 		return brcmf_cfg80211_del_ap_iface(wiphy, wdev);
 	case NL80211_IFTYPE_P2P_CLIENT:
@@ -6339,9 +6418,10 @@ static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
 	struct ieee80211_iface_limit *c0_limits = NULL;
 	struct ieee80211_iface_limit *p2p_limits = NULL;
 	struct ieee80211_iface_limit *mbss_limits = NULL;
-	bool mbss, p2p;
-	int i, c, n_combos;
+	bool mon, mbss, p2p;
+	int i, c, n_combos, n_limits;
 
+	mon = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MON_802_11_FLAG);
 	mbss = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS);
 	p2p = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_P2P);
 
@@ -6353,14 +6433,21 @@ static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
 	wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
 				 BIT(NL80211_IFTYPE_ADHOC) |
 				 BIT(NL80211_IFTYPE_AP);
+	if (mon)
+		wiphy->interface_modes |= BIT(NL80211_IFTYPE_MONITOR);
 
 	c = 0;
 	i = 0;
-	c0_limits = kcalloc(p2p ? 3 : 2, sizeof(*c0_limits), GFP_KERNEL);
+	n_limits = 1 + mon + p2p ? 2 : 1;
+	c0_limits = kcalloc(n_limits, sizeof(*c0_limits), GFP_KERNEL);
 	if (!c0_limits)
 		goto err;
 	c0_limits[i].max = 1;
 	c0_limits[i++].types = BIT(NL80211_IFTYPE_STATION);
+	if (mon) {
+		c0_limits[i].max = 1;
+		c0_limits[i++].types = BIT(NL80211_IFTYPE_MONITOR);
+	}
 	if (p2p) {
 		if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MCHAN))
 			combo[c].num_different_channels = 2;
@@ -6406,14 +6493,20 @@ static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
 	if (mbss) {
 		c++;
 		i = 0;
-		mbss_limits = kcalloc(1, sizeof(*mbss_limits), GFP_KERNEL);
+		n_limits = 1 + mon;
+		mbss_limits = kcalloc(n_limits, sizeof(*mbss_limits),
+				      GFP_KERNEL);
 		if (!mbss_limits)
 			goto err;
 		mbss_limits[i].max = 4;
 		mbss_limits[i++].types = BIT(NL80211_IFTYPE_AP);
+		if (mon) {
+			mbss_limits[i].max = 1;
+			mbss_limits[i++].types = BIT(NL80211_IFTYPE_MONITOR);
+		}
 		combo[c].beacon_int_infra_match = true;
 		combo[c].num_different_channels = 1;
-		combo[c].max_interfaces = 4;
+		combo[c].max_interfaces = 4 + mon;
 		combo[c].n_limits = i;
 		combo[c].limits = mbss_limits;
 	}
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index f58d7f7f1359..3f62e75dfa44 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -623,7 +623,7 @@ int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked)
 	return -EBADE;
 }
 
-static void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked)
+void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked)
 {
 	if (ndev->reg_state == NETREG_REGISTERED) {
 		if (rtnl_locked)
@@ -636,6 +636,69 @@ static void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked)
 	}
 }
 
+static int brcmf_net_mon_open(struct net_device *ndev)
+{
+	struct brcmf_if *ifp = netdev_priv(ndev);
+	u32 monitor;
+	int err;
+
+	brcmf_dbg(TRACE, "Enter\n");
+
+	err = brcmf_fil_cmd_int_get(ifp, BRCMF_C_GET_MONITOR, &monitor);
+	if (err) {
+		brcmf_err("BRCMF_C_GET_MONITOR error (%d)\n", err);
+		return err;
+	} else if (monitor) {
+		brcmf_err("Monitor mode is already enabled\n");
+		return -EEXIST;
+	}
+
+	monitor = 3;
+	err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_MONITOR, monitor);
+	if (err)
+		brcmf_err("BRCMF_C_SET_MONITOR error (%d)\n", err);
+
+	return err;
+}
+
+static int brcmf_net_mon_stop(struct net_device *ndev)
+{
+	struct brcmf_if *ifp = netdev_priv(ndev);
+	u32 monitor;
+	int err;
+
+	brcmf_dbg(TRACE, "Enter\n");
+
+	monitor = 0;
+	err = brcmf_fil_cmd_int_set(ifp, BRCMF_C_SET_MONITOR, monitor);
+	if (err)
+		brcmf_err("BRCMF_C_SET_MONITOR error (%d)\n", err);
+
+	return err;
+}
+
+static const struct net_device_ops brcmf_netdev_ops_mon = {
+	.ndo_open = brcmf_net_mon_open,
+	.ndo_stop = brcmf_net_mon_stop,
+};
+
+int brcmf_net_mon_attach(struct brcmf_if *ifp)
+{
+	struct net_device *ndev;
+	int err;
+
+	brcmf_dbg(TRACE, "Enter\n");
+
+	ndev = ifp->ndev;
+	ndev->netdev_ops = &brcmf_netdev_ops_mon;
+
+	err = register_netdevice(ndev);
+	if (err)
+		brcmf_err("Failed to register %s device\n", ndev->name);
+
+	return err;
+}
+
 void brcmf_net_setcarrier(struct brcmf_if *ifp, bool on)
 {
 	struct net_device *ndev;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
index dcf6e27cc16f..2d37a2fc6a6f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.h
@@ -218,6 +218,8 @@ void brcmf_txflowblock_if(struct brcmf_if *ifp,
 void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success);
 void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb);
 void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb);
+void brcmf_net_detach(struct net_device *ndev, bool rtnl_locked);
+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);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
index 63b1287e2e6d..0d9492fd758d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h
@@ -60,6 +60,8 @@
 #define BRCMF_C_GET_PM				85
 #define BRCMF_C_SET_PM				86
 #define BRCMF_C_GET_REVINFO			98
+#define BRCMF_C_GET_MONITOR			107
+#define BRCMF_C_SET_MONITOR			108
 #define BRCMF_C_GET_CURR_RATESET		114
 #define BRCMF_C_GET_AP				117
 #define BRCMF_C_SET_AP				118
-- 
2.13.6

Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Arend van Spriel]

On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Some features supported by firmware aren't advertised and there is no
> way for a driver to query them. This includes e.g. monitor mode details.
> Some firmwares support tagging monitor frames, some build radiotap
> header but there is no way to detect it.
>
> This commit adds table that will allow specifying features like:
> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }

I have my reservations taking this route. Full-dongle monitor mode is 
not very reliable especially when running it next to regular STA/AP 
interface due to memory constraints. So enabling a feature from host 
side could cause issues that are hard to debug. So I think it would be 
better if the cap iovar would get a new flag for this. Please hold this 
patch and let me discuss internally.

some more specific comments below...

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index 876731c57bf5..1194d31d3902 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
>   }
>   #endif /* DEBUG */
>
> +struct brcmf_feat_fwfeat {
> +	const char * const fwid;
> +	u32 flags;

For consistency call this feat_flags as well.

> +};
> +
> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
> +};
> +
> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)

Try to keep name of the struct brcmf_pub instance 'drvr'. Maybe the 
function name could be brcmf_feat_firmware_overrides() instead.

Regards,
Arend

Re: [PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[kbuild test robot]

Hi Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on v4.17-rc6 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/brcmfmac-allow-specifying-features-per-firmware-version/20180523-160546
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:304:30: sparse: expression using sizeof(void)
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:417:34: sparse: incorrect type in assignment (different base types) @@    expected restricted __le16 [usertype] it_len @@    got 6 [usertype] it_len @@
   drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:417:34:    expected restricted __le16 [usertype] it_len
   drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c:417:34:    got unsigned long

vim +417 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c

   264	
   265	static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
   266						   struct net_device *ndev)
   267	{
   268		int ret;
   269		struct brcmf_if *ifp = netdev_priv(ndev);
   270		struct brcmf_pub *drvr = ifp->drvr;
   271		struct ethhdr *eh;
   272		int head_delta;
   273	
   274		brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
   275	
   276		/* Can the device send data? */
   277		if (drvr->bus_if->state != BRCMF_BUS_UP) {
   278			brcmf_err("xmit rejected state=%d\n", drvr->bus_if->state);
   279			netif_stop_queue(ndev);
   280			dev_kfree_skb(skb);
   281			ret = -ENODEV;
   282			goto done;
   283		}
   284	
   285		/* Some recent Broadcom's firmwares disassociate STA when they receive
   286		 * an 802.11f ADD frame. This behavior can lead to a local DoS security
   287		 * issue. Attacker may trigger disassociation of any STA by sending a
   288		 * proper Ethernet frame to the wireless interface.
   289		 *
   290		 * Moreover this feature may break AP interfaces in some specific
   291		 * setups. This applies e.g. to the bridge with hairpin mode enabled and
   292		 * IFLA_BRPORT_MCAST_TO_UCAST set. IAPP packet generated by a firmware
   293		 * will get passed back to the wireless interface and cause immediate
   294		 * disassociation of a just-connected STA.
   295		 */
   296		if (!drvr->settings->iapp && brcmf_skb_is_iapp(skb)) {
   297			dev_kfree_skb(skb);
   298			ret = -EINVAL;
   299			goto done;
   300		}
   301	
   302		/* Make sure there's enough writeable headroom */
   303		if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
 > 304			head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0);
   305	
   306			brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
   307				  brcmf_ifname(ifp), head_delta);
   308			atomic_inc(&drvr->bus_if->stats.pktcowed);
   309			ret = pskb_expand_head(skb, ALIGN(head_delta, NET_SKB_PAD), 0,
   310					       GFP_ATOMIC);
   311			if (ret < 0) {
   312				brcmf_err("%s: failed to expand headroom\n",
   313					  brcmf_ifname(ifp));
   314				atomic_inc(&drvr->bus_if->stats.pktcow_failed);
   315				goto done;
   316			}
   317		}
   318	
   319		/* validate length for ether packet */
   320		if (skb->len < sizeof(*eh)) {
   321			ret = -EINVAL;
   322			dev_kfree_skb(skb);
   323			goto done;
   324		}
   325	
   326		eh = (struct ethhdr *)(skb->data);
   327	
   328		if (eh->h_proto == htons(ETH_P_PAE))
   329			atomic_inc(&ifp->pend_8021x_cnt);
   330	
   331		/* determine the priority */
   332		if ((skb->priority == 0) || (skb->priority > 7))
   333			skb->priority = cfg80211_classify8021d(skb, NULL);
   334	
   335		ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);
   336		if (ret < 0)
   337			brcmf_txfinalize(ifp, skb, false);
   338	
   339	done:
   340		if (ret) {
   341			ndev->stats.tx_dropped++;
   342		} else {
   343			ndev->stats.tx_packets++;
   344			ndev->stats.tx_bytes += skb->len;
   345		}
   346	
   347		/* Return ok: we always eat the packet */
   348		return NETDEV_TX_OK;
   349	}
   350	
   351	void brcmf_txflowblock_if(struct brcmf_if *ifp,
   352				  enum brcmf_netif_stop_reason reason, bool state)
   353	{
   354		unsigned long flags;
   355	
   356		if (!ifp || !ifp->ndev)
   357			return;
   358	
   359		brcmf_dbg(TRACE, "enter: bsscfgidx=%d stop=0x%X reason=%d state=%d\n",
   360			  ifp->bsscfgidx, ifp->netif_stop, reason, state);
   361	
   362		spin_lock_irqsave(&ifp->netif_stop_lock, flags);
   363		if (state) {
   364			if (!ifp->netif_stop)
   365				netif_stop_queue(ifp->ndev);
   366			ifp->netif_stop |= reason;
   367		} else {
   368			ifp->netif_stop &= ~reason;
   369			if (!ifp->netif_stop)
   370				netif_wake_queue(ifp->ndev);
   371		}
   372		spin_unlock_irqrestore(&ifp->netif_stop_lock, flags);
   373	}
   374	
   375	void brcmf_netif_rx(struct brcmf_if *ifp, struct sk_buff *skb)
   376	{
   377		/* Most of Broadcom's firmwares send 802.11f ADD frame every time a new
   378		 * STA connects to the AP interface. This is an obsoleted standard most
   379		 * users don't use, so don't pass these frames up unless requested.
   380		 */
   381		if (!ifp->drvr->settings->iapp && brcmf_skb_is_iapp(skb)) {
   382			brcmu_pkt_buf_free_skb(skb);
   383			return;
   384		}
   385	
   386		if (skb->pkt_type == PACKET_MULTICAST)
   387			ifp->ndev->stats.multicast++;
   388	
   389		if (!(ifp->ndev->flags & IFF_UP)) {
   390			brcmu_pkt_buf_free_skb(skb);
   391			return;
   392		}
   393	
   394		ifp->ndev->stats.rx_bytes += skb->len;
   395		ifp->ndev->stats.rx_packets++;
   396	
   397		brcmf_dbg(DATA, "rx proto=0x%X\n", ntohs(skb->protocol));
   398		if (in_interrupt())
   399			netif_rx(skb);
   400		else
   401			/* If the receive is not processed inside an ISR,
   402			 * the softirqd must be woken explicitly to service
   403			 * the NET_RX_SOFTIRQ.  This is handled by netif_rx_ni().
   404			 */
   405			netif_rx_ni(skb);
   406	}
   407	
   408	void brcmf_netif_mon_rx(struct brcmf_if *ifp, struct sk_buff *skb)
   409	{
   410		if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MON_FMT_RADIOTAP)) {
   411			/* Do nothing */
   412		} else {
   413			struct ieee80211_radiotap_header *radiotap;
   414	
   415			radiotap = skb_push(skb, sizeof(*radiotap));
   416			memset(radiotap, 0, sizeof(*radiotap));
 > 417			radiotap->it_len = sizeof(*radiotap);
   418	
   419			/* TODO: what are these extra 4 bytes? */
   420			skb->len -= 4;
   421		}
   422	
   423		skb->dev = ifp->ndev;
   424		skb_reset_mac_header(skb);
   425		skb->pkt_type = PACKET_OTHERHOST;
   426		skb->protocol = htons(ETH_P_802_2);
   427	
   428		brcmf_netif_rx(ifp, skb);
   429	}
   430	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Rafał Miłecki]

On 23.05.2018 09:58, Arend van Spriel wrote:
> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Some features supported by firmware aren't advertised and there is no
>> way for a driver to query them. This includes e.g. monitor mode details.
>> Some firmwares support tagging monitor frames, some build radiotap
>> header but there is no way to detect it.
>>
>> This commit adds table that will allow specifying features like:
>> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
> 
> I have my reservations taking this route. Full-dongle monitor mode is not very reliable especially when running it next to regular STA/AP interface due to memory constraints. So enabling a feature from host side could cause issues that are hard to debug.

I was using this *really* intensively on BCM4366 and didn't notice any
problems. My use case is listening to the air traffic for 300 ms every
few seconds (and I got that running for months!).


 > So I think it would be better if the cap iovar would get a new flag for this. Please hold this patch and let me discuss internally.

That would be a pretty big limitation to have to wait for and use a
special firmware for this feature. Also considering time it takes to
release brcmfmac4366c-pcie.bin, Broadcom vs. Cypress, licensing issues
with Cypress, we will likely never get all firmwares updated properly.

As for me (!) it seems rather unacceptable (no offence!). I believe we
should have a free choice to use that discovered feature even if
Broadcom didn't test it for host machine purposes (just internal fw
purposes as I get it).


> some more specific comments below...
> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> index 876731c57bf5..1194d31d3902 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
>>   }
>>   #endif /* DEBUG */
>>
>> +struct brcmf_feat_fwfeat {
>> +    const char * const fwid;
>> +    u32 flags;
> 
> For consistency call this feat_flags as well.

Sure.


>> +};
>> +
>> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
>> +};
>> +
>> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
> 
> Try to keep name of the struct brcmf_pub instance 'drvr'.

Wait, doesn't "pub" make more sense for "struct brcmf_pub" than "drvr"?
:) I'm sure I also saw "pub" variables all over the code, so this isn't
some new/mine convention.


 > Maybe the function name could be brcmf_feat_firmware_overrides() instead.

Sure!

Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Arend van Spriel]

On 5/24/2018 7:27 AM, Rafał Miłecki wrote:
> On 23.05.2018 09:58, Arend van Spriel wrote:
>> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Some features supported by firmware aren't advertised and there is no
>>> way for a driver to query them. This includes e.g. monitor mode details.
>>> Some firmwares support tagging monitor frames, some build radiotap
>>> header but there is no way to detect it.
>>>
>>> This commit adds table that will allow specifying features like:
>>> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
>>
>> I have my reservations taking this route. Full-dongle monitor mode is
>> not very reliable especially when running it next to regular STA/AP
>> interface due to memory constraints. So enabling a feature from host
>> side could cause issues that are hard to debug.
>
> I was using this *really* intensively on BCM4366 and didn't notice any
> problems. My use case is listening to the air traffic for 300 ms every
> few seconds (and I got that running for months!).

I understand. I am just saying that we have quite a variety of devices 
that we support with brcmfmac and this allows enabling features from the 
host driver. So any patch adding an entry will need to be reviewed with 
more scrutiny.

>  > So I think it would be better if the cap iovar would get a new flag
> for this. Please hold this patch and let me discuss internally.
>
> That would be a pretty big limitation to have to wait for and use a
> special firmware for this feature. Also considering time it takes to
> release brcmfmac4366c-pcie.bin, Broadcom vs. Cypress, licensing issues
> with Cypress, we will likely never get all firmwares updated properly.
>
> As for me (!) it seems rather unacceptable (no offence!). I believe we
> should have a free choice to use that discovered feature even if
> Broadcom didn't test it for host machine purposes (just internal fw
> purposes as I get it).

Not sure if "unacceptable" is the right word here. Having it 
discoverable from the firmware itself seems preferred to me regardless. 
For the 4366 firmware you are right it is taking a lot of time. It has 
passed verification. It needed to pass sensitive topic of radar 
detection for FCC and ETSI. So I am preparing that release. So I just 
have to discuss this monitor feature tomorrow. That is not too long, is it?

>> some more specific comments below...
>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>   .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24
>>> ++++++++++++++++++++++
>>>   1 file changed, 24 insertions(+)
>>>
>>> diff --git
>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> index 876731c57bf5..1194d31d3902 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file
>>> *seq, void *data)
>>>   }
>>>   #endif /* DEBUG */
>>>
>>> +struct brcmf_feat_fwfeat {
>>> +    const char * const fwid;
>>> +    u32 flags;
>>
>> For consistency call this feat_flags as well.
>
> Sure.
>
>
>>> +};
>>> +
>>> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
>>> +};
>>> +
>>> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
>>
>> Try to keep name of the struct brcmf_pub instance 'drvr'.
>
> Wait, doesn't "pub" make more sense for "struct brcmf_pub" than "drvr"?
> :) I'm sure I also saw "pub" variables all over the code, so this isn't
> some new/mine convention.

We used to have a private and public structure long ago and we collapsed 
it into brcmf_pub. So not all places changed the variable to 'drvr' and 
I do not care that much for existing stuff. Could put the rename task on 
TODO list on wireless wiki, but not sure if newbies would look there. 
For new stuff let's try to stick with 'drvr'.

Regards,
Arend

Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Rafał Miłecki]

On 2018-05-24 09:52, Arend van Spriel wrote:
> On 5/24/2018 7:27 AM, Rafał Miłecki wrote:
>> On 23.05.2018 09:58, Arend van Spriel wrote:
>>> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>> 
>>>> Some features supported by firmware aren't advertised and there is 
>>>> no
>>>> way for a driver to query them. This includes e.g. monitor mode 
>>>> details.
>>>> Some firmwares support tagging monitor frames, some build radiotap
>>>> header but there is no way to detect it.
>>>> 
>>>> This commit adds table that will allow specifying features like:
>>>> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }
>>> 
>>> I have my reservations taking this route. Full-dongle monitor mode is
>>> not very reliable especially when running it next to regular STA/AP
>>> interface due to memory constraints. So enabling a feature from host
>>> side could cause issues that are hard to debug.
>> 
>> I was using this *really* intensively on BCM4366 and didn't notice any
>> problems. My use case is listening to the air traffic for 300 ms every
>> few seconds (and I got that running for months!).
> 
> I understand. I am just saying that we have quite a variety of devices
> that we support with brcmfmac and this allows enabling features from
> the host driver. So any patch adding an entry will need to be reviewed
> with more scrutiny.

Sounds OK to me!


>>  > So I think it would be better if the cap iovar would get a new flag
>> for this. Please hold this patch and let me discuss internally.
>> 
>> That would be a pretty big limitation to have to wait for and use a
>> special firmware for this feature. Also considering time it takes to
>> release brcmfmac4366c-pcie.bin, Broadcom vs. Cypress, licensing issues
>> with Cypress, we will likely never get all firmwares updated properly.
>> 
>> As for me (!) it seems rather unacceptable (no offence!). I believe we
>> should have a free choice to use that discovered feature even if
>> Broadcom didn't test it for host machine purposes (just internal fw
>> purposes as I get it).
> 
> Not sure if "unacceptable" is the right word here. Having it
> discoverable from the firmware itself seems preferred to me
> regardless. For the 4366 firmware you are right it is taking a lot of
> time. It has passed verification. It needed to pass sensitive topic of
> radar detection for FCC and ETSI. So I am preparing that release.

OT: is your brcmfmac4366c-pcie.bin going to include BCM4366E support?


> So I
> just have to discuss this monitor feature tomorrow. That is not too
> long, is it?

Sure thing, let us know when you know some details :)


>>> some more specific comments below...
>>> 
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>>   .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24
>>>> ++++++++++++++++++++++
>>>>   1 file changed, 24 insertions(+)
>>>> 
>>>> diff --git
>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> index 876731c57bf5..1194d31d3902 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>>> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct 
>>>> seq_file
>>>> *seq, void *data)
>>>>   }
>>>>   #endif /* DEBUG */
>>>> 
>>>> +struct brcmf_feat_fwfeat {
>>>> +    const char * const fwid;
>>>> +    u32 flags;
>>> 
>>> For consistency call this feat_flags as well.
>> 
>> Sure.
>> 
>> 
>>>> +};
>>>> +
>>>> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
>>>> +};
>>>> +
>>>> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
>>> 
>>> Try to keep name of the struct brcmf_pub instance 'drvr'.
>> 
>> Wait, doesn't "pub" make more sense for "struct brcmf_pub" than 
>> "drvr"?
>> :) I'm sure I also saw "pub" variables all over the code, so this 
>> isn't
>> some new/mine convention.
> 
> We used to have a private and public structure long ago and we
> collapsed it into brcmf_pub. So not all places changed the variable to
> 'drvr' and I do not care that much for existing stuff. Could put the
> rename task on TODO list on wireless wiki, but not sure if newbies
> would look there. For new stuff let's try to stick with 'drvr'.

Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Arend van Spriel]

On 5/24/2018 10:21 AM, Rafał Miłecki wrote:
>
> OT: is your brcmfmac4366c-pcie.bin going to include BCM4366E support?

I would think so. I actually tested with 4366E before passing the 
firmware for verification.

Regards,
Arend

Re: [PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[Arend van Spriel]

On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> New Broadcom firmwares mark monitor mode packets using a newly defined
> bit in the flags field. Use it to filter them out and pass to the
> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
>
> As not every firmware generates radiotap header this commit introduces
> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
> and prepends it with an empty radiotap header.
>
> It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
> will require some extra research.

So I went looking on my shelf and found the patch I made for SDIO a 
while back. It relies on firmware change and I did not introduce a 
firmware flag for it. I will send the patch as RFT so you can have a 
look at it.

I checked our internal driver and it turns out the raw vs. radiotap is a 
compilation option. So depending on customer they would get either 
firmware doing the radiotap header generation or the host driver, 
without any run-time way to determine it. Not nice for brcmfmac.

Regards,
Arend

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 24 ++++++++++++++++++++++
>   .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  2 ++
>   .../wireless/broadcom/brcm80211/brcmfmac/feature.h |  6 +++++-
>   .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 17 +++++++++++++++
>   4 files changed, 48 insertions(+), 1 deletion(-)
>

Re: [PATCH 1/3] brcmfmac: allow specifying features per firmware version

[Arend van Spriel]

On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Some features supported by firmware aren't advertised and there is no
> way for a driver to query them. This includes e.g. monitor mode details.
> Some firmwares support tagging monitor frames, some build radiotap
> header but there is no way to detect it.
>
> This commit adds table that will allow specifying features like:
> { "01-abcdef01", BIT(BRCMF_FEAT_FOO) }

Discussed this over here. As we are releasing 4366c1 firmware we could 
add a proper flag, but there may be other devices/firmwares that can 
support certain features so this approach is an acceptable 
alternative.... (more below)

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 24 ++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> index 876731c57bf5..1194d31d3902 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
> @@ -91,6 +91,28 @@ static int brcmf_feat_debugfs_read(struct seq_file *seq, void *data)
>   }
>   #endif /* DEBUG */
>
> +struct brcmf_feat_fwfeat {
> +	const char * const fwid;
> +	u32 flags;
> +};
> +
> +static const struct brcmf_feat_fwfeat brcmf_feat_fwfeat_map[] = {
> +};
> +
> +static void brcmf_feat_firmware_features(struct brcmf_pub *pub)
> +{
> +	const struct brcmf_feat_fwfeat *e;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(brcmf_feat_fwfeat_map); i++) {
> +		e = &brcmf_feat_fwfeat_map[i];
> +		if (!strcmp(e->fwid, pub->fwver)) {
> +			pub->feat_flags |= e->flags;

... However, let's be more verbose about this here.

Regards,
Arend

Re: [PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[Julian Calaby]

Hi Arend,
On Fri, May 25, 2018 at 12:38 PM Arend van Spriel <
arend.vanspriel@broadcom.com> wrote:

> On 5/22/2018 3:18 PM, Rafa=C5=82 Mi=C5=82ecki wrote:
> > From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
> >
> > New Broadcom firmwares mark monitor mode packets using a newly defined
> > bit in the flags field. Use it to filter them out and pass to the
> > monitor interface. These defines were found in bcmmsgbuf.h from SDK.
> >
> > As not every firmware generates radiotap header this commit introduces
> > BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
> > not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 fram=
e
> > and prepends it with an empty radiotap header.
> >
> > It's limited to the msgbuf protocol. Adding support for SDIO/USB device=
s
> > will require some extra research.

> So I went looking on my shelf and found the patch I made for SDIO a
> while back. It relies on firmware change and I did not introduce a
> firmware flag for it. I will send the patch as RFT so you can have a
> look at it.

> I checked our internal driver and it turns out the raw vs. radiotap is a
> compilation option. So depending on customer they would get either
> firmware doing the radiotap header generation or the host driver,
> without any run-time way to determine it. Not nice for brcmfmac.

I pretty sure I know what the answer to this is going to be, however I
still have to ask:

Is it possible to open up _any_ part of the firmware? (Even if it's just
the host-interface end and the hardware end is a big ugly blob) It'd make
dealing with stuff like this so much easier if we can just toggle a flag
and recompile. (I'm also sure that we'd be more than happy to pass some
flag through telling us/you that it's unofficial firmware and has probably
broken the hardware, etc.)

Thanks,

--=20
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[Arend van Spriel]

On 5/27/2018 7:34 AM, Julian Calaby wrote:
> Hi Arend,
> On Fri, May 25, 2018 at 12:38 PM Arend van Spriel <
> arend.vanspriel@broadcom.com> wrote:
>
>> On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> New Broadcom firmwares mark monitor mode packets using a newly defined
>>> bit in the flags field. Use it to filter them out and pass to the
>>> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
>>>
>>> As not every firmware generates radiotap header this commit introduces
>>> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
>>> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
>>> and prepends it with an empty radiotap header.
>>>
>>> It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
>>> will require some extra research.
>
>> So I went looking on my shelf and found the patch I made for SDIO a
>> while back. It relies on firmware change and I did not introduce a
>> firmware flag for it. I will send the patch as RFT so you can have a
>> look at it.
>
>> I checked our internal driver and it turns out the raw vs. radiotap is a
>> compilation option. So depending on customer they would get either
>> firmware doing the radiotap header generation or the host driver,
>> without any run-time way to determine it. Not nice for brcmfmac.
>
> I pretty sure I know what the answer to this is going to be, however I
> still have to ask:
>
> Is it possible to open up _any_ part of the firmware? (Even if it's just
> the host-interface end and the hardware end is a big ugly blob) It'd make
> dealing with stuff like this so much easier if we can just toggle a flag
> and recompile. (I'm also sure that we'd be more than happy to pass some
> flag through telling us/you that it's unofficial firmware and has probably
> broken the hardware, etc.)

Hah. Yeah, with next to nil uncertainty I know the answer to this as 
well. If you know how long it took before we committed to supporting the 
upstream drivers. Despite my better judgement I will pass your idea, but 
it is not something that is going to fall in the timeframe that Rafał 
had in mind.

Regards,
Arend

Re: [PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[Arend van Spriel]

On 5/22/2018 3:18 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> New Broadcom firmwares mark monitor mode packets using a newly defined
> bit in the flags field. Use it to filter them out and pass to the
> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
>
> As not every firmware generates radiotap header this commit introduces
> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version. If
> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11 frame
> and prepends it with an empty radiotap header.
>
> It's limited to the msgbuf protocol. Adding support for SDIO/USB devices
> will require some extra research.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/core.c    | 24 ++++++++++++++++++++++
>   .../wireless/broadcom/brcm80211/brcmfmac/core.h    |  2 ++
>   .../wireless/broadcom/brcm80211/brcmfmac/feature.h |  6 +++++-
>   .../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c  | 17 +++++++++++++++
>   4 files changed, 48 insertions(+), 1 deletion(-)
>

[snip]

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> index d1193825e559..6e417d104b7f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
> @@ -33,6 +33,8 @@
>    * MFP: 802.11w Management Frame Protection.
>    * GSCAN: enhanced scan offload feature.
>    * FWSUP: Firmware supplicant.
> + * MON_802_11_FLAG: monitor packets flagged as 802.11
> + * MON_FMT_RADIOTAP: monitor packets include radiotap header

Hi Rafał,

I am preparing the 4366c0 release. Can you tell me which firmwares that 
you have provide 802.11 packets and/or radiotap+802.11? Doing 'strings 
fwfile | tail -1' should give me the necessary info.

Regards,
Arend

>    */
>   #define BRCMF_FEAT_LIST \
>   	BRCMF_FEAT_DEF(MBSS) \
> @@ -48,7 +50,9 @@
>   	BRCMF_FEAT_DEF(WOWL_ARP_ND) \
>   	BRCMF_FEAT_DEF(MFP) \
>   	BRCMF_FEAT_DEF(GSCAN) \
> -	BRCMF_FEAT_DEF(FWSUP)
> +	BRCMF_FEAT_DEF(FWSUP) \
> +	BRCMF_FEAT_DEF(MON_802_11_FLAG) \
> +	BRCMF_FEAT_DEF(MON_FMT_RADIOTAP)

Re: [PATCH 2/3] brcmfmac: handle monitor mode marked msgbuf packets

[Julian Calaby]

Hi Arend,

On Wed, May 30, 2018 at 8:05 PM Arend van Spriel <
arend.vanspriel@broadcom.com> wrote:

> On 5/27/2018 7:34 AM, Julian Calaby wrote:
> > Hi Arend,
> > On Fri, May 25, 2018 at 12:38 PM Arend van Spriel <
> > arend.vanspriel@broadcom.com> wrote:
> >
> >> On 5/22/2018 3:18 PM, Rafa=C5=82 Mi=C5=82ecki wrote:
> >>> From: Rafa=C5=82 Mi=C5=82ecki <rafal@milecki.pl>
> >>>
> >>> New Broadcom firmwares mark monitor mode packets using a newly define=
d
> >>> bit in the flags field. Use it to filter them out and pass to the
> >>> monitor interface. These defines were found in bcmmsgbuf.h from SDK.
> >>>
> >>> As not every firmware generates radiotap header this commit introduce=
s
> >>> BRCMF_FEAT_MON_FMT_RADIOTAP that has to be set per firmware version.
If
> >>> not present brcmf_netif_mon_rx() assumed packet being a raw 802.11
frame
> >>> and prepends it with an empty radiotap header.
> >>>
> >>> It's limited to the msgbuf protocol. Adding support for SDIO/USB
devices
> >>> will require some extra research.
> >
> >> So I went looking on my shelf and found the patch I made for SDIO a
> >> while back. It relies on firmware change and I did not introduce a
> >> firmware flag for it. I will send the patch as RFT so you can have a
> >> look at it.
> >
> >> I checked our internal driver and it turns out the raw vs. radiotap is
a
> >> compilation option. So depending on customer they would get either
> >> firmware doing the radiotap header generation or the host driver,
> >> without any run-time way to determine it. Not nice for brcmfmac.
> >
> > I pretty sure I know what the answer to this is going to be, however I
> > still have to ask:
> >
> > Is it possible to open up _any_ part of the firmware? (Even if it's jus=
t
> > the host-interface end and the hardware end is a big ugly blob) It'd
make
> > dealing with stuff like this so much easier if we can just toggle a fla=
g
> > and recompile. (I'm also sure that we'd be more than happy to pass some
> > flag through telling us/you that it's unofficial firmware and has
probably
> > broken the hardware, etc.)

> Hah. Yeah, with next to nil uncertainty I know the answer to this as
> well. If you know how long it took before we committed to supporting the
> upstream drivers. Despite my better judgement I will pass your idea, but
> it is not something that is going to fall in the timeframe that Rafa=C5=
=82
> had in mind.

Fair enough, I can't ask for more than that.

Good Luck!

--=20
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/