[PATCH 0/7] brcmfmac: nvram loading and code rework

[PATCH 0/7] brcmfmac: nvram loading and code rework

[Arend van Spriel]

This series comprises of following changes:
- support NVRAM loading for bcm47xx platform.
- revise announced interface combinations and validate against it.
- new debugfs entry for msgbuf protocol layer used with PCIe devices.
- couple of PCIe fixes.
- rework dealing with interface instances.

The series is intended for v4.3 kernel and applies to the master branch
of the wireless-drivers-next repository. The first patch has been compile
tested against 4.2-rc1 as it relies on change introduced in last merge
window. Assuming wireless-drivers-next/master will move to 4.2-rc1 before
applying this series.

Arend van Spriel (3):
  brcmfmac: correct interface combination info
  brcmfmac: make use of cfg80211_check_combinations()
  brcmfmac: consolidate ifp lookup in driver core

Franky Lin (2):
  brcmfmac: add debugfs entry for msgbuf statistics
  brcmfmac: block the correct flowring when backup queue overflow

Hante Meuleman (2):
  brcmfmac: Add support for host platform NVRAM loading.
  brcmfmac: Increase nr of supported flowrings.

 drivers/net/wireless/brcm80211/brcmfmac/bcdc.c     |  25 ++-
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 195 ++++++++++++++++-----
 drivers/net/wireless/brcm80211/brcmfmac/core.c     |  19 ++
 drivers/net/wireless/brcm80211/brcmfmac/core.h     |   2 +-
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c |  43 +++--
 drivers/net/wireless/brcm80211/brcmfmac/flowring.c |  48 ++---
 drivers/net/wireless/brcm80211/brcmfmac/flowring.h |  20 +--
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c   |  79 +++++++--
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h   |   2 +-
 9 files changed, 317 insertions(+), 116 deletions(-)

-- 
1.9.1

[PATCH 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Arend van Spriel]

From: Hante Meuleman <meuleman@broadcom.com>

Host platforms such as routers supported by OpenWRT can
support NVRAM reading directly from internal NVRAM store.
With this patch the nvram load routines will fall back to
this method when there is no nvram file and support is
available in the kernel.

Cc: Rafał Miłecki <zajec5@gmail.com>
Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Signed-off-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
Hi Rafał,

Hopefully I resolved the merge conflicts properly. It still works as
intended for x86. Can you verify this patch for MIPS?

Regards,
Arend
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 43 ++++++++++++++--------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 743f16b..1503937 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
+#include <linux/bcm47xx_nvram.h>
 
 #include "debug.h"
 #include "firmware.h"
@@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
 	u32 cplen;
 
 	c = nvp->data[nvp->pos];
-	if (!is_nvram_char(c)) {
+	if (!is_nvram_char(c) && (c != ' ')) {
 		/* key,value pair complete */
 		ekv = (u8 *)&nvp->data[nvp->pos];
 		skv = (u8 *)&nvp->data[nvp->entry];
@@ -207,6 +208,7 @@ static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
 
 	memset(nvp, 0, sizeof(*nvp));
 	nvp->data = data;
+
 	/* Limit size to MAX_NVRAM_SIZE, some files contain lot of comment */
 	if (data_len > BRCMF_FW_MAX_NVRAM_SIZE)
 		size = BRCMF_FW_MAX_NVRAM_SIZE;
@@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 	struct brcmf_fw *fwctx = ctx;
 	u32 nvram_length = 0;
 	void *nvram = NULL;
+	u8 *data = NULL;
+	size_t data_len;
+	bool raw_nvram;
 
 	brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
-	if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-		goto fail;
+	if ((fw) && (fw->data)) {
+		data = (u8 *)fw->data;
+		data_len = fw->size;
+		raw_nvram = false;
+	} else {
+		data = bcm47xx_nvram_get_contents(&data_len);
+		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
+			goto fail;
+		raw_nvram = true;
+	}
 
-	if (fw) {
-		nvram = brcmf_fw_nvram_strip(fw->data, fw->size, &nvram_length,
+	if (data) {
+		nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length,
 					     fwctx->domain_nr, fwctx->bus_nr);
-		release_firmware(fw);
-		if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-			goto fail;
+		if (raw_nvram)
+			bcm47xx_nvram_release_contents(data);
 	}
 
+	if (fw)
+		release_firmware(fw);
+	if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
+		goto fail;
+
 	fwctx->done(fwctx->dev, fwctx->code, nvram, nvram_length);
 	kfree(fwctx);
 	return;
@@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 	if (!ret)
 		return;
 
-	/* when nvram is optional call .done() callback here */
-	if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) {
-		fwctx->done(fwctx->dev, fw, NULL, 0);
-		kfree(fwctx);
-		return;
-	}
+	brcmf_fw_request_nvram_done(NULL, fwctx);
+	return;
 
-	/* failed nvram request */
-	release_firmware(fw);
 fail:
 	brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev));
 	device_release_driver(fwctx->dev);
-- 
1.9.1

[PATCH 2/7] brcmfmac: correct interface combination info

[Arend van Spriel]

The interface combination provided by brcmfmac did not truly reflect
the combinations supported by driver and/or firmware.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Pontus Fuchs <pontusf@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 151 +++++++++++++++------
 1 file changed, 112 insertions(+), 39 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index d86d1f1..ae54b66 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -5695,63 +5695,132 @@ brcmf_txrx_stypes[NUM_NL80211_IFTYPES] = {
 	}
 };
 
+/**
+ * brcmf_setup_ifmodes() - determine interface modes and combinations.
+ *
+ * @wiphy: wiphy object.
+ * @ifp: interface object needed for feat module api.
+ *
+ * The interface modes and combinations are determined dynamically here
+ * based on firmware functionality.
+ *
+ * no p2p and no mbss:
+ *
+ *	#STA <= 1, #AP <= 1, channels = 1, 2 total
+ *
+ * no p2p and mbss:
+ *
+ *	#STA <= 1, #AP <= 1, channels = 1, 2 total
+ *	#AP <= 4, matching BI, channels = 1, 4 total
+ *
+ * p2p, no mchan, and mbss:
+ *
+ *	#STA <= 1, #P2P-DEV <= 1, #{P2P-CL, P2P-GO} <= 1, channels = 1, 3 total
+ *	#STA <= 1, #P2P-DEV <= 1, #AP <= 1, #P2P-CL <= 1, channels = 1, 4 total
+ *	#AP <= 4, matching BI, channels = 1, 4 total
+ *
+ * p2p, mchan, and mbss:
+ *
+ *	#STA <= 1, #P2P-DEV <= 1, #{P2P-CL, P2P-GO} <= 1, channels = 2, 3 total
+ *	#STA <= 1, #P2P-DEV <= 1, #AP <= 1, #P2P-CL <= 1, channels = 1, 4 total
+ *	#AP <= 4, matching BI, channels = 1, 4 total
+ */
 static int brcmf_setup_ifmodes(struct wiphy *wiphy, struct brcmf_if *ifp)
 {
 	struct ieee80211_iface_combination *combo = NULL;
-	struct ieee80211_iface_limit *limits = NULL;
-	int i = 0, max_iface_cnt;
+	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;
+
+	mbss = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS);
+	p2p = brcmf_feat_is_enabled(ifp, BRCMF_FEAT_P2P);
 
-	combo = kzalloc(sizeof(*combo), GFP_KERNEL);
+	n_combos = 1 + !!p2p + !!mbss;
+	combo = kcalloc(n_combos, sizeof(*combo), GFP_KERNEL);
 	if (!combo)
 		goto err;
 
-	limits = kzalloc(sizeof(*limits) * 4, GFP_KERNEL);
-	if (!limits)
+	c0_limits = kcalloc(p2p ? 3 : 2, sizeof(*c0_limits), GFP_KERNEL);
+	if (!c0_limits)
 		goto err;
 
+	if (p2p) {
+		p2p_limits = kcalloc(4, sizeof(*p2p_limits), GFP_KERNEL);
+		if (!p2p_limits)
+			goto err;
+	}
+
+	if (mbss) {
+		mbss_limits = kcalloc(1, sizeof(*mbss_limits), GFP_KERNEL);
+		if (!mbss_limits)
+			goto err;
+	}
+
 	wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
 				 BIT(NL80211_IFTYPE_ADHOC) |
 				 BIT(NL80211_IFTYPE_AP);
 
-	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MCHAN))
-		combo->num_different_channels = 2;
-	else
-		combo->num_different_channels = 1;
-
-	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MBSS)) {
-		limits[i].max = 1;
-		limits[i++].types = BIT(NL80211_IFTYPE_STATION);
-		limits[i].max = 4;
-		limits[i++].types = BIT(NL80211_IFTYPE_AP);
-		max_iface_cnt = 5;
-	} else {
-		limits[i].max = 2;
-		limits[i++].types = BIT(NL80211_IFTYPE_STATION) |
-				    BIT(NL80211_IFTYPE_AP);
-		max_iface_cnt = 2;
-	}
-
-	if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_P2P)) {
+	c = 0;
+	i = 0;
+	combo[c].num_different_channels = 1;
+	c0_limits[i].max = 1;
+	c0_limits[i++].types = BIT(NL80211_IFTYPE_STATION);
+	if (p2p) {
+		if (brcmf_feat_is_enabled(ifp, BRCMF_FEAT_MCHAN))
+			combo[c].num_different_channels = 2;
 		wiphy->interface_modes |= BIT(NL80211_IFTYPE_P2P_CLIENT) |
 					  BIT(NL80211_IFTYPE_P2P_GO) |
 					  BIT(NL80211_IFTYPE_P2P_DEVICE);
-		limits[i].max = 1;
-		limits[i++].types = BIT(NL80211_IFTYPE_P2P_CLIENT) |
-				    BIT(NL80211_IFTYPE_P2P_GO);
-		limits[i].max = 1;
-		limits[i++].types = BIT(NL80211_IFTYPE_P2P_DEVICE);
-		max_iface_cnt += 2;
-	}
-	combo->max_interfaces = max_iface_cnt;
-	combo->limits = limits;
-	combo->n_limits = i;
-
+		c0_limits[i].max = 1;
+		c0_limits[i++].types = BIT(NL80211_IFTYPE_P2P_DEVICE);
+		c0_limits[i].max = 1;
+		c0_limits[i++].types = BIT(NL80211_IFTYPE_P2P_CLIENT) |
+				       BIT(NL80211_IFTYPE_P2P_GO);
+	} else {
+		c0_limits[i].max = 1;
+		c0_limits[i++].types = BIT(NL80211_IFTYPE_AP);
+	}
+	combo[c].max_interfaces = i;
+	combo[c].n_limits = i;
+	combo[c].limits = c0_limits;
+
+	if (p2p) {
+		c++;
+		i = 0;
+		combo[c].num_different_channels = 1;
+		p2p_limits[i].max = 1;
+		p2p_limits[i++].types = BIT(NL80211_IFTYPE_STATION);
+		p2p_limits[i].max = 1;
+		p2p_limits[i++].types = BIT(NL80211_IFTYPE_AP);
+		p2p_limits[i].max = 1;
+		p2p_limits[i++].types = BIT(NL80211_IFTYPE_P2P_CLIENT);
+		p2p_limits[i].max = 1;
+		p2p_limits[i++].types = BIT(NL80211_IFTYPE_P2P_DEVICE);
+		combo[c].max_interfaces = i;
+		combo[c].n_limits = i;
+		combo[c].limits = p2p_limits;
+	}
+
+	if (mbss) {
+		c++;
+		combo[c].beacon_int_infra_match = true;
+		combo[c].num_different_channels = 1;
+		mbss_limits[0].max = 4;
+		mbss_limits[0].types = BIT(NL80211_IFTYPE_AP);
+		combo[c].max_interfaces = 4;
+		combo[c].n_limits = 1;
+		combo[c].limits = mbss_limits;
+	}
+	wiphy->n_iface_combinations = n_combos;
 	wiphy->iface_combinations = combo;
-	wiphy->n_iface_combinations = 1;
 	return 0;
 
 err:
-	kfree(limits);
+	kfree(c0_limits);
+	kfree(p2p_limits);
+	kfree(mbss_limits);
 	kfree(combo);
 	return -ENOMEM;
 }
@@ -6059,11 +6128,15 @@ static void brcmf_cfg80211_reg_notifier(struct wiphy *wiphy,
 
 static void brcmf_free_wiphy(struct wiphy *wiphy)
 {
+	int i;
+
 	if (!wiphy)
 		return;
 
-	if (wiphy->iface_combinations)
-		kfree(wiphy->iface_combinations->limits);
+	if (wiphy->iface_combinations) {
+		for (i = 0; i < wiphy->n_iface_combinations; i++)
+			kfree(wiphy->iface_combinations[i].limits);
+	}
 	kfree(wiphy->iface_combinations);
 	if (wiphy->bands[IEEE80211_BAND_2GHZ]) {
 		kfree(wiphy->bands[IEEE80211_BAND_2GHZ]->channels);
-- 
1.9.1

[PATCH 3/7] brcmfmac: Increase nr of supported flowrings.

[Arend van Spriel]

From: Hante Meuleman <meuleman@broadcom.com>

Next generation devices will have firmware which will have more
than 256 flowrings. This patch increases the maximum number of
supported flowrings to 512.

Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/flowring.c | 38 ++++++++++++----------
 drivers/net/wireless/brcm80211/brcmfmac/flowring.h | 20 ++++++------
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c   | 11 +++++--
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h   |  2 +-
 4 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/flowring.c b/drivers/net/wireless/brcm80211/brcmfmac/flowring.c
index 5944063..e30f8fa 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/flowring.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/flowring.c
@@ -32,7 +32,7 @@
 #define BRCMF_FLOWRING_LOW		(BRCMF_FLOWRING_HIGH - 256)
 #define BRCMF_FLOWRING_INVALID_IFIDX	0xff
 
-#define BRCMF_FLOWRING_HASH_AP(da, fifo, ifidx) (da[5] + fifo + ifidx * 16)
+#define BRCMF_FLOWRING_HASH_AP(da, fifo, ifidx) (da[5] * 2 + fifo + ifidx * 16)
 #define BRCMF_FLOWRING_HASH_STA(fifo, ifidx) (fifo + ifidx * 16)
 
 static const u8 brcmf_flowring_prio2fifo[] = {
@@ -68,7 +68,7 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 			  u8 prio, u8 ifidx)
 {
 	struct brcmf_flowring_hash *hash;
-	u8 hash_idx;
+	u16 hash_idx;
 	u32 i;
 	bool found;
 	bool sta;
@@ -88,6 +88,7 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 	}
 	hash_idx =  sta ? BRCMF_FLOWRING_HASH_STA(fifo, ifidx) :
 			  BRCMF_FLOWRING_HASH_AP(mac, fifo, ifidx);
+	hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
 	found = false;
 	hash = flow->hash;
 	for (i = 0; i < BRCMF_FLOWRING_HASHSIZE; i++) {
@@ -98,6 +99,7 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 			break;
 		}
 		hash_idx++;
+		hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
 	}
 	if (found)
 		return hash[hash_idx].flowid;
@@ -111,7 +113,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 {
 	struct brcmf_flowring_ring *ring;
 	struct brcmf_flowring_hash *hash;
-	u8 hash_idx;
+	u16 hash_idx;
 	u32 i;
 	bool found;
 	u8 fifo;
@@ -131,6 +133,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 	}
 	hash_idx =  sta ? BRCMF_FLOWRING_HASH_STA(fifo, ifidx) :
 			  BRCMF_FLOWRING_HASH_AP(mac, fifo, ifidx);
+	hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
 	found = false;
 	hash = flow->hash;
 	for (i = 0; i < BRCMF_FLOWRING_HASHSIZE; i++) {
@@ -140,6 +143,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 			break;
 		}
 		hash_idx++;
+		hash_idx &= (BRCMF_FLOWRING_HASHSIZE - 1);
 	}
 	if (found) {
 		for (i = 0; i < flow->nrofrings; i++) {
@@ -169,7 +173,7 @@ u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 }
 
 
-u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid)
+u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u16 flowid)
 {
 	struct brcmf_flowring_ring *ring;
 
@@ -179,7 +183,7 @@ u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid)
 }
 
 
-static void brcmf_flowring_block(struct brcmf_flowring *flow, u8 flowid,
+static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid,
 				 bool blocked)
 {
 	struct brcmf_flowring_ring *ring;
@@ -224,10 +228,10 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u8 flowid,
 }
 
 
-void brcmf_flowring_delete(struct brcmf_flowring *flow, u8 flowid)
+void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid)
 {
 	struct brcmf_flowring_ring *ring;
-	u8 hash_idx;
+	u16 hash_idx;
 	struct sk_buff *skb;
 
 	ring = flow->rings[flowid];
@@ -249,7 +253,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u8 flowid)
 }
 
 
-u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u8 flowid,
+u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u16 flowid,
 			   struct sk_buff *skb)
 {
 	struct brcmf_flowring_ring *ring;
@@ -275,7 +279,7 @@ u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u8 flowid,
 }
 
 
-struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u8 flowid)
+struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u16 flowid)
 {
 	struct brcmf_flowring_ring *ring;
 	struct sk_buff *skb;
@@ -296,7 +300,7 @@ struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u8 flowid)
 }
 
 
-void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u8 flowid,
+void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u16 flowid,
 			     struct sk_buff *skb)
 {
 	struct brcmf_flowring_ring *ring;
@@ -307,7 +311,7 @@ void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u8 flowid,
 }
 
 
-u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u8 flowid)
+u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u16 flowid)
 {
 	struct brcmf_flowring_ring *ring;
 
@@ -322,7 +326,7 @@ u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u8 flowid)
 }
 
 
-void brcmf_flowring_open(struct brcmf_flowring *flow, u8 flowid)
+void brcmf_flowring_open(struct brcmf_flowring *flow, u16 flowid)
 {
 	struct brcmf_flowring_ring *ring;
 
@@ -336,10 +340,10 @@ void brcmf_flowring_open(struct brcmf_flowring *flow, u8 flowid)
 }
 
 
-u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u8 flowid)
+u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u16 flowid)
 {
 	struct brcmf_flowring_ring *ring;
-	u8 hash_idx;
+	u16 hash_idx;
 
 	ring = flow->rings[flowid];
 	hash_idx = ring->hash_id;
@@ -380,7 +384,7 @@ void brcmf_flowring_detach(struct brcmf_flowring *flow)
 	struct brcmf_pub *drvr = bus_if->drvr;
 	struct brcmf_flowring_tdls_entry *search;
 	struct brcmf_flowring_tdls_entry *remove;
-	u8 flowid;
+	u16 flowid;
 
 	for (flowid = 0; flowid < flow->nrofrings; flowid++) {
 		if (flow->rings[flowid])
@@ -404,7 +408,7 @@ void brcmf_flowring_configure_addr_mode(struct brcmf_flowring *flow, int ifidx,
 	struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev);
 	struct brcmf_pub *drvr = bus_if->drvr;
 	u32 i;
-	u8 flowid;
+	u16 flowid;
 
 	if (flow->addr_mode[ifidx] != addr_mode) {
 		for (i = 0; i < ARRAY_SIZE(flow->hash); i++) {
@@ -430,7 +434,7 @@ void brcmf_flowring_delete_peer(struct brcmf_flowring *flow, int ifidx,
 	struct brcmf_flowring_tdls_entry *prev;
 	struct brcmf_flowring_tdls_entry *search;
 	u32 i;
-	u8 flowid;
+	u16 flowid;
 	bool sta;
 
 	sta = (flow->addr_mode[ifidx] == ADDR_INDIRECT);
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/flowring.h b/drivers/net/wireless/brcm80211/brcmfmac/flowring.h
index 5551861..3a7d9c2 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/flowring.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/flowring.h
@@ -16,7 +16,7 @@
 #define BRCMFMAC_FLOWRING_H
 
 
-#define BRCMF_FLOWRING_HASHSIZE		256
+#define BRCMF_FLOWRING_HASHSIZE		512		/* has to be 2^x */
 #define BRCMF_FLOWRING_INVALID_ID	0xFFFFFFFF
 
 
@@ -24,7 +24,7 @@ struct brcmf_flowring_hash {
 	u8 mac[ETH_ALEN];
 	u8 fifo;
 	u8 ifidx;
-	u8 flowid;
+	u16 flowid;
 };
 
 enum ring_status {
@@ -61,16 +61,16 @@ u32 brcmf_flowring_lookup(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 			  u8 prio, u8 ifidx);
 u32 brcmf_flowring_create(struct brcmf_flowring *flow, u8 da[ETH_ALEN],
 			  u8 prio, u8 ifidx);
-void brcmf_flowring_delete(struct brcmf_flowring *flow, u8 flowid);
-void brcmf_flowring_open(struct brcmf_flowring *flow, u8 flowid);
-u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u8 flowid);
-u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u8 flowid,
+void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid);
+void brcmf_flowring_open(struct brcmf_flowring *flow, u16 flowid);
+u8 brcmf_flowring_tid(struct brcmf_flowring *flow, u16 flowid);
+u32 brcmf_flowring_enqueue(struct brcmf_flowring *flow, u16 flowid,
 			   struct sk_buff *skb);
-struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u8 flowid);
-void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u8 flowid,
+struct sk_buff *brcmf_flowring_dequeue(struct brcmf_flowring *flow, u16 flowid);
+void brcmf_flowring_reinsert(struct brcmf_flowring *flow, u16 flowid,
 			     struct sk_buff *skb);
-u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u8 flowid);
-u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u8 flowid);
+u32 brcmf_flowring_qlen(struct brcmf_flowring *flow, u16 flowid);
+u8 brcmf_flowring_ifidx_get(struct brcmf_flowring *flow, u16 flowid);
 struct brcmf_flowring *brcmf_flowring_attach(struct device *dev, u16 nrofrings);
 void brcmf_flowring_detach(struct brcmf_flowring *flow);
 void brcmf_flowring_configure_addr_mode(struct brcmf_flowring *flow, int ifidx,
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
index 898c380..363a31e 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
@@ -678,7 +678,7 @@ static u32 brcmf_msgbuf_flowring_create(struct brcmf_msgbuf *msgbuf, int ifidx,
 }
 
 
-static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u8 flowid)
+static void brcmf_msgbuf_txflow(struct brcmf_msgbuf *msgbuf, u16 flowid)
 {
 	struct brcmf_flowring *flow = msgbuf->flow;
 	struct brcmf_commonring *commonring;
@@ -1318,7 +1318,7 @@ int brcmf_proto_msgbuf_rx_trigger(struct device *dev)
 }
 
 
-void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u8 flowid)
+void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u16 flowid)
 {
 	struct brcmf_msgbuf *msgbuf = (struct brcmf_msgbuf *)drvr->proto->pd;
 	struct msgbuf_tx_flowring_delete_req *delete;
@@ -1369,6 +1369,13 @@ int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr)
 	u32 count;
 
 	if_msgbuf = drvr->bus_if->msgbuf;
+
+	if (if_msgbuf->nrof_flowrings >= BRCMF_FLOWRING_HASHSIZE) {
+		brcmf_err("driver not configured for this many flowrings %d\n",
+			  if_msgbuf->nrof_flowrings);
+		if_msgbuf->nrof_flowrings = BRCMF_FLOWRING_HASHSIZE - 1;
+	}
+
 	msgbuf = kzalloc(sizeof(*msgbuf), GFP_KERNEL);
 	if (!msgbuf)
 		goto fail;
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h
index 3d513e4..ee6906a 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.h
@@ -33,7 +33,7 @@
 
 
 int brcmf_proto_msgbuf_rx_trigger(struct device *dev);
-void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u8 flowid);
+void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u16 flowid);
 int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr);
 void brcmf_proto_msgbuf_detach(struct brcmf_pub *drvr);
 #else
-- 
1.9.1

[PATCH 4/7] brcmfmac: add debugfs entry for msgbuf statistics

[Arend van Spriel]

From: Franky Lin <frankyl@broadcom.com>

Expose ring buffer read/write pointers and other useful statistics
through debugfs.

Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Franky Lin <frankyl@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 56 ++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
index 363a31e..d0e1ce5 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
@@ -1360,6 +1360,60 @@ void brcmf_msgbuf_delete_flowring(struct brcmf_pub *drvr, u16 flowid)
 	}
 }
 
+#ifdef DEBUG
+static int brcmf_msgbuf_stats_read(struct seq_file *seq, void *data)
+{
+	struct brcmf_bus *bus_if = dev_get_drvdata(seq->private);
+	struct brcmf_pub *drvr = bus_if->drvr;
+	struct brcmf_msgbuf *msgbuf = (struct brcmf_msgbuf *)drvr->proto->pd;
+	struct brcmf_commonring *commonring;
+	u16 i;
+	struct brcmf_flowring_ring *ring;
+	struct brcmf_flowring_hash *hash;
+
+	commonring = msgbuf->commonrings[BRCMF_H2D_MSGRING_CONTROL_SUBMIT];
+	seq_printf(seq, "h2d_ctl_submit: rp %4u, wp %4u, depth %4u\n",
+		   commonring->r_ptr, commonring->w_ptr, commonring->depth);
+	commonring = msgbuf->commonrings[BRCMF_H2D_MSGRING_RXPOST_SUBMIT];
+	seq_printf(seq, "h2d_rx_submit:  rp %4u, wp %4u, depth %4u\n",
+		   commonring->r_ptr, commonring->w_ptr, commonring->depth);
+	commonring = msgbuf->commonrings[BRCMF_D2H_MSGRING_CONTROL_COMPLETE];
+	seq_printf(seq, "d2h_ctl_cmplt:  rp %4u, wp %4u, depth %4u\n",
+		   commonring->r_ptr, commonring->w_ptr, commonring->depth);
+	commonring = msgbuf->commonrings[BRCMF_D2H_MSGRING_TX_COMPLETE];
+	seq_printf(seq, "d2h_tx_cmplt:   rp %4u, wp %4u, depth %4u\n",
+		   commonring->r_ptr, commonring->w_ptr, commonring->depth);
+	commonring = msgbuf->commonrings[BRCMF_D2H_MSGRING_RX_COMPLETE];
+	seq_printf(seq, "d2h_rx_cmplt:   rp %4u, wp %4u, depth %4u\n",
+		   commonring->r_ptr, commonring->w_ptr, commonring->depth);
+
+	seq_printf(seq, "\nh2d_flowrings: depth %u\n",
+		   BRCMF_H2D_TXFLOWRING_MAX_ITEM);
+	seq_puts(seq, "Active flowrings:\n");
+	hash = msgbuf->flow->hash;
+	for (i = 0; i < msgbuf->flow->nrofrings; i++) {
+		if (!msgbuf->flow->rings[i])
+			continue;
+		ring = msgbuf->flow->rings[i];
+		if (ring->status != RING_OPEN)
+			continue;
+		commonring = msgbuf->flowrings[i];
+		hash = &msgbuf->flow->hash[ring->hash_id];
+		seq_printf(seq, "id %3u: rp %4u, wp %4u, qlen %4u, blocked %u\n"
+				"        ifidx %u, fifo %u, da %pM\n",
+				i, commonring->r_ptr, commonring->w_ptr,
+				skb_queue_len(&ring->skblist), ring->blocked,
+				hash->ifidx, hash->fifo, hash->mac);
+	}
+
+	return 0;
+}
+#else
+static int brcmf_msgbuf_stats_read(struct seq_file *seq, void *data)
+{
+	return 0;
+}
+#endif
 
 int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr)
 {
@@ -1467,6 +1521,8 @@ int brcmf_proto_msgbuf_attach(struct brcmf_pub *drvr)
 	spin_lock_init(&msgbuf->flowring_work_lock);
 	INIT_LIST_HEAD(&msgbuf->work_queue);
 
+	brcmf_debugfs_add_entry(drvr, "msgbuf_stats", brcmf_msgbuf_stats_read);
+
 	return 0;
 
 fail:
-- 
1.9.1

[PATCH 5/7] brcmfmac: make use of cfg80211_check_combinations()

[Arend van Spriel]

Use cfg80211_check_combinations() so we can bail out early when an
interface add or change results in an invalid combination.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c | 44 +++++++++++++++++++++-
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
index ae54b66..f9ea360 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/cfg80211.c
@@ -469,6 +469,36 @@ brcmf_find_wpsie(const u8 *parse, u32 len)
 	return NULL;
 }
 
+static int brcmf_vif_change_validate(struct brcmf_cfg80211_info *cfg,
+				     struct brcmf_cfg80211_vif *vif,
+				     enum nl80211_iftype new_type)
+{
+	int iftype_num[NUM_NL80211_IFTYPES];
+	struct brcmf_cfg80211_vif *pos;
+
+	memset(&iftype_num[0], 0, sizeof(iftype_num));
+	list_for_each_entry(pos, &cfg->vif_list, list)
+		if (pos == vif)
+			iftype_num[new_type]++;
+		else
+			iftype_num[pos->wdev.iftype]++;
+
+	return cfg80211_check_combinations(cfg->wiphy, 1, 0, iftype_num);
+}
+
+static int brcmf_vif_add_validate(struct brcmf_cfg80211_info *cfg,
+				  enum nl80211_iftype new_type)
+{
+	int iftype_num[NUM_NL80211_IFTYPES];
+	struct brcmf_cfg80211_vif *pos;
+
+	memset(&iftype_num[0], 0, sizeof(iftype_num));
+	list_for_each_entry(pos, &cfg->vif_list, list)
+		iftype_num[pos->wdev.iftype]++;
+
+	iftype_num[new_type]++;
+	return cfg80211_check_combinations(cfg->wiphy, 1, 0, iftype_num);
+}
 
 static void convert_key_from_CPU(struct brcmf_wsec_key *key,
 				 struct brcmf_wsec_key_le *key_le)
@@ -663,8 +693,14 @@ static struct wireless_dev *brcmf_cfg80211_add_iface(struct wiphy *wiphy,
 						     struct vif_params *params)
 {
 	struct wireless_dev *wdev;
+	int err;
 
 	brcmf_dbg(TRACE, "enter: %s type %d\n", name, type);
+	err = brcmf_vif_add_validate(wiphy_to_cfg(wiphy), type);
+	if (err) {
+		brcmf_err("iface validation failed: err=%d\n", err);
+		return ERR_PTR(err);
+	}
 	switch (type) {
 	case NL80211_IFTYPE_ADHOC:
 	case NL80211_IFTYPE_STATION:
@@ -823,8 +859,12 @@ brcmf_cfg80211_change_iface(struct wiphy *wiphy, struct net_device *ndev,
 	s32 ap = 0;
 	s32 err = 0;
 
-	brcmf_dbg(TRACE, "Enter, ndev=%p, type=%d\n", ndev, type);
-
+	brcmf_dbg(TRACE, "Enter, idx=%d, type=%d\n", ifp->bssidx, type);
+	err = brcmf_vif_change_validate(wiphy_to_cfg(wiphy), vif, type);
+	if (err) {
+		brcmf_err("iface validation failed: err=%d\n", err);
+		return err;
+	}
 	switch (type) {
 	case NL80211_IFTYPE_MONITOR:
 	case NL80211_IFTYPE_WDS:
-- 
1.9.1

[PATCH 6/7] brcmfmac: consolidate ifp lookup in driver core

[Arend van Spriel]

In rx path the firmware provide an interface index which is used to
map to a struct brcmf_if instance. However, this involves some trick
that is done in two places. This is changed by having driver core
providing brcmf_get_ifp() function.

Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/bcdc.c   | 25 ++++++++++--------------
 drivers/net/wireless/brcm80211/brcmfmac/core.c   | 19 ++++++++++++++++++
 drivers/net/wireless/brcm80211/brcmfmac/core.h   |  2 +-
 drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c | 12 ++----------
 4 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcdc.c b/drivers/net/wireless/brcm80211/brcmfmac/bcdc.c
index 8e0e91c..83aa6a0 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/bcdc.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/bcdc.c
@@ -276,6 +276,7 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool do_fws, u8 *ifidx,
 			 struct sk_buff *pktbuf)
 {
 	struct brcmf_proto_bcdc_header *h;
+	struct brcmf_if *ifp;
 
 	brcmf_dbg(BCDC, "Enter\n");
 
@@ -289,30 +290,21 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool do_fws, u8 *ifidx,
 	trace_brcmf_bcdchdr(pktbuf->data);
 	h = (struct brcmf_proto_bcdc_header *)(pktbuf->data);
 
-	*ifidx = BCDC_GET_IF_IDX(h);
-	if (*ifidx >= BRCMF_MAX_IFS) {
-		brcmf_err("rx data ifnum out of range (%d)\n", *ifidx);
+	ifp = brcmf_get_ifp(drvr, BCDC_GET_IF_IDX(h));
+	if (IS_ERR_OR_NULL(ifp)) {
+		brcmf_dbg(INFO, "no matching ifp found\n");
 		return -EBADE;
 	}
-	/* The ifidx is the idx to map to matching netdev/ifp. When receiving
-	 * events this is easy because it contains the bssidx which maps
-	 * 1-on-1 to the netdev/ifp. But for data frames the ifidx is rcvd.
-	 * bssidx 1 is used for p2p0 and no data can be received or
-	 * transmitted on it. Therefor bssidx is ifidx + 1 if ifidx > 0
-	 */
-	if (*ifidx)
-		(*ifidx)++;
-
 	if (((h->flags & BCDC_FLAG_VER_MASK) >> BCDC_FLAG_VER_SHIFT) !=
 	    BCDC_PROTO_VER) {
 		brcmf_err("%s: non-BCDC packet received, flags 0x%x\n",
-			  brcmf_ifname(drvr, *ifidx), h->flags);
+			  brcmf_ifname(drvr, ifp->ifidx), h->flags);
 		return -EBADE;
 	}
 
 	if (h->flags & BCDC_FLAG_SUM_GOOD) {
 		brcmf_dbg(BCDC, "%s: BDC rcv, good checksum, flags 0x%x\n",
-			  brcmf_ifname(drvr, *ifidx), h->flags);
+			  brcmf_ifname(drvr, ifp->ifidx), h->flags);
 		pktbuf->ip_summed = CHECKSUM_UNNECESSARY;
 	}
 
@@ -320,12 +312,15 @@ brcmf_proto_bcdc_hdrpull(struct brcmf_pub *drvr, bool do_fws, u8 *ifidx,
 
 	skb_pull(pktbuf, BCDC_HEADER_LEN);
 	if (do_fws)
-		brcmf_fws_hdrpull(drvr, *ifidx, h->data_offset << 2, pktbuf);
+		brcmf_fws_hdrpull(drvr, ifp->ifidx, h->data_offset << 2,
+				  pktbuf);
 	else
 		skb_pull(pktbuf, h->data_offset << 2);
 
 	if (pktbuf->len == 0)
 		return -ENODATA;
+
+	*ifidx = ifp->ifidx;
 	return 0;
 }
 
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/core.c b/drivers/net/wireless/brcm80211/brcmfmac/core.c
index fe9d3fb..1747d59 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/core.c
@@ -83,6 +83,25 @@ char *brcmf_ifname(struct brcmf_pub *drvr, int ifidx)
 	return "<if_none>";
 }
 
+struct brcmf_if *brcmf_get_ifp(struct brcmf_pub *drvr, int ifidx)
+{
+	if (ifidx < 0 || ifidx >= BRCMF_MAX_IFS) {
+		brcmf_err("ifidx %d out of range\n", ifidx);
+		return ERR_PTR(-ERANGE);
+	}
+
+	/* The ifidx is the idx to map to matching netdev/ifp. When receiving
+	 * events this is easy because it contains the bssidx which maps
+	 * 1-on-1 to the netdev/ifp. But for data frames the ifidx is rcvd.
+	 * bssidx 1 is used for p2p0 and no data can be received or
+	 * transmitted on it. Therefor bssidx is ifidx + 1 if ifidx > 0
+	 */
+	if (ifidx)
+		ifidx++;
+
+	return drvr->iflist[ifidx];
+}
+
 static void _brcmf_set_multicast_list(struct work_struct *work)
 {
 	struct brcmf_if *ifp;
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/core.h b/drivers/net/wireless/brcm80211/brcmfmac/core.h
index fd74a9c..e49c6bf 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/core.h
+++ b/drivers/net/wireless/brcm80211/brcmfmac/core.h
@@ -199,7 +199,7 @@ int brcmf_netdev_wait_pend8021x(struct brcmf_if *ifp);
 
 /* Return pointer to interface name */
 char *brcmf_ifname(struct brcmf_pub *drvr, int idx);
-
+struct brcmf_if *brcmf_get_ifp(struct brcmf_pub *drvr, int ifidx);
 int brcmf_net_attach(struct brcmf_if *ifp, bool rtnl_locked);
 struct brcmf_if *brcmf_add_if(struct brcmf_pub *drvr, s32 bssidx, s32 ifidx,
 			      char *name, u8 *mac_addr);
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
index d0e1ce5..db3d53a 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/msgbuf.c
@@ -1081,16 +1081,8 @@ brcmf_msgbuf_rx_skb(struct brcmf_msgbuf *msgbuf, struct sk_buff *skb,
 {
 	struct brcmf_if *ifp;
 
-	/* The ifidx is the idx to map to matching netdev/ifp. When receiving
-	 * events this is easy because it contains the bssidx which maps
-	 * 1-on-1 to the netdev/ifp. But for data frames the ifidx is rcvd.
-	 * bssidx 1 is used for p2p0 and no data can be received or
-	 * transmitted on it. Therefor bssidx is ifidx + 1 if ifidx > 0
-	 */
-	if (ifidx)
-		(ifidx)++;
-	ifp = msgbuf->drvr->iflist[ifidx];
-	if (!ifp || !ifp->ndev) {
+	ifp = brcmf_get_ifp(msgbuf->drvr, ifidx);
+	if (IS_ERR_OR_NULL(ifp) || !ifp->ndev) {
 		brcmf_err("Received pkt for invalid ifidx %d\n", ifidx);
 		brcmu_pkt_buf_free_skb(skb);
 		return;
-- 
1.9.1

[PATCH 7/7] brcmfmac: block the correct flowring when backup queue overflow

[Arend van Spriel]

From: Franky Lin <frankyl@broadcom.com>

brcmf_flowring_block blocks the last active flowring under the same
interface instead of the one provided by caller. This could lead to a
dead lock of netif stop if there are more than one flowring under the
interface and the traffic is high enough so brcmf_flowring_enqueue can
not unblock the ring right away.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Franky Lin <frankyl@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/flowring.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/flowring.c b/drivers/net/wireless/brcm80211/brcmfmac/flowring.c
index e30f8fa..5b7a8ee 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/flowring.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/flowring.c
@@ -198,11 +198,15 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid,
 	spin_lock_irqsave(&flow->block_lock, flags);
 
 	ring = flow->rings[flowid];
+	if (ring->blocked == blocked) {
+		spin_unlock_irqrestore(&flow->block_lock, flags);
+		return;
+	}
 	ifidx = brcmf_flowring_ifidx_get(flow, flowid);
 
 	currently_blocked = false;
 	for (i = 0; i < flow->nrofrings; i++) {
-		if (flow->rings[i]) {
+		if ((flow->rings[i]) && (i != flowid)) {
 			ring = flow->rings[i];
 			if ((ring->status == RING_OPEN) &&
 			    (brcmf_flowring_ifidx_get(flow, i) == ifidx)) {
@@ -213,8 +217,8 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid,
 			}
 		}
 	}
-	ring->blocked = blocked;
-	if (currently_blocked == blocked) {
+	flow->rings[flowid]->blocked = blocked;
+	if (currently_blocked) {
 		spin_unlock_irqrestore(&flow->block_lock, flags);
 		return;
 	}
-- 
1.9.1

Re: [PATCH 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Rafał Miłecki]

On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>         u32 cplen;
>
>         c = nvp->data[nvp->pos];
> -       if (!is_nvram_char(c)) {
> +       if (!is_nvram_char(c) && (c != ' ')) {

This is redundant, please drop this change.
See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces")


> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>         struct brcmf_fw *fwctx = ctx;
>         u32 nvram_length = 0;
>         void *nvram = NULL;
> +       u8 *data = NULL;

This can be const.


> +       size_t data_len;
> +       bool raw_nvram;
>
>         brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
> -       if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> -               goto fail;
> +       if ((fw) && (fw->data)) {

I think I was already pointing similar coding issue to you. There is
no need for these extra braces. And if they are not needed, don't use
them. There is no point in using if (((foo))) schema just because it
works. You could be confused by macros where we sometimes need tricks
like this, but this is a standard part of code.


> +               data = (u8 *)fw->data;

Don't cast to workaround const != const. You won't need casting after
making local "data" a const variable.


> +               data_len = fw->size;
> +               raw_nvram = false;
> +       } else {
> +               data = bcm47xx_nvram_get_contents(&data_len);
> +               if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> +                       goto fail;
> +               raw_nvram = true;
> +       }
>
> -       if (fw) {
> -               nvram = brcmf_fw_nvram_strip(fw->data, fw->size, &nvram_length,
> +       if (data) {
> +               nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length,
>                                              fwctx->domain_nr, fwctx->bus_nr);
> -               release_firmware(fw);
> -               if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> -                       goto fail;
> +               if (raw_nvram)
> +                       bcm47xx_nvram_release_contents(data);

This is cosmetical but maybe you could move above 2 lines next to the
release_firmware? So we have all freeing code at one please? Do you
think it would improve readability?
Nothing important thought. Feel free to ignore me here.


> @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>         if (!ret)
>                 return;
>
> -       /* when nvram is optional call .done() callback here */
> -       if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) {
> -               fwctx->done(fwctx->dev, fw, NULL, 0);
> -               kfree(fwctx);
> -               return;
> -       }
> +       brcmf_fw_request_nvram_done(NULL, fwctx);
> +       return;

It gave me a 5 minutes headache ;) Could you add a short comment why
you call _done anyway? Something like
/* Even if we failed to init user space fw request we may get a platform one */

-- 
Rafał

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Rafał Miłecki]

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

On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
> This series comprises of following changes:
> - support NVRAM loading for bcm47xx platform.
> - revise announced interface combinations and validate against it.
> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
> - couple of PCIe fixes.
> - rework dealing with interface instances.

I'm not sure which patch has caused this (I applied all of them except
for the 1st one) but now brcmfmac fails totally:
> Unable to handle kernel NULL pointer dereference at virtual address 00000014

I'm afraid I won't be able to spend any more time of this soon, so can
you try to reproduce this problem, please?

[-- Attachment #2: fatal.txt --]
[-- Type: text/plain, Size: 22827 bytes --]

[  156.060647] ------------[ cut here ]------------
[  156.065299] WARNING: CPU: 1 PID: 1171 at /home/zajec/openwrt/openwrt-15.05-arm.git/build_dir/target-arm_cortex-a9_uClibc-0.9.33.2_eabi/linux-bcm53xx/compat-wireless-2015-03-09/drivers/net/wireless/brcm80211/brcmfmac/core.c:1163 brcmf_netdev_wait_pend8021x+0xf0/0x100 )
[  156.089879] Modules linked in: pppoe ppp_async iptable_nat brcmfmac b43 pppox ppp_generic nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 mac80211 ipt_REJECT ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_id xt_conntn
[  156.155919] CPU: 1 PID: 1171 Comm: hostapd Not tainted 3.18.18 #6
[  156.161991] Backtrace: 
[  156.164457] [<c0015394>] (dump_backtrace) from [<c00155a0>] (show_stack+0x18/0x1c)
[  156.171997]  r6:bf27dec8 r5:00000009 r4:00000000 r3:00400140
[  156.177669] [<c0015588>] (show_stack) from [<c0162358>] (dump_stack+0x7c/0x98)
[  156.184881] [<c01622dc>] (dump_stack) from [<c001fd1c>] (warn_slowpath_common+0x70/0x94)
[  156.192939]  r4:00000000 r3:00000000
[  156.196522] [<c001fcac>] (warn_slowpath_common) from [<c001fde4>] (warn_slowpath_null+0x24/0x2c)
[  156.205268]  r8:00000000 r7:c68a0000 r6:c7309524 r5:c7309480 r4:00000001
[  156.212009] [<c001fdc0>] (warn_slowpath_null) from [<bf27180c>] (brcmf_netdev_wait_pend8021x+0xf0/0x100 [brcmfmac])
[  156.222427] [<bf27171c>] (brcmf_netdev_wait_pend8021x [brcmfmac]) from [<bf262284>] (brcmf_cfg80211_get_key+0x1d0/0x398 [brcmfmac])
[  156.234195]  r6:00000000 r5:c6405b9c r4:c7309480
[  156.238832] [<bf2621f0>] (brcmf_cfg80211_get_key [brcmfmac]) from [<bf2624a8>] (brcmf_cfg80211_del_key+0x5c/0x74 [brcmfmac])
[  156.250007]  r5:c7309480 r4:00000000
[  156.253637] [<bf26244c>] (brcmf_cfg80211_del_key [brcmfmac]) from [<bf164f68>] (nl80211_del_key+0xe4/0x144 [cfg80211])
[  156.264296]  r5:c7043120 r4:c7309000
[  156.267896] [<bf164e84>] (nl80211_del_key [cfg80211]) from [<c024bd78>] (genl_rcv_msg+0x25c/0x2f4)
[  156.276818]  r7:c706c040 r6:c7043114 r5:bf179834 r4:bf182198
[  156.282497] [<c024bb1c>] (genl_rcv_msg) from [<c024b248>] (netlink_rcv_skb+0x60/0xb4)
[  156.290288]  r10:c6405dd8 r9:00000000 r8:c6405d14 r7:00000030 r6:c024bb1c r5:c706c040
[  156.298124]  r4:c7043100
[  156.300662] [<c024b1e8>] (netlink_rcv_skb) from [<c024bb08>] (genl_rcv+0x28/0x3c)
[  156.308104]  r6:c68d2400 r5:c706c040 r4:c03b8f78 r3:00000001
[  156.313781] [<c024bae0>] (genl_rcv) from [<c024abbc>] (netlink_unicast+0x138/0x22c)
[  156.321406]  r5:c706c040 r4:c79efc00
[  156.324986] [<c024aa84>] (netlink_unicast) from [<c024b084>] (netlink_sendmsg+0x32c/0x394)
[  156.333219]  r9:00000030 r8:c6405f5c r7:c68d2400 r6:00000000 r5:00000000 r4:c706c040
[  156.340988] [<c024ad58>] (netlink_sendmsg) from [<c0213098>] (sock_sendmsg+0x78/0x8c)
[  156.348782]  r10:c6405e60 r9:c750b500 r8:00000000 r7:c796b0c0 r6:c6405f5c r5:00000030
[  156.356617]  r4:c750b500
[  156.359152] [<c0213020>] (sock_sendmsg) from [<c02148c4>] (___sys_sendmsg.part.30+0x18c/0x210)
[  156.367732]  r7:c6405e40 r6:00000000 r5:00000030 r4:c6405f5c
[  156.373415] [<c0214738>] (___sys_sendmsg.part.30) from [<c02158b8>] (__sys_sendmsg+0x54/0x78)
[  156.381904]  r10:00000000 r9:c6404000 r8:c0008aa4 r7:00000128 r6:bea9ba5c r5:00000000
[  156.389734]  r4:c750b500
[  156.392274] [<c0215864>] (__sys_sendmsg) from [<c02158ec>] (SyS_sendmsg+0x10/0x14)
[  156.399809]  r6:00c29418 r5:bea9ba5c r4:00c29418
[  156.404441] [<c02158dc>] (SyS_sendmsg) from [<c0008900>] (ret_fast_syscall+0x0/0x38)
[  156.412156] ---[ end trace 0910e9bfcbffa23b ]---
[  156.460639] ------------[ cut here ]------------
[  156.465280] WARNING: CPU: 1 PID: 1171 at /home/zajec/openwrt/openwrt-15.05-arm.git/build_dir/target-arm_cortex-a9_uClibc-0.9.33.2_eabi/linux-bcm53xx/compat-wireless-2015-03-09/drivers/net/wireless/brcm80211/brcmfmac/core.c:1163 brcmf_netdev_wait_pend8021x+0xf0/0x100 )
[  156.489857] Modules linked in: pppoe ppp_async iptable_nat brcmfmac b43 pppox ppp_generic nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 mac80211 ipt_REJECT ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_id xt_conntn
[  156.555892] CPU: 1 PID: 1171 Comm: hostapd Tainted: G        W      3.18.18 #6
[  156.563086] Backtrace: 
[  156.565549] [<c0015394>] (dump_backtrace) from [<c00155a0>] (show_stack+0x18/0x1c)
[  156.573091]  r6:bf27dec8 r5:00000009 r4:00000000 r3:00400140
[  156.578763] [<c0015588>] (show_stack) from [<c0162358>] (dump_stack+0x7c/0x98)
[  156.585976] [<c01622dc>] (dump_stack) from [<c001fd1c>] (warn_slowpath_common+0x70/0x94)
[  156.594040]  r4:00000000 r3:00000000
[  156.597625] [<c001fcac>] (warn_slowpath_common) from [<c001fde4>] (warn_slowpath_null+0x24/0x2c)
[  156.606372]  r8:00000000 r7:c68a0000 r6:c7309524 r5:c7309480 r4:00000001
[  156.613114] [<c001fdc0>] (warn_slowpath_null) from [<bf27180c>] (brcmf_netdev_wait_pend8021x+0xf0/0x100 [brcmfmac])
[  156.623531] [<bf27171c>] (brcmf_netdev_wait_pend8021x [brcmfmac]) from [<bf262284>] (brcmf_cfg80211_get_key+0x1d0/0x398 [brcmfmac])
[  156.635298]  r6:00000000 r5:c6405b9c r4:c7309480
[  156.639935] [<bf2621f0>] (brcmf_cfg80211_get_key [brcmfmac]) from [<bf2624a8>] (brcmf_cfg80211_del_key+0x5c/0x74 [brcmfmac])
[  156.651103]  r5:c7309480 r4:00000000
[  156.654719] [<bf26244c>] (brcmf_cfg80211_del_key [brcmfmac]) from [<bf164f68>] (nl80211_del_key+0xe4/0x144 [cfg80211])
[  156.665367]  r5:c70a6920 r4:c7309000
[  156.668965] [<bf164e84>] (nl80211_del_key [cfg80211]) from [<c024bd78>] (genl_rcv_msg+0x25c/0x2f4)
[  156.677887]  r7:c706c040 r6:c70a6914 r5:bf179834 r4:bf182198
[  156.683565] [<c024bb1c>] (genl_rcv_msg) from [<c024b248>] (netlink_rcv_skb+0x60/0xb4)
[  156.691364]  r10:c6405dd8 r9:00000000 r8:c6405d14 r7:00000030 r6:c024bb1c r5:c706c040
[  156.699193]  r4:c70a6900
[  156.701734] [<c024b1e8>] (netlink_rcv_skb) from [<c024bb08>] (genl_rcv+0x28/0x3c)
[  156.709181]  r6:c68d2400 r5:c706c040 r4:c03b8f78 r3:00000001
[  156.714857] [<c024bae0>] (genl_rcv) from [<c024abbc>] (netlink_unicast+0x138/0x22c)
[  156.722489]  r5:c706c040 r4:c79efc00
[  156.726073] [<c024aa84>] (netlink_unicast) from [<c024b084>] (netlink_sendmsg+0x32c/0x394)
[  156.734305]  r9:00000030 r8:c6405f5c r7:c68d2400 r6:00000000 r5:00000000 r4:c706c040
[  156.742076] [<c024ad58>] (netlink_sendmsg) from [<c0213098>] (sock_sendmsg+0x78/0x8c)
[  156.749867]  r10:c6405e60 r9:c750b500 r8:00000000 r7:c796b0c0 r6:c6405f5c r5:00000030
[  156.757704]  r4:c750b500
[  156.760237] [<c0213020>] (sock_sendmsg) from [<c02148c4>] (___sys_sendmsg.part.30+0x18c/0x210)
[  156.768813]  r7:c6405e40 r6:00000000 r5:00000030 r4:c6405f5c
[  156.774493] [<c0214738>] (___sys_sendmsg.part.30) from [<c02158b8>] (__sys_sendmsg+0x54/0x78)
[  156.782982]  r10:00000000 r9:c6404000 r8:c0008aa4 r7:00000128 r6:bea9ba8c r5:00000000
[  156.790818]  r4:c750b500
[  156.793352] [<c0215864>] (__sys_sendmsg) from [<c02158ec>] (SyS_sendmsg+0x10/0x14)
[  156.800901]  r6:00c29418 r5:bea9ba8c r4:00c29418
[  156.805526] [<c02158dc>] (SyS_sendmsg) from [<c0008900>] (ret_fast_syscall+0x0/0x38)
[  156.813244] ---[ end trace 0910e9bfcbffa23c ]---

root@OpenWrt:/# 
root@OpenWrt:/# [  158.480645] ------------[ cut here ]------------
[  158.485301] WARNING: CPU: 1 PID: 1171 at /home/zajec/openwrt/openwrt-15.05-arm.git/build_dir/target-arm_cortex-a9_uClibc-0.9.33.2_eabi/linux-bcm53xx/compat-wireless-2015-03-09/drivers/net/wireless/brcm80211/brcmfmac/core.c:1163 brcmf_netdev_wait_pend8021x+0xf0/0x100 )
[  158.509875] Modules linked in: pppoe ppp_async iptable_nat brcmfmac b43 pppox ppp_generic nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 mac80211 ipt_REJECT ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_id xt_conntn
[  158.575897] CPU: 1 PID: 1171 Comm: hostapd Tainted: G        W      3.18.18 #6
[  158.583092] Backtrace: 
[  158.585558] [<c0015394>] (dump_backtrace) from [<c00155a0>] (show_stack+0x18/0x1c)
[  158.593097]  r6:bf27dec8 r5:00000009 r4:00000000 r3:00400140
[  158.598770] [<c0015588>] (show_stack) from [<c0162358>] (dump_stack+0x7c/0x98)
[  158.605982] [<c01622dc>] (dump_stack) from [<c001fd1c>] (warn_slowpath_common+0x70/0x94)
[  158.614048]  r4:00000000 r3:00000000
[  158.617632] [<c001fcac>] (warn_slowpath_common) from [<c001fde4>] (warn_slowpath_null+0x24/0x2c)
[  158.626379]  r8:00000000 r7:c68a0000 r6:c7309524 r5:c7309480 r4:00000001
[  158.633121] [<c001fdc0>] (warn_slowpath_null) from [<bf27180c>] (brcmf_netdev_wait_pend8021x+0xf0/0x100 [brcmfmac])
[  158.643536] [<bf27171c>] (brcmf_netdev_wait_pend8021x [brcmfmac]) from [<bf262284>] (brcmf_cfg80211_get_key+0x1d0/0x398 [brcmfmac])
[  158.655305]  r6:00000000 r5:c6405b9c r4:c7309480
[  158.659942] [<bf2621f0>] (brcmf_cfg80211_get_key [brcmfmac]) from [<bf2624a8>] (brcmf_cfg80211_del_key+0x5c/0x74 [brcmfmac])
[  158.671108]  r5:c7309480 r4:00000000
[  158.674726] [<bf26244c>] (brcmf_cfg80211_del_key [brcmfmac]) from [<bf164f68>] (nl80211_del_key+0xe4/0x144 [cfg80211])
[  158.685381]  r5:c7043120 r4:c7309000
[  158.688981] [<bf164e84>] (nl80211_del_key [cfg80211]) from [<c024bd78>] (genl_rcv_msg+0x25c/0x2f4)
[  158.697901]  r7:c689d580 r6:c7043114 r5:bf179834 r4:bf182198
[  158.703580] [<c024bb1c>] (genl_rcv_msg) from [<c024b248>] (netlink_rcv_skb+0x60/0xb4)
[  158.711378]  r10:c6405dd8 r9:00000000 r8:c6405d14 r7:00000030 r6:c024bb1c r5:c689d580
[  158.719208]  r4:c7043100
[  158.721749] [<c024b1e8>] (netlink_rcv_skb) from [<c024bb08>] (genl_rcv+0x28/0x3c)
[  158.729196]  r6:c68d2400 r5:c689d580 r4:c03b8f78 r3:00000001
[  158.734872] [<c024bae0>] (genl_rcv) from [<c024abbc>] (netlink_unicast+0x138/0x22c)
[  158.742491]  r5:c689d580 r4:c79efc00
[  158.746070] [<c024aa84>] (netlink_unicast) from [<c024b084>] (netlink_sendmsg+0x32c/0x394)
[  158.754306]  r9:00000030 r8:c6405f5c r7:c68d2400 r6:00000000 r5:00000000 r4:c689d580
[  158.762083] [<c024ad58>] (netlink_sendmsg) from [<c0213098>] (sock_sendmsg+0x78/0x8c)
[  158.769874]  r10:c6405e60 r9:c750b500 r8:00000000 r7:c796b0c0 r6:c6405f5c r5:00000030
[  158.777710]  r4:c750b500
[  158.780244] [<c0213020>] (sock_sendmsg) from [<c02148c4>] (___sys_sendmsg.part.30+0x18c/0x210)
[  158.788820]  r7:c6405e40 r6:00000000 r5:00000030 r4:c6405f5c
[  158.794499] [<c0214738>] (___sys_sendmsg.part.30) from [<c02158b8>] (__sys_sendmsg+0x54/0x78)
[  158.802988]  r10:00000000 r9:c6404000 r8:c0008aa4 r7:00000128 r6:bea9bb0c r5:00000000
[  158.810824]  r4:c750b500
[  158.813357] [<c0215864>] (__sys_sendmsg) from [<c02158ec>] (SyS_sendmsg+0x10/0x14)
[  158.820902]  r6:00c29418 r5:bea9bb0c r4:00c29418
[  158.825534] [<c02158dc>] (SyS_sendmsg) from [<c0008900>] (ret_fast_syscall+0x0/0x38)
[  158.833256] ---[ end trace 0910e9bfcbffa23d ]---

root@OpenWrt:/# 
root@OpenWrt:/# 
root@OpenWrt:/# [  160.110638] ------------[ cut here ]------------
[  160.115291] WARNING: CPU: 1 PID: 1171 at /home/zajec/openwrt/openwrt-15.05-arm.git/build_dir/target-arm_cortex-a9_uClibc-0.9.33.2_eabi/linux-bcm53xx/compat-wireless-2015-03-09/drivers/net/wireless/brcm80211/brcmfmac/core.c:1163 brcmf_netdev_wait_pend8021x+0xf0/0x100 )
[  160.139870] Modules linked in: pppoe ppp_async iptable_nat brcmfmac b43 pppox ppp_generic nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 mac80211 ipt_REJECT ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_id xt_conntn
[  160.205902] CPU: 1 PID: 1171 Comm: hostapd Tainted: G        W      3.18.18 #6
[  160.213098] Backtrace: 
[  160.215563] [<c0015394>] (dump_backtrace) from [<c00155a0>] (show_stack+0x18/0x1c)
[  160.223103]  r6:bf27dec8 r5:00000009 r4:00000000 r3:00400140
[  160.228776] [<c0015588>] (show_stack) from [<c0162358>] (dump_stack+0x7c/0x98)
[  160.235989] [<c01622dc>] (dump_stack) from [<c001fd1c>] (warn_slowpath_common+0x70/0x94)
[  160.244053]  r4:00000000 r3:00000000
[  160.247637] [<c001fcac>] (warn_slowpath_common) from [<c001fde4>] (warn_slowpath_null+0x24/0x2c)
[  160.256384]  r8:00000000 r7:c68a0000 r6:c7309524 r5:c7309480 r4:00000001
[  160.263126] [<c001fdc0>] (warn_slowpath_null) from [<bf27180c>] (brcmf_netdev_wait_pend8021x+0xf0/0x100 [brcmfmac])
[  160.273542] [<bf27171c>] (brcmf_netdev_wait_pend8021x [brcmfmac]) from [<bf262284>] (brcmf_cfg80211_get_key+0x1d0/0x398 [brcmfmac])
[  160.285311]  r6:00000000 r5:c6405b9c r4:c7309480
[  160.289947] [<bf2621f0>] (brcmf_cfg80211_get_key [brcmfmac]) from [<bf2624a8>] (brcmf_cfg80211_del_key+0x5c/0x74 [brcmfmac])
[  160.301112]  r5:c7309480 r4:00000000
[  160.304732] [<bf26244c>] (brcmf_cfg80211_del_key [brcmfmac]) from [<bf164f68>] (nl80211_del_key+0xe4/0x144 [cfg80211])
[  160.315385]  r5:c7043120 r4:c7309000
[  160.318984] [<bf164e84>] (nl80211_del_key [cfg80211]) from [<c024bd78>] (genl_rcv_msg+0x25c/0x2f4)
[  160.327907]  r7:c7282400 r6:c7043114 r5:bf179834 r4:bf182198
[  160.333585] [<c024bb1c>] (genl_rcv_msg) from [<c024b248>] (netlink_rcv_skb+0x60/0xb4)
[  160.341383]  r10:c6405dd8 r9:00000000 r8:c6405d14 r7:00000030 r6:c024bb1c r5:c7282400
[  160.349214]  r4:c7043100
[  160.351753] [<c024b1e8>] (netlink_rcv_skb) from [<c024bb08>] (genl_rcv+0x28/0x3c)
[  160.359193]  r6:c68d2400 r5:c7282400 r4:c03b8f78 r3:00000001
[  160.364869] [<c024bae0>] (genl_rcv) from [<c024abbc>] (netlink_unicast+0x138/0x22c)
[  160.372495]  r5:c7282400 r4:c79efc00
[  160.376076] [<c024aa84>] (netlink_unicast) from [<c024b084>] (netlink_sendmsg+0x32c/0x394)
[  160.384311]  r9:00000030 r8:c6405f5c r7:c68d2400 r6:00000000 r5:00000000 r4:c7282400
[  160.392086] [<c024ad58>] (netlink_sendmsg) from [<c0213098>] (sock_sendmsg+0x78/0x8c)
[  160.399879]  r10:c6405e60 r9:c750b500 r8:00000000 r7:c796b0c0 r6:c6405f5c r5:00000030
[  160.407714]  r4:c750b500
[  160.410249] [<c0213020>] (sock_sendmsg) from [<c02148c4>] (___sys_sendmsg.part.30+0x18c/0x210)
[  160.418824]  r7:c6405e40 r6:00000000 r5:00000030 r4:c6405f5c
[  160.424504] [<c0214738>] (___sys_sendmsg.part.30) from [<c02158b8>] (__sys_sendmsg+0x54/0x78)
[  160.432993]  r10:00000000 r9:c6404000 r8:c0008aa4 r7:00000128 r6:bea9b114 r5:00000000
[  160.440840]  r4:c750b500
[  160.443372] [<c0215864>] (__sys_sendmsg) from [<c02158ec>] (SyS_sendmsg+0x10/0x14)
[  160.450914]  r6:00c29418 r5:bea9b114 r4:00c29418
[  160.455538] [<c02158dc>] (SyS_sendmsg) from [<c0008900>] (ret_fast_syscall+0x0/0x38)
[  160.463258] ---[ end trace 0910e9bfcbffa23e ]---
[  160.468655] Unable to handle kernel NULL pointer dereference at virtual address 00000014
[  160.476716] pgd = c6ae8000
[  160.479410] [00000014] *pgd=0648e831, *pte=00000000, *ppte=00000000
[  160.485670] Internal error: Oops: 17 [#1] SMP ARM
[  160.490347] Modules linked in: pppoe ppp_async iptable_nat brcmfmac b43 pppox ppp_generic nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 mac80211 ipt_REJECT ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_id xt_conntn
[  160.556330] CPU: 1 PID: 1171 Comm: hostapd Tainted: G        W      3.18.18 #6
[  160.563523] task: c796b0c0 ti: c6404000 task.ti: c6404000
[  160.568907] PC is at _raw_spin_lock_irqsave+0x1c/0x58
[  160.573944] LR is at skb_queue_tail+0x20/0x50
[  160.578282] pc : [<c0011200>]    lr : [<c021b860>]    psr: 00000093
[  160.578282] sp : c6405c00  ip : c6405c10  fp : c6405c0c
[  160.589700] r10: 00000001  r9 : c7240000  r8 : 00000000
[  160.594901] r7 : c7282400  r6 : 00000014  r5 : c7282400  r4 : 00000008
[  160.601399] r3 : 00000014  r2 : c7282400  r1 : c7282400  r0 : 00000013
[  160.607898] Flags: nzcv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
[  160.615085] Control: 10c5387d  Table: 06ae804a  DAC: 00000015
[  160.620804] Process hostapd (pid: 1171, stack limit = 0xc64041b8)
[  160.626871] Stack: (0xc6405c00 to 0xc6406000)
[  160.631214] 5c00: c6405c2c c6405c10 c021b860 c00111f0 c7300800 00000000 00000003 c7240000
[  160.639352] 5c20: c6405c4c c6405c30 bf273dc8 c021b84c c7282400 c701f940 00000003 c7282400
[  160.647490] 5c40: c6405c84 c6405c50 bf274db8 bf273da8 c6405c74 00000001 c024aa44 c7282400
[  160.655629] 5c60: c7340000 c7350000 c7309480 00000000 c7959402 c7309000 c6405cac c6405c88
[  160.663767] 5c80: bf26d0c0 bf274cb4 c7309000 c7350000 c7282400 c7282400 c7959402 c7043400
[  160.671906] 5ca0: c6405cd4 c6405cb0 bf270950 bf26d050 bf2707e0 00000001 c7282400 c7005800
[  160.680046] 5cc0: c7043400 00000000 c6405d24 c6405cd8 c022aa78 bf2707ec 000004d0 c6405d2c
[  160.688183] 5ce0: 00000000 00000000 00000000 00000000 c7282400 c7309000 c6405d24 00000001
[  160.696323] 5d00: c7282400 c7005800 c7043400 c7282400 c7309000 c7005868 c6405d5c c6405d28
[  160.704462] 5d20: c0242760 c022a834 c6404000 00000010 00000063 c7005800 c7282400 c7309000
[  160.712600] 5d40: c7043400 00000000 00000000 c7005868 c6405d9c c6405d60 c022ad74 c02426b8
[  160.720740] 5d60: c7005868 00000001 c03b10cc fffffff4 00000001 c6425800 c7282400 c7309000
[  160.728879] 5d80: 00000063 00000000 c7959540 00000002 c6405dac c6405da0 c022b02c c022ab1c
[  160.737017] 5da0: c6405e34 c6405db0 c02a28d4 c022b024 00000000 00000000 00000000 00000000
[  160.745156] 5dc0: 00000000 c6405f0c c750b200 00000000 00008e88 00000010 00000000 00000000
[  160.753294] 5de0: 00000013 00000000 00000000 00000000 c03b0000 00000000 00000000 00000000
[  160.761434] 5e00: 00000000 00000000 c6405e3c c750b200 00000063 c6405ee4 c796b0c0 bea9b044
[  160.769573] 5e20: 00000000 00000000 c6405ecc c6405e38 c0213098 c02a1d10 00000000 00000000
[  160.777711] 5e40: 00000000 c6405e70 c796b0c0 00000000 00000000 00000000 00000000 00000000
[  160.785849] 5e60: 00000000 00000000 00000000 00000000 20000013 00000000 00000000 00000063
[  160.793989] 5e80: c750b200 c6405e80 00000000 c6405ee4 00000000 00000000 c6405eb4 00000063
[  160.802128] 5ea0: 00000014 c6405f00 bea9b044 c6405f00 c750b200 00000063 00000014 c6405f00
[  160.810267] 5ec0: c6405fa4 c6405ed0 c0215558 c021302c 0000000a 00000000 00000000 00c2c980
[  160.818405] 5ee0: 00000063 c6405f00 00000014 c6405edc 00000001 00000000 00000000 00000000
[  160.826545] 5f00: 8e880011 0000000a 06000000 9bf0d678 0000bcba c00c36a8 c6405f5c c6405f28
[  160.834684] 5f20: c00a9404 c00c3864 00000020 00000000 c6405f54 c796b0c0 00000000 c03bc4e4
[  160.842823] 5f40: 00000006 c0008aa4 c6404000 00000000 c6405f6c c6405f60 c00a947c c00a9270
[  160.850962] 5f60: c6405f8c c6405f70 c0034864 c000e3c4 c706c040 c6404000 c6405fb0 c0008aa4
[  160.859101] 5f80: bea9b044 00000014 00000014 00000122 c0008aa4 c6404000 00000000 c6405fa8
[  160.867239] 5fa0: c0008900 c02154a0 bea9b044 00000014 00000008 00c2c980 00000063 00000000
[  160.875378] 5fc0: bea9b044 00000014 00000014 00000122 00c2b7d8 00000063 00c2c980 00000000
[  160.883517] 5fe0: 00000000 bea9b008 00039c7c b6e883bc 60000010 00000008 00000000 00000000
[  160.891649] Backtrace: 
[  160.894100] [<c00111e4>] (_raw_spin_lock_irqsave) from [<c021b860>] (skb_queue_tail+0x20/0x50)
[  160.902702] [<c021b840>] (skb_queue_tail) from [<bf273dc8>] (brcmf_flowring_enqueue+0x2c/0x78 [brcmfmac])
[  160.912222]  r6:c7240000 r5:00000003 r4:00000000 r3:c7300800
[  160.917907] [<bf273d9c>] (brcmf_flowring_enqueue [brcmfmac]) from [<bf274db8>] (brcmf_msgbuf_txdata+0x110/0x1c0 [brcmfmac])
[  160.928983]  r7:c7282400 r6:00000003 r5:c701f940 r4:c7282400
[  160.934666] [<bf274ca8>] (brcmf_msgbuf_txdata [brcmfmac]) from [<bf26d0c0>] (brcmf_fws_process_skb+0x7c/0x2dc [brcmfmac])
[  160.945564]  r10:c7309000 r9:c7959402 r8:00000000 r7:c7309480 r6:c7350000 r5:c7340000
[  160.953390]  r4:c7282400
[  160.955940] [<bf26d044>] (brcmf_fws_process_skb [brcmfmac]) from [<bf270950>] (brcmf_netdev_start_xmit+0x170/0x1d8 [brcmfmac])
[  160.967276]  r9:c7043400 r8:c7959402 r7:c7282400 r6:c7282400 r5:c7350000 r4:c7309000
[  160.975047] [<bf2707e0>] (brcmf_netdev_start_xmit [brcmfmac]) from [<c022aa78>] (dev_hard_start_xmit+0x250/0x2e8)
[  160.985255]  r8:00000000 r7:c7043400 r6:c7005800 r5:c7282400 r4:00000001 r3:bf2707e0
[  160.993015] [<c022a828>] (dev_hard_start_xmit) from [<c0242760>] (sch_direct_xmit+0xb4/0x1ec)
[  161.001499]  r10:c7005868 r9:c7309000 r8:c7282400 r7:c7043400 r6:c7005800 r5:c7282400
[  161.009325]  r4:00000001
[  161.011859] [<c02426ac>] (sch_direct_xmit) from [<c022ad74>] (__dev_queue_xmit+0x264/0x508)
[  161.020169]  r10:c7005868 r9:00000000 r8:00000000 r7:c7043400 r6:c7309000 r5:c7282400
[  161.027997]  r4:c7005800
[  161.030530] [<c022ab10>] (__dev_queue_xmit) from [<c022b02c>] (dev_queue_xmit+0x14/0x18)
[  161.038582]  r10:00000002 r9:c7959540 r8:00000000 r7:00000063 r6:c7309000 r5:c7282400
[  161.046410]  r4:c6425800
[  161.048951] [<c022b018>] (dev_queue_xmit) from [<c02a28d4>] (packet_sendmsg+0xbd0/0xcac)
[  161.057020] [<c02a1d04>] (packet_sendmsg) from [<c0213098>] (sock_sendmsg+0x78/0x8c)
[  161.064726]  r10:00000000 r9:00000000 r8:bea9b044 r7:c796b0c0 r6:c6405ee4 r5:00000063
[  161.072553]  r4:c750b200
[  161.075087] [<c0213020>] (sock_sendmsg) from [<c0215558>] (SyS_sendto+0xc4/0xe8)
[  161.082446]  r7:c6405f00 r6:00000014 r5:00000063 r4:c750b200
[  161.088114] [<c0215494>] (SyS_sendto) from [<c0008900>] (ret_fast_syscall+0x0/0x38)
[  161.095727]  r9:c6404000 r8:c0008aa4 r7:00000122 r6:00000014 r5:00000014 r4:bea9b044
[  161.103473] Code: e1a03000 e10f0000 f10c0080 f593f000 (e1931f9f) 
[  161.109538] ---[ end trace 0910e9bfcbffa23f ]---
[  161.114132] Kernel panic - not syncing: Fatal exception in interrupt
[  161.120461] CPU0: stopping
[  161.123168] CPU: 0 PID: 163 Comm: spi32766 Tainted: G      D W      3.18.18 #6
[  161.130354] Backtrace: 
[  161.132812] [<c0015394>] (dump_backtrace) from [<c00155a0>] (show_stack+0x18/0x1c)
[  161.140344]  r6:c039f894 r5:00000000 r4:00000000 r3:00208040
[  161.146014] [<c0015588>] (show_stack) from [<c0162358>] (dump_stack+0x7c/0x98)
[  161.153207] [<c01622dc>] (dump_stack) from [<c00171e4>] (handle_IPI+0xcc/0x168)
[  161.160474]  r4:00000000 r3:00000000
[  161.164050] [<c0017118>] (handle_IPI) from [<c000869c>] (gic_handle_irq+0x5c/0x64)
[  161.171577]  r6:c03a5478 r5:c78dfee0 r4:c8802100 r3:00000405
[  161.177244] [<c0008640>] (gic_handle_irq) from [<c0009220>] (__irq_svc+0x40/0x54)
[  161.184686] Exception stack(0xc78dfee0 to 0xc78dff28)
[  161.189720] fee0: 00000000 00000000 00000000 00000000 c7997534 c78de038 c78de000 00000001
[  161.197858] ff00: 00000000 00000000 00000000 c78dff34 c78dff38 c78dff28 c0035cdc c000e0f0
[  161.205992] ff20: 60000013 ffffffff
[  161.209463]  r6:ffffffff r5:60000013 r4:c000e0f0 r3:c0035cdc
[  161.215142] [<c000e078>] (schedule) from [<c0035cdc>] (kthread_worker_fn+0xc8/0x110)
[  161.222858] [<c0035c14>] (kthread_worker_fn) from [<c0035c00>] (kthread+0xf0/0x104)
[  161.230475]  r8:00000000 r7:c0035c14 r6:c7997534 r5:00000000 r4:c79bf180 r3:00000000
[  161.238226] [<c0035b10>] (kthread) from [<c00089a0>] (ret_from_fork+0x14/0x34)
[  161.245414]  r7:00000000 r6:00000000 r5:c0035b10 r4:c79bf180
[  161.251077] Rebooting in 3 seconds..

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Arend van Spriel]

On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>> This series comprises of following changes:
>> - support NVRAM loading for bcm47xx platform.
>> - revise announced interface combinations and validate against it.
>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>> - couple of PCIe fixes.
>> - rework dealing with interface instances.
>
> I'm not sure which patch has caused this (I applied all of them except
> for the 1st one) but now brcmfmac fails totally:
>> Unable to handle kernel NULL pointer dereference at virtual address 00000014
>
> I'm afraid I won't be able to spend any more time of this soon, so can
> you try to reproduce this problem, please?

So what is your scenario for this issue. Multiple AP interfaces?

Regards,
Arend

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Kalle Valo]

Arend van Spriel <arend@broadcom.com> writes:

> On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>> This series comprises of following changes:
>>> - support NVRAM loading for bcm47xx platform.
>>> - revise announced interface combinations and validate against it.
>>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>>> - couple of PCIe fixes.
>>> - rework dealing with interface instances.
>>
>> I'm not sure which patch has caused this (I applied all of them except
>> for the 1st one) but now brcmfmac fails totally:
>>> Unable to handle kernel NULL pointer dereference at virtual address 00000014
>>
>> I'm afraid I won't be able to spend any more time of this soon, so can
>> you try to reproduce this problem, please?
>
> So what is your scenario for this issue. Multiple AP interfaces?

Arend, what should I do with this patchset?

-- 
Kalle Valo

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Arend van Spriel]

On 07/23/2015 06:02 PM, Kalle Valo wrote:
> Arend van Spriel <arend@broadcom.com> writes:
>
>> On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>> This series comprises of following changes:
>>>> - support NVRAM loading for bcm47xx platform.
>>>> - revise announced interface combinations and validate against it.
>>>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>>>> - couple of PCIe fixes.
>>>> - rework dealing with interface instances.
>>>
>>> I'm not sure which patch has caused this (I applied all of them except
>>> for the 1st one) but now brcmfmac fails totally:
>>>> Unable to handle kernel NULL pointer dereference at virtual address 00000014
>>>
>>> I'm afraid I won't be able to spend any more time of this soon, so can
>>> you try to reproduce this problem, please?
>>
>> So what is your scenario for this issue. Multiple AP interfaces?
>
> Arend, what should I do with this patchset?
>

Let's drop the series. I will resend when getting back to work. The 
combination of my sons condition and getting a little baby sister is 
quite impacting on our personal life so professional life has to wait.

Regards,
Arend

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Kalle Valo]

Arend van Spriel <arend@broadcom.com> writes:

> On 07/23/2015 06:02 PM, Kalle Valo wrote:
>
>> Arend, what should I do with this patchset?
>>
>
> Let's drop the series. I will resend when getting back to work.

Ok, I dropped the series.

> The combination of my sons condition and getting a little baby sister
> is quite impacting on our personal life so professional life has to
> wait.

Oh, I hope your son gets better soon. But congratulations for the baby
girl!

-- 
Kalle Valo

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Rafał Miłecki]

On 20 July 2015 at 19:14, Arend van Spriel <arend@broadcom.com> wrote:
> On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
>>
>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>
>>> This series comprises of following changes:
>>> - support NVRAM loading for bcm47xx platform.
>>> - revise announced interface combinations and validate against it.
>>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>>> - couple of PCIe fixes.
>>> - rework dealing with interface instances.
>>
>>
>> I'm not sure which patch has caused this (I applied all of them except
>> for the 1st one) but now brcmfmac fails totally:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address
>>> 00000014
>>
>>
>> I'm afraid I won't be able to spend any more time of this soon, so can
>> you try to reproduce this problem, please?
>
>
> So what is your scenario for this issue. Multiple AP interfaces?

Sorry for missing this question. Yes. This is my standard OpenWrt testing:

uci set wireless.radio0.disabled=0
uci set wireless.@wifi-iface[-1].ssid="OpenWrtA"
uci set wireless.@wifi-iface[-1].encryption="psk2"
uci set wireless.@wifi-iface[-1].key="password123"
uci add wireless wifi-iface
uci set wireless.@wifi-iface[-1].device="radio0"
uci set wireless.@wifi-iface[-1].network="lan"
uci set wireless.@wifi-iface[-1].mode="ap"
uci set wireless.@wifi-iface[-1].ssid="OpenWrtB"
uci set wireless.@wifi-iface[-1].encryption="psk2"
uci set wireless.@wifi-iface[-1].key="password123"
uci add wireless wifi-iface
uci set wireless.@wifi-iface[-1].device="radio0"
uci set wireless.@wifi-iface[-1].network="lan"
uci set wireless.@wifi-iface[-1].mode="ap"
uci set wireless.@wifi-iface[-1].ssid="OpenWrtC"
uci set wireless.@wifi-iface[-1].encryption="psk2"
uci set wireless.@wifi-iface[-1].key="password123"
uci commit wireless
/etc/init.d/network reload

-- 
Rafał

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Arend van Spriel]

On 07/26/2015 05:40 PM, Rafał Miłecki wrote:
> On 20 July 2015 at 19:14, Arend van Spriel <arend@broadcom.com> wrote:
>> On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
>>>
>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>>
>>>> This series comprises of following changes:
>>>> - support NVRAM loading for bcm47xx platform.
>>>> - revise announced interface combinations and validate against it.
>>>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>>>> - couple of PCIe fixes.
>>>> - rework dealing with interface instances.
>>>
>>>
>>> I'm not sure which patch has caused this (I applied all of them except
>>> for the 1st one) but now brcmfmac fails totally:
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>> 00000014
>>>
>>>
>>> I'm afraid I won't be able to spend any more time of this soon, so can
>>> you try to reproduce this problem, please?

You did a warm reboot of the device, right? There seems to be an issue 
because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which 
makes the device unaccessible. Because on OpenWrt the firmware request 
is synchronous (OpenWrt patch on top of upstream brcmfmac) it results in 
probe failing on invalid access making the issue OpenWrt specific. Hante 
created a fix for the warm reboot issue.

Regards,
Arend

>> So what is your scenario for this issue. Multiple AP interfaces?
>
> Sorry for missing this question. Yes. This is my standard OpenWrt testing:
>
> uci set wireless.radio0.disabled=0
> uci set wireless.@wifi-iface[-1].ssid="OpenWrtA"
> uci set wireless.@wifi-iface[-1].encryption="psk2"
> uci set wireless.@wifi-iface[-1].key="password123"
> uci add wireless wifi-iface
> uci set wireless.@wifi-iface[-1].device="radio0"
> uci set wireless.@wifi-iface[-1].network="lan"
> uci set wireless.@wifi-iface[-1].mode="ap"
> uci set wireless.@wifi-iface[-1].ssid="OpenWrtB"
> uci set wireless.@wifi-iface[-1].encryption="psk2"
> uci set wireless.@wifi-iface[-1].key="password123"
> uci add wireless wifi-iface
> uci set wireless.@wifi-iface[-1].device="radio0"
> uci set wireless.@wifi-iface[-1].network="lan"
> uci set wireless.@wifi-iface[-1].mode="ap"
> uci set wireless.@wifi-iface[-1].ssid="OpenWrtC"
> uci set wireless.@wifi-iface[-1].encryption="psk2"
> uci set wireless.@wifi-iface[-1].key="password123"
> uci commit wireless
> /etc/init.d/network reload
>

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Rafał Miłecki]

On 12 August 2015 at 10:58, Arend van Spriel <arend@broadcom.com> wrote:
> On 07/26/2015 05:40 PM, Rafał Miłecki wrote:
>>
>> On 20 July 2015 at 19:14, Arend van Spriel <arend@broadcom.com> wrote:
>>>
>>> On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
>>>>
>>>>
>>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>>>
>>>>>
>>>>> This series comprises of following changes:
>>>>> - support NVRAM loading for bcm47xx platform.
>>>>> - revise announced interface combinations and validate against it.
>>>>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>>>>> - couple of PCIe fixes.
>>>>> - rework dealing with interface instances.
>>>>
>>>>
>>>>
>>>> I'm not sure which patch has caused this (I applied all of them except
>>>> for the 1st one) but now brcmfmac fails totally:
>>>>>
>>>>>
>>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>>> 00000014
>>>>
>>>>
>>>>
>>>> I'm afraid I won't be able to spend any more time of this soon, so can
>>>> you try to reproduce this problem, please?
>
>
> You did a warm reboot of the device, right? There seems to be an issue
> because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which
> makes the device unaccessible. Because on OpenWrt the firmware request is
> synchronous (OpenWrt patch on top of upstream brcmfmac) it results in probe
> failing on invalid access making the issue OpenWrt specific. Hante created a
> fix for the warm reboot issue.

I'm not sure about this, it was too long time ago. I'm coming back
home on next Tuesday, so I should be able to work on this again soon.

-- 
Rafał

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Rafał Miłecki]

On 12 August 2015 at 16:07, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 12 August 2015 at 10:58, Arend van Spriel <arend@broadcom.com> wrote:
>> On 07/26/2015 05:40 PM, Rafał Miłecki wrote:
>>>
>>> On 20 July 2015 at 19:14, Arend van Spriel <arend@broadcom.com> wrote:
>>>>
>>>> On 07/19/2015 05:05 PM, Rafał Miłecki wrote:
>>>>>
>>>>>
>>>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>>>>
>>>>>>
>>>>>> This series comprises of following changes:
>>>>>> - support NVRAM loading for bcm47xx platform.
>>>>>> - revise announced interface combinations and validate against it.
>>>>>> - new debugfs entry for msgbuf protocol layer used with PCIe devices.
>>>>>> - couple of PCIe fixes.
>>>>>> - rework dealing with interface instances.
>>>>>
>>>>>
>>>>>
>>>>> I'm not sure which patch has caused this (I applied all of them except
>>>>> for the 1st one) but now brcmfmac fails totally:
>>>>>>
>>>>>>
>>>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>>>> 00000014
>>>>>
>>>>>
>>>>>
>>>>> I'm afraid I won't be able to spend any more time of this soon, so can
>>>>> you try to reproduce this problem, please?
>>
>>
>> You did a warm reboot of the device, right? There seems to be an issue
>> because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which
>> makes the device unaccessible. Because on OpenWrt the firmware request is
>> synchronous (OpenWrt patch on top of upstream brcmfmac) it results in probe
>> failing on invalid access making the issue OpenWrt specific. Hante created a
>> fix for the warm reboot issue.
>
> I'm not sure about this, it was too long time ago. I'm coming back
> home on next Tuesday, so I should be able to work on this again soon.

I tested warm reboots today and indeed they are broken in OpenWrt.
However warm reboot attempt result in next-boot hang with:
brcmfmac: brcmf_fw_request_nvram_done: Found platform NVRAM (19204 B)
being the last message.

This warm reboot issue is something different from the regression
introduced by this patches which results in:
Unable to handle kernel NULL pointer dereference at virtual address 00000014

I'm focusing on V2 now.

-- 
Rafał

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Arend van Spriel]

subject changed to v2. So let's go over your beef.

On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>          u32 cplen;
>>
>>          c = nvp->data[nvp->pos];
>> -       if (!is_nvram_char(c)) {
>> +       if (!is_nvram_char(c) && (c != ' ')) {
>
> This is redundant, please drop this change.
> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces")

done

>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>>          struct brcmf_fw *fwctx = ctx;
>>          u32 nvram_length = 0;
>>          void *nvram = NULL;
>> +       u8 *data = NULL;
>
> This can be const.

done

>> +       size_t data_len;
>> +       bool raw_nvram;
>>
>>          brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
>> -       if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>> -               goto fail;
>> +       if ((fw) && (fw->data)) {
>
> I think I was already pointing similar coding issue to you. There is
> no need for these extra braces. And if they are not needed, don't use
> them. There is no point in using if (((foo))) schema just because it
> works. You could be confused by macros where we sometimes need tricks
> like this, but this is a standard part of code.

No confusion, just paranoid. You clearly have never been on road of 
chasing compiler issues with logical condition, but indeed it can be 
removed although checkpatch does not seem to be bothered with it. Will 
change it.

>> +               data = (u8 *)fw->data;
>
> Don't cast to workaround const != const. You won't need casting after
> making local "data" a const variable.

done

>> +               data_len = fw->size;
>> +               raw_nvram = false;
>> +       } else {
>> +               data = bcm47xx_nvram_get_contents(&data_len);
>> +               if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>> +                       goto fail;
>> +               raw_nvram = true;
>> +       }
>>
>> -       if (fw) {
>> -               nvram = brcmf_fw_nvram_strip(fw->data, fw->size, &nvram_length,
>> +       if (data) {
>> +               nvram = brcmf_fw_nvram_strip(data, data_len, &nvram_length,
>>                                               fwctx->domain_nr, fwctx->bus_nr);
>> -               release_firmware(fw);
>> -               if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>> -                       goto fail;
>> +               if (raw_nvram)
>> +                       bcm47xx_nvram_release_contents(data);
>
> This is cosmetical but maybe you could move above 2 lines next to the
> release_firmware? So we have all freeing code at one please? Do you
> think it would improve readability?
> Nothing important thought. Feel free to ignore me here.

confused! The release_firmware call is removed here, right?

>> @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>>          if (!ret)
>>                  return;
>>
>> -       /* when nvram is optional call .done() callback here */
>> -       if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) {
>> -               fwctx->done(fwctx->dev, fw, NULL, 0);
>> -               kfree(fwctx);
>> -               return;
>> -       }
>> +       brcmf_fw_request_nvram_done(NULL, fwctx);
>> +       return;
>
> It gave me a 5 minutes headache ;) Could you add a short comment why
> you call _done anyway? Something like
> /* Even if we failed to init user space fw request we may get a platform one */

For the resulting code I don't see value adding such comment. Reading 
this patch you might want Hante to explain this change, but you figured 
it out. Sorry for the headache ;-)

Regards,
Arend

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Arend van Spriel]

On 08/19/2015 11:21 PM, Arend van Spriel wrote:
> subject changed to v2. So let's go over your beef.
>
> On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>>          u32 cplen;
>>>
>>>          c = nvp->data[nvp->pos];
>>> -       if (!is_nvram_char(c)) {
>>> +       if (!is_nvram_char(c) && (c != ' ')) {
>>
>> This is redundant, please drop this change.
>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces")
>
> done
>
>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const
>>> struct firmware *fw, void *ctx)
>>>          struct brcmf_fw *fwctx = ctx;
>>>          u32 nvram_length = 0;
>>>          void *nvram = NULL;
>>> +       u8 *data = NULL;
>>
>> This can be const.
>
> done

Actually this is not done, but either way will require a cast because 
bcm47xx_nvram_release_contents expects char* so there is nothing gained. 
Unless someone will change bcm47xx_nvram_get/release_contents api to 
const char*.

Regards,
Arend

>>> +       size_t data_len;
>>> +       bool raw_nvram;
>>>
>>>          brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
>>> -       if (!fw && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>> -               goto fail;
>>> +       if ((fw) && (fw->data)) {
>>
>> I think I was already pointing similar coding issue to you. There is
>> no need for these extra braces. And if they are not needed, don't use
>> them. There is no point in using if (((foo))) schema just because it
>> works. You could be confused by macros where we sometimes need tricks
>> like this, but this is a standard part of code.
>
> No confusion, just paranoid. You clearly have never been on road of
> chasing compiler issues with logical condition, but indeed it can be
> removed although checkpatch does not seem to be bothered with it. Will
> change it.
>
>>> +               data = (u8 *)fw->data;
>>
>> Don't cast to workaround const != const. You won't need casting after
>> making local "data" a const variable.
>
> done
>
>>> +               data_len = fw->size;
>>> +               raw_nvram = false;
>>> +       } else {
>>> +               data = bcm47xx_nvram_get_contents(&data_len);
>>> +               if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>> +                       goto fail;
>>> +               raw_nvram = true;
>>> +       }
>>>
>>> -       if (fw) {
>>> -               nvram = brcmf_fw_nvram_strip(fw->data, fw->size,
>>> &nvram_length,
>>> +       if (data) {
>>> +               nvram = brcmf_fw_nvram_strip(data, data_len,
>>> &nvram_length,
>>>                                               fwctx->domain_nr,
>>> fwctx->bus_nr);
>>> -               release_firmware(fw);
>>> -               if (!nvram && !(fwctx->flags &
>>> BRCMF_FW_REQ_NV_OPTIONAL))
>>> -                       goto fail;
>>> +               if (raw_nvram)
>>> +                       bcm47xx_nvram_release_contents(data);
>>
>> This is cosmetical but maybe you could move above 2 lines next to the
>> release_firmware? So we have all freeing code at one please? Do you
>> think it would improve readability?
>> Nothing important thought. Feel free to ignore me here.
>
> confused! The release_firmware call is removed here, right?
>
>>> @@ -473,15 +490,9 @@ static void brcmf_fw_request_code_done(const
>>> struct firmware *fw, void *ctx)
>>>          if (!ret)
>>>                  return;
>>>
>>> -       /* when nvram is optional call .done() callback here */
>>> -       if (fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL) {
>>> -               fwctx->done(fwctx->dev, fw, NULL, 0);
>>> -               kfree(fwctx);
>>> -               return;
>>> -       }
>>> +       brcmf_fw_request_nvram_done(NULL, fwctx);
>>> +       return;
>>
>> It gave me a 5 minutes headache ;) Could you add a short comment why
>> you call _done anyway? Something like
>> /* Even if we failed to init user space fw request we may get a
>> platform one */
>
> For the resulting code I don't see value adding such comment. Reading
> this patch you might want Hante to explain this change, but you figured
> it out. Sorry for the headache ;-)
>
> Regards,
> Arend
>

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Rafał Miłecki]

On 12 August 2015 at 10:58, Arend van Spriel <arend@broadcom.com> wrote:
> You did a warm reboot of the device, right? There seems to be an issue
> because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which
> makes the device unaccessible. Because on OpenWrt the firmware request is
> synchronous (OpenWrt patch on top of upstream brcmfmac) it results in probe
> failing on invalid access making the issue OpenWrt specific. Hante created a
> fix for the warm reboot issue.

Hante could you share your fix?

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Arend van Spriel]

On 08/20/2015 01:06 AM, Rafał Miłecki wrote:
> On 12 August 2015 at 10:58, Arend van Spriel <arend@broadcom.com> wrote:
>> You did a warm reboot of the device, right? There seems to be an issue
>> because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which
>> makes the device unaccessible. Because on OpenWrt the firmware request is
>> synchronous (OpenWrt patch on top of upstream brcmfmac) it results in probe
>> failing on invalid access making the issue OpenWrt specific. Hante created a
>> fix for the warm reboot issue.
>
> Hante could you share your fix?

Hi Rafał,

Just sent you the patch privately.

Regards,
Arend

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Rafał Miłecki]

On 20 August 2015 at 12:23, Arend van Spriel <arend@broadcom.com> wrote:
> On 08/20/2015 01:06 AM, Rafał Miłecki wrote:
>>
>> On 12 August 2015 at 10:58, Arend van Spriel <arend@broadcom.com> wrote:
>>>
>>> You did a warm reboot of the device, right? There seems to be an issue
>>> because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which
>>> makes the device unaccessible. Because on OpenWrt the firmware request is
>>> synchronous (OpenWrt patch on top of upstream brcmfmac) it results in
>>> probe
>>> failing on invalid access making the issue OpenWrt specific. Hante
>>> created a
>>> fix for the warm reboot issue.
>>
>>
>> Hante could you share your fix?
>
>
> Hi Rafał,
>
> Just sent you the patch privately.

Seems to be working nice, thanks! I hope you'll be able to publish it soon :)

-- 
Rafał

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Rafał Miłecki]

On 19 August 2015 at 23:43, Arend van Spriel <arend@broadcom.com> wrote:
> On 08/19/2015 11:21 PM, Arend van Spriel wrote:
>>
>> subject changed to v2. So let's go over your beef.
>>
>> On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
>>>
>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>>
>>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>>>          u32 cplen;
>>>>
>>>>          c = nvp->data[nvp->pos];
>>>> -       if (!is_nvram_char(c)) {
>>>> +       if (!is_nvram_char(c) && (c != ' ')) {
>>>
>>>
>>> This is redundant, please drop this change.
>>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces")
>>
>>
>> done
>>
>>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const
>>>> struct firmware *fw, void *ctx)
>>>>          struct brcmf_fw *fwctx = ctx;
>>>>          u32 nvram_length = 0;
>>>>          void *nvram = NULL;
>>>> +       u8 *data = NULL;
>>>
>>>
>>> This can be const.
>>
>>
>> done
>
>
> Actually this is not done, but either way will require a cast because
> bcm47xx_nvram_release_contents expects char* so there is nothing gained.
> Unless someone will change bcm47xx_nvram_get/release_contents api to const
> char*.

Passing non-const pointer to function taking const one is OK. You
don't need casting, compiler won't complain about this.

On the other hand casing const pointer to the non-const one is hacky
and I believe you should avoid that.

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Rafał Miłecki]

On 19 August 2015 at 23:21, Arend van Spriel <arend@broadcom.com> wrote:
> subject changed to v2. So let's go over your beef.

I really hope none of my comment was mean or anything :)


> On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
>>
>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>> +               data_len = fw->size;
>>> +               raw_nvram = false;
>>> +       } else {
>>> +               data = bcm47xx_nvram_get_contents(&data_len);
>>> +               if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>> +                       goto fail;
>>> +               raw_nvram = true;
>>> +       }
>>>
>>> -       if (fw) {
>>> -               nvram = brcmf_fw_nvram_strip(fw->data, fw->size,
>>> &nvram_length,
>>> +       if (data) {
>>> +               nvram = brcmf_fw_nvram_strip(data, data_len,
>>> &nvram_length,
>>>                                               fwctx->domain_nr,
>>> fwctx->bus_nr);
>>> -               release_firmware(fw);
>>> -               if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>> -                       goto fail;
>>> +               if (raw_nvram)
>>> +                       bcm47xx_nvram_release_contents(data);
>>
>>
>> This is cosmetical but maybe you could move above 2 lines next to the
>> release_firmware? So we have all freeing code at one please? Do you
>> think it would improve readability?
>> Nothing important thought. Feel free to ignore me here.
>
>
> confused! The release_firmware call is removed here, right?

Yes, you removed it from the "if (data) {" condition body but also
re-added right after it. AFAIR I got an impression it may make more
sense to have something like:
if (raw_nvram)
        bcm47xx_nvram_release_contents(data);
if (fw)
        release_firmware(fw);
but you can just ignore it if it doesn't sound clear.

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Arend van Spriel]

On 08/20/2015 05:53 PM, Rafał Miłecki wrote:
> On 19 August 2015 at 23:43, Arend van Spriel <arend@broadcom.com> wrote:
>> On 08/19/2015 11:21 PM, Arend van Spriel wrote:
>>>
>>> subject changed to v2. So let's go over your beef.
>>>
>>> On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
>>>>
>>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>>>
>>>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>>>>           u32 cplen;
>>>>>
>>>>>           c = nvp->data[nvp->pos];
>>>>> -       if (!is_nvram_char(c)) {
>>>>> +       if (!is_nvram_char(c) && (c != ' ')) {
>>>>
>>>>
>>>> This is redundant, please drop this change.
>>>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces")
>>>
>>>
>>> done
>>>
>>>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const
>>>>> struct firmware *fw, void *ctx)
>>>>>           struct brcmf_fw *fwctx = ctx;
>>>>>           u32 nvram_length = 0;
>>>>>           void *nvram = NULL;
>>>>> +       u8 *data = NULL;
>>>>
>>>>
>>>> This can be const.
>>>
>>>
>>> done
>>
>>
>> Actually this is not done, but either way will require a cast because
>> bcm47xx_nvram_release_contents expects char* so there is nothing gained.
>> Unless someone will change bcm47xx_nvram_get/release_contents api to const
>> char*.
>
> Passing non-const pointer to function taking const one is OK. You
> don't need casting, compiler won't complain about this.

bcm47xx_nvram_release_contents expect a non-const pointer so the const 
data pointer needs to be cast to non-const. Which you claim is hacky.
Here is what happens when I make data pointer const:

   CC [M]  drivers/net/wireless/brcm80211/brcmfmac/firmware.o
drivers/net/wireless/brcm80211/brcmfmac/firmware.c: In function 
���brcmf_fw_request_nvram_done���:
drivers/net/wireless/brcm80211/brcmfmac/firmware.c:450:4: warning: 
passing argument 1 of ���bcm47xx_nvram_release_contents��� discards 
���const��� qualifier from pointer target type [enabled by default]
     bcm47xx_nvram_release_contents(data);
     ^
In file included from 
drivers/net/wireless/brcm80211/brcmfmac/firmware.c:22:0:
include/linux/bcm47xx_nvram.h:44:20: note: expected ���char *��� but 
argument is of type ���const u8 *���
  static inline void bcm47xx_nvram_release_contents(char *nvram)
                     ^
> On the other hand casing const pointer to the non-const one is hacky
> and I believe you should avoid that.

Either way you have to do a cast from const to non-const.

u8 *data => data = (u8 *)fw->data;
const u8 *data => bcm47xx_nvram_release_contents((char *)data);

Regards,
Arend

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Arend van Spriel]

On 08/20/2015 05:59 PM, Rafał Miłecki wrote:
> On 19 August 2015 at 23:21, Arend van Spriel <arend@broadcom.com> wrote:
>> subject changed to v2. So let's go over your beef.
>
> I really hope none of my comment was mean or anything :)
>
>
>> On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
>>>
>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>> +               data_len = fw->size;
>>>> +               raw_nvram = false;
>>>> +       } else {
>>>> +               data = bcm47xx_nvram_get_contents(&data_len);
>>>> +               if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>>> +                       goto fail;
>>>> +               raw_nvram = true;
>>>> +       }
>>>>
>>>> -       if (fw) {
>>>> -               nvram = brcmf_fw_nvram_strip(fw->data, fw->size,
>>>> &nvram_length,
>>>> +       if (data) {
>>>> +               nvram = brcmf_fw_nvram_strip(data, data_len,
>>>> &nvram_length,
>>>>                                                fwctx->domain_nr,
>>>> fwctx->bus_nr);
>>>> -               release_firmware(fw);
>>>> -               if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
>>>> -                       goto fail;
>>>> +               if (raw_nvram)
>>>> +                       bcm47xx_nvram_release_contents(data);
>>>
>>>
>>> This is cosmetical but maybe you could move above 2 lines next to the
>>> release_firmware? So we have all freeing code at one please? Do you
>>> think it would improve readability?
>>> Nothing important thought. Feel free to ignore me here.
>>
>>
>> confused! The release_firmware call is removed here, right?
>
> Yes, you removed it from the "if (data) {" condition body but also
> re-added right after it. AFAIR I got an impression it may make more
> sense to have something like:
> if (raw_nvram)
>          bcm47xx_nvram_release_contents(data);

I did not check whether bcm47xx_nvram_release_contents deals with data 
being NULL pointer. If so, I can change it.

Regards,
Arend

> if (fw)
>          release_firmware(fw);
> but you can just ignore it if it doesn't sound clear.
>

Re: [PATCH v2 1/7] brcmfmac: Add support for host platform NVRAM loading.

[Rafał Miłecki]

On 20 August 2015 at 18:06, Arend van Spriel <arend@broadcom.com> wrote:
> On 08/20/2015 05:53 PM, Rafał Miłecki wrote:
>>
>> On 19 August 2015 at 23:43, Arend van Spriel <arend@broadcom.com> wrote:
>>>
>>> On 08/19/2015 11:21 PM, Arend van Spriel wrote:
>>>>
>>>>
>>>> subject changed to v2. So let's go over your beef.
>>>>
>>>> On 07/11/2015 07:09 PM, Rafał Miłecki wrote:
>>>>>
>>>>>
>>>>> On 10 July 2015 at 20:31, Arend van Spriel <arend@broadcom.com> wrote:
>>>>>>
>>>>>>
>>>>>> @@ -146,7 +147,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>>>>>           u32 cplen;
>>>>>>
>>>>>>           c = nvp->data[nvp->pos];
>>>>>> -       if (!is_nvram_char(c)) {
>>>>>> +       if (!is_nvram_char(c) && (c != ' ')) {
>>>>>
>>>>>
>>>>>
>>>>> This is redundant, please drop this change.
>>>>> See fc23e81eb8f4 ("brcmfmac: allow NVRAM values to contain spaces")
>>>>
>>>>
>>>>
>>>> done
>>>>
>>>>>> @@ -426,19 +428,34 @@ static void brcmf_fw_request_nvram_done(const
>>>>>> struct firmware *fw, void *ctx)
>>>>>>           struct brcmf_fw *fwctx = ctx;
>>>>>>           u32 nvram_length = 0;
>>>>>>           void *nvram = NULL;
>>>>>> +       u8 *data = NULL;
>>>>>
>>>>>
>>>>>
>>>>> This can be const.
>>>>
>>>>
>>>>
>>>> done
>>>
>>>
>>>
>>> Actually this is not done, but either way will require a cast because
>>> bcm47xx_nvram_release_contents expects char* so there is nothing gained.
>>> Unless someone will change bcm47xx_nvram_get/release_contents api to
>>> const
>>> char*.
>>
>>
>> Passing non-const pointer to function taking const one is OK. You
>> don't need casting, compiler won't complain about this.
>
>
> bcm47xx_nvram_release_contents expect a non-const pointer so the const data
> pointer needs to be cast to non-const. Which you claim is hacky.
> Here is what happens when I make data pointer const:
>
>   CC [M]  drivers/net/wireless/brcm80211/brcmfmac/firmware.o
> drivers/net/wireless/brcm80211/brcmfmac/firmware.c: In function
> ���brcmf_fw_request_nvram_done���:
> drivers/net/wireless/brcm80211/brcmfmac/firmware.c:450:4: warning: passing
> argument 1 of ���bcm47xx_nvram_release_contents��� discards ���const���
> qualifier from pointer target type [enabled by default]
>     bcm47xx_nvram_release_contents(data);
>     ^
> In file included from
> drivers/net/wireless/brcm80211/brcmfmac/firmware.c:22:0:
> include/linux/bcm47xx_nvram.h:44:20: note: expected ���char *��� but
> argument is of type ���const u8 *���
>  static inline void bcm47xx_nvram_release_contents(char *nvram)
>                     ^
>>
>> On the other hand casing const pointer to the non-const one is hacky
>> and I believe you should avoid that.
>
>
> Either way you have to do a cast from const to non-const.
>
> u8 *data => data = (u8 *)fw->data;
> const u8 *data => bcm47xx_nvram_release_contents((char *)data);

Oh, I feel silly. Yeah, you're right. In OpenWrt I was using two
separated variables and it was what basically let it work. Just ignore
that noise from me :|

-- 
Rafał

Re: [PATCH 0/7] brcmfmac: nvram loading and code rework

[Arend van Spriel]

On 08/20/2015 05:51 PM, Rafał Miłecki wrote:
> On 20 August 2015 at 12:23, Arend van Spriel <arend@broadcom.com> wrote:
>> On 08/20/2015 01:06 AM, Rafał Miłecki wrote:
>>>
>>> On 12 August 2015 at 10:58, Arend van Spriel <arend@broadcom.com> wrote:
>>>>
>>>> You did a warm reboot of the device, right? There seems to be an issue
>>>> because on OpenWrt the brcmfmac is not unloaded upon warm reboot, which
>>>> makes the device unaccessible. Because on OpenWrt the firmware request is
>>>> synchronous (OpenWrt patch on top of upstream brcmfmac) it results in
>>>> probe
>>>> failing on invalid access making the issue OpenWrt specific. Hante
>>>> created a
>>>> fix for the warm reboot issue.
>>>
>>>
>>> Hante could you share your fix?
>>
>>
>> Hi Rafał,
>>
>> Just sent you the patch privately.
>
> Seems to be working nice, thanks! I hope you'll be able to publish it soon :)

Not before monday and I suspect a merge window coming up, but I might be 
just paranoid ;-)

Regards,
Arend