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

[PATCH 6/6] 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: linux-mips@linux-mips.org
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 Kalle,

This patch relies on a change which has been submitted to the
linux-mips maintainer [1]. However, the brcmfmac code that relies on
it is under CONFIG_BCM47XX flag. Still need to know whether that
patch will be accepted or not before applying this one.

Regards,
Arend

[1] http://mid.gmane.org/1432122655-3224-1-git-send-email-arend@broadcom.com
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 107 ++++++++++++++-------
 1 file changed, 72 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 8ff31ff..bf8928d 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -19,6 +19,9 @@
 #include <linux/device.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
+#if IS_ENABLED(CONFIG_BCM47XX)
+#include <linux/bcm47xx_nvram.h>
+#endif
 
 #include "debug.h"
 #include "firmware.h"
@@ -43,7 +46,8 @@ enum nvram_parser_state {
  * struct nvram_parser - internal info for parser.
  *
  * @state: current parser state.
- * @fwnv: input buffer being parsed.
+ * @data: input buffer data pointer.
+ * @data_len : len of input buffer.
  * @nvram: output buffer with parse result.
  * @nvram_len: lenght of parse result.
  * @line: current line.
@@ -55,7 +59,8 @@ enum nvram_parser_state {
  */
 struct nvram_parser {
 	enum nvram_parser_state state;
-	const struct firmware *fwnv;
+	u8 *data;
+	u32 data_len;
 	u8 *nvram;
 	u32 nvram_len;
 	u32 line;
@@ -66,6 +71,27 @@ struct nvram_parser {
 	bool multi_dev_v2;
 };
 
+#if IS_ENABLED(CONFIG_BCM47XX)
+static char *brcmf_nvram_get_contents(size_t *nvram_size)
+{
+	return bcm47xx_nvram_get_contents(nvram_size);
+}
+
+static void brcmf_nvram_release_contents(char *nvram)
+{
+	bcm47xx_nvram_release_contents(nvram);
+}
+#else
+static char *brcmf_nvram_get_contents(size_t *nvram_size)
+{
+	return NULL;
+}
+
+static void brcmf_nvram_release_contents(char *nvram)
+{
+}
+#endif
+
 static bool is_nvram_char(char c)
 {
 	/* comment marker excluded */
@@ -85,7 +111,7 @@ static enum nvram_parser_state brcmf_nvram_handle_idle(struct nvram_parser *nvp)
 {
 	char c;
 
-	c = nvp->fwnv->data[nvp->pos];
+	c = nvp->data[nvp->pos];
 	if (c == '\n')
 		return COMMENT;
 	if (is_whitespace(c))
@@ -109,16 +135,16 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 	enum nvram_parser_state st = nvp->state;
 	char c;
 
-	c = nvp->fwnv->data[nvp->pos];
+	c = nvp->data[nvp->pos];
 	if (c == '=') {
 		/* ignore RAW1 by treating as comment */
-		if (strncmp(&nvp->fwnv->data[nvp->entry], "RAW1", 4) == 0)
+		if (strncmp(&nvp->data[nvp->entry], "RAW1", 4) == 0)
 			st = COMMENT;
 		else
 			st = VALUE;
-		if (strncmp(&nvp->fwnv->data[nvp->entry], "devpath", 7) == 0)
+		if (strncmp(&nvp->data[nvp->entry], "devpath", 7) == 0)
 			nvp->multi_dev_v1 = true;
-		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
+		if (strncmp(&nvp->data[nvp->entry], "pcie/", 5) == 0)
 			nvp->multi_dev_v2 = true;
 	} else if (!is_nvram_char(c)) {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
@@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
 	char *ekv;
 	u32 cplen;
 
-	c = nvp->fwnv->data[nvp->pos];
-	if (!is_nvram_char(c)) {
+	c = nvp->data[nvp->pos];
+	if (!is_nvram_char(c) && (c != ' ')) {
 		/* key,value pair complete */
-		ekv = (u8 *)&nvp->fwnv->data[nvp->pos];
-		skv = (u8 *)&nvp->fwnv->data[nvp->entry];
+		ekv = (u8 *)&nvp->data[nvp->pos];
+		skv = (u8 *)&nvp->data[nvp->entry];
 		cplen = ekv - skv;
 		if (nvp->nvram_len + cplen + 1 >= BRCMF_FW_MAX_NVRAM_SIZE)
 			return END;
@@ -164,7 +190,7 @@ brcmf_nvram_handle_comment(struct nvram_parser *nvp)
 {
 	char *eol, *sol;
 
-	sol = (char *)&nvp->fwnv->data[nvp->pos];
+	sol = (char *)&nvp->data[nvp->pos];
 	eol = strchr(sol, '\n');
 	if (eol == NULL)
 		return END;
@@ -191,18 +217,20 @@ static enum nvram_parser_state
 	brcmf_nvram_handle_end
 };
 
-static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
-				   const struct firmware *nv)
+static int brcmf_init_nvram_parser(struct nvram_parser *nvp, u8 *data,
+				   u32 data_len)
 {
 	size_t size;
 
 	memset(nvp, 0, sizeof(*nvp));
-	nvp->fwnv = nv;
+	nvp->data = data;
+	nvp->data_len = data_len;
+
 	/* Limit size to MAX_NVRAM_SIZE, some files contain lot of comment */
-	if (nv->size > BRCMF_FW_MAX_NVRAM_SIZE)
+	if (data_len > BRCMF_FW_MAX_NVRAM_SIZE)
 		size = BRCMF_FW_MAX_NVRAM_SIZE;
 	else
-		size = nv->size;
+		size = data_len;
 	/* Alloc for extra 0 byte + roundup by 4 + length field */
 	size += 1 + 3 + sizeof(u32);
 	nvp->nvram = kzalloc(size, GFP_KERNEL);
@@ -342,7 +370,7 @@ fail:
  * and converts newlines to NULs. Shortens buffer as needed and pads with NULs.
  * End of buffer is completed with token identifying length of buffer.
  */
-static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length,
+static void *brcmf_fw_nvram_strip(u8 *data, u32 data_len, u32 *new_length,
 				  u16 domain_nr, u16 bus_nr)
 {
 	struct nvram_parser nvp;
@@ -350,10 +378,10 @@ static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length,
 	u32 token;
 	__le32 token_le;
 
-	if (brcmf_init_nvram_parser(&nvp, nv) < 0)
+	if (brcmf_init_nvram_parser(&nvp, data, data_len) < 0)
 		return NULL;
 
-	while (nvp.pos < nv->size) {
+	while (nvp.pos < data_len) {
 		nvp.state = nv_parser_states[nvp.state](&nvp);
 		if (nvp.state == END)
 			break;
@@ -406,19 +434,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 = brcmf_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, &nvram_length,
+	if (data) {
+		nvram = brcmf_fw_nvram_strip(data, (u32)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)
+			brcmf_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;
@@ -453,15 +496,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 6/6] 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: linux-mips@linux-mips.org
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 Kalle,

This patch relies on a change which has been submitted to the
linux-mips maintainer [1]. However, the brcmfmac code that relies on
it is under CONFIG_BCM47XX flag. Still need to know whether that
patch will be accepted or not before applying this one.

Regards,
Arend

[1] http://mid.gmane.org/1432122655-3224-1-git-send-email-arend@broadcom.com
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 107 ++++++++++++++-------
 1 file changed, 72 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 8ff31ff..bf8928d 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -19,6 +19,9 @@
 #include <linux/device.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
+#if IS_ENABLED(CONFIG_BCM47XX)
+#include <linux/bcm47xx_nvram.h>
+#endif
 
 #include "debug.h"
 #include "firmware.h"
@@ -43,7 +46,8 @@ enum nvram_parser_state {
  * struct nvram_parser - internal info for parser.
  *
  * @state: current parser state.
- * @fwnv: input buffer being parsed.
+ * @data: input buffer data pointer.
+ * @data_len : len of input buffer.
  * @nvram: output buffer with parse result.
  * @nvram_len: lenght of parse result.
  * @line: current line.
@@ -55,7 +59,8 @@ enum nvram_parser_state {
  */
 struct nvram_parser {
 	enum nvram_parser_state state;
-	const struct firmware *fwnv;
+	u8 *data;
+	u32 data_len;
 	u8 *nvram;
 	u32 nvram_len;
 	u32 line;
@@ -66,6 +71,27 @@ struct nvram_parser {
 	bool multi_dev_v2;
 };
 
+#if IS_ENABLED(CONFIG_BCM47XX)
+static char *brcmf_nvram_get_contents(size_t *nvram_size)
+{
+	return bcm47xx_nvram_get_contents(nvram_size);
+}
+
+static void brcmf_nvram_release_contents(char *nvram)
+{
+	bcm47xx_nvram_release_contents(nvram);
+}
+#else
+static char *brcmf_nvram_get_contents(size_t *nvram_size)
+{
+	return NULL;
+}
+
+static void brcmf_nvram_release_contents(char *nvram)
+{
+}
+#endif
+
 static bool is_nvram_char(char c)
 {
 	/* comment marker excluded */
@@ -85,7 +111,7 @@ static enum nvram_parser_state brcmf_nvram_handle_idle(struct nvram_parser *nvp)
 {
 	char c;
 
-	c = nvp->fwnv->data[nvp->pos];
+	c = nvp->data[nvp->pos];
 	if (c == '\n')
 		return COMMENT;
 	if (is_whitespace(c))
@@ -109,16 +135,16 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 	enum nvram_parser_state st = nvp->state;
 	char c;
 
-	c = nvp->fwnv->data[nvp->pos];
+	c = nvp->data[nvp->pos];
 	if (c == '=') {
 		/* ignore RAW1 by treating as comment */
-		if (strncmp(&nvp->fwnv->data[nvp->entry], "RAW1", 4) == 0)
+		if (strncmp(&nvp->data[nvp->entry], "RAW1", 4) == 0)
 			st = COMMENT;
 		else
 			st = VALUE;
-		if (strncmp(&nvp->fwnv->data[nvp->entry], "devpath", 7) == 0)
+		if (strncmp(&nvp->data[nvp->entry], "devpath", 7) == 0)
 			nvp->multi_dev_v1 = true;
-		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
+		if (strncmp(&nvp->data[nvp->entry], "pcie/", 5) == 0)
 			nvp->multi_dev_v2 = true;
 	} else if (!is_nvram_char(c)) {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
@@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
 	char *ekv;
 	u32 cplen;
 
-	c = nvp->fwnv->data[nvp->pos];
-	if (!is_nvram_char(c)) {
+	c = nvp->data[nvp->pos];
+	if (!is_nvram_char(c) && (c != ' ')) {
 		/* key,value pair complete */
-		ekv = (u8 *)&nvp->fwnv->data[nvp->pos];
-		skv = (u8 *)&nvp->fwnv->data[nvp->entry];
+		ekv = (u8 *)&nvp->data[nvp->pos];
+		skv = (u8 *)&nvp->data[nvp->entry];
 		cplen = ekv - skv;
 		if (nvp->nvram_len + cplen + 1 >= BRCMF_FW_MAX_NVRAM_SIZE)
 			return END;
@@ -164,7 +190,7 @@ brcmf_nvram_handle_comment(struct nvram_parser *nvp)
 {
 	char *eol, *sol;
 
-	sol = (char *)&nvp->fwnv->data[nvp->pos];
+	sol = (char *)&nvp->data[nvp->pos];
 	eol = strchr(sol, '\n');
 	if (eol == NULL)
 		return END;
@@ -191,18 +217,20 @@ static enum nvram_parser_state
 	brcmf_nvram_handle_end
 };
 
-static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
-				   const struct firmware *nv)
+static int brcmf_init_nvram_parser(struct nvram_parser *nvp, u8 *data,
+				   u32 data_len)
 {
 	size_t size;
 
 	memset(nvp, 0, sizeof(*nvp));
-	nvp->fwnv = nv;
+	nvp->data = data;
+	nvp->data_len = data_len;
+
 	/* Limit size to MAX_NVRAM_SIZE, some files contain lot of comment */
-	if (nv->size > BRCMF_FW_MAX_NVRAM_SIZE)
+	if (data_len > BRCMF_FW_MAX_NVRAM_SIZE)
 		size = BRCMF_FW_MAX_NVRAM_SIZE;
 	else
-		size = nv->size;
+		size = data_len;
 	/* Alloc for extra 0 byte + roundup by 4 + length field */
 	size += 1 + 3 + sizeof(u32);
 	nvp->nvram = kzalloc(size, GFP_KERNEL);
@@ -342,7 +370,7 @@ fail:
  * and converts newlines to NULs. Shortens buffer as needed and pads with NULs.
  * End of buffer is completed with token identifying length of buffer.
  */
-static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length,
+static void *brcmf_fw_nvram_strip(u8 *data, u32 data_len, u32 *new_length,
 				  u16 domain_nr, u16 bus_nr)
 {
 	struct nvram_parser nvp;
@@ -350,10 +378,10 @@ static void *brcmf_fw_nvram_strip(const struct firmware *nv, u32 *new_length,
 	u32 token;
 	__le32 token_le;
 
-	if (brcmf_init_nvram_parser(&nvp, nv) < 0)
+	if (brcmf_init_nvram_parser(&nvp, data, data_len) < 0)
 		return NULL;
 
-	while (nvp.pos < nv->size) {
+	while (nvp.pos < data_len) {
 		nvp.state = nv_parser_states[nvp.state](&nvp);
 		if (nvp.state == END)
 			break;
@@ -406,19 +434,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 = brcmf_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, &nvram_length,
+	if (data) {
+		nvram = brcmf_fw_nvram_strip(data, (u32)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)
+			brcmf_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;
@@ -453,15 +496,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

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

[Rafał Miłecki]

On 20 May 2015 at 14:09, Arend van Spriel <arend@broadcom.com> wrote:
> 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.

FWIW it's OpenWrt :)


> This patch relies on a change which has been submitted to the
> linux-mips maintainer [1]. However, the brcmfmac code that relies on
> it is under CONFIG_BCM47XX flag. Still need to know whether that
> patch will be accepted or not before applying this one.

Yeah, lets give this patch a few days at least, so we can be sure
it'll be properly synced with MIPS work.
I'm OK with brcmfmac internal changes (you may send them now as
separated patch if you want to), but lets be careful with
bcm47xx_nvram.h part.


> @@ -19,6 +19,9 @@
>  #include <linux/device.h>
>  #include <linux/firmware.h>
>  #include <linux/module.h>
> +#if IS_ENABLED(CONFIG_BCM47XX)
> +#include <linux/bcm47xx_nvram.h>
> +#endif

This header is safe to include on any arch, as well as all functions
are. Drop the #if please.


> @@ -66,6 +71,27 @@ struct nvram_parser {
>         bool multi_dev_v2;
>  };
>
> +#if IS_ENABLED(CONFIG_BCM47XX)
> +static char *brcmf_nvram_get_contents(size_t *nvram_size)
> +{
> +       return bcm47xx_nvram_get_contents(nvram_size);
> +}
> +
> +static void brcmf_nvram_release_contents(char *nvram)
> +{
> +       bcm47xx_nvram_release_contents(nvram);
> +}
> +#else
> +static char *brcmf_nvram_get_contents(size_t *nvram_size)
> +{
> +       return NULL;
> +}
> +
> +static void brcmf_nvram_release_contents(char *nvram)
> +{
> +}
> +#endif

Everything you put in #else simply re-implements what you sent for
MIPS tree. We don't want to duplicate that code.
Also applying above code will break building wireless-drivers-next on
MIPS, we can't push this patch.

I can understand you are looking for a way to get this patch into
current -next and to somehow sync work across trees. I'm against
anything like merging MIPS tree to wireless-driver-next, but I may
have some idea.
I think the best way for achieving this is to rework your patch to
modify include/linux/bcm47xx_nvram.h. You could modify it the same way
you did in your patch for MIPS tree, except for
bcm47xx_nvram_get_contents. Don't implement this function for real (in
.c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
{
        /* TODO: Implement in .c file */
        return NULL;
}

-- 
Rafał

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

[Rafał Miłecki]

On 20 May 2015 at 16:33, Rafał Miłecki <zajec5@gmail.com> wrote:
> I think the best way for achieving this is to rework your patch to
> modify include/linux/bcm47xx_nvram.h. You could modify it the same way
> you did in your patch for MIPS tree, except for
> bcm47xx_nvram_get_contents. Don't implement this function for real (in
> .c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
> static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
> {
>         /* TODO: Implement in .c file */
>         return NULL;
> }

One more note.
This of course will lead to conflict at some point, but I believe
Linus will handle it.

-- 
Rafał

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

[Arend van Spriel]

On 05/21/15 10:28, Rafał Miłecki wrote:
> On 20 May 2015 at 16:33, Rafał Miłecki<zajec5@gmail.com>  wrote:
>> I think the best way for achieving this is to rework your patch to
>> modify include/linux/bcm47xx_nvram.h. You could modify it the same way
>> you did in your patch for MIPS tree, except for
>> bcm47xx_nvram_get_contents. Don't implement this function for real (in
>> .c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
>> static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>> {
>>          /* TODO: Implement in .c file */
>>          return NULL;
>> }
>
> One more note.
> This of course will lead to conflict at some point, but I believe
> Linus will handle it.

I prefer to avoid tricks so I will ask to drop this patch and wait for 
it to land in the next kernel, ie. 4.2, and resubmit this patch for 4.3. 
I am not in a hurry.

Regards,
Arend

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

[Arend van Spriel]

On 05/21/15 11:28, Arend van Spriel wrote:
> On 05/21/15 10:28, Rafał Miłecki wrote:
>> On 20 May 2015 at 16:33, Rafał Miłecki<zajec5@gmail.com> wrote:
>>> I think the best way for achieving this is to rework your patch to
>>> modify include/linux/bcm47xx_nvram.h. You could modify it the same way
>>> you did in your patch for MIPS tree, except for
>>> bcm47xx_nvram_get_contents. Don't implement this function for real (in
>>> .c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
>>> static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>> {
>>> /* TODO: Implement in .c file */
>>> return NULL;
>>> }
>>
>> One more note.
>> This of course will lead to conflict at some point, but I believe
>> Linus will handle it.
>
> I prefer to avoid tricks so I will ask to drop this patch and wait for
> it to land in the next kernel, ie. 4.2, and resubmit this patch for 4.3.
> I am not in a hurry.

The 'it' in 'wait for it to land' being the mips patch providing the new 
api function. I will submit a v2 for that one.

Regards,
Arend

> Regards,
> Arend

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

[Rafał Miłecki]

On 21 May 2015 at 11:28, Arend van Spriel <arend@broadcom.com> wrote:
> On 05/21/15 10:28, Rafał Miłecki wrote:
>>
>> On 20 May 2015 at 16:33, Rafał Miłecki<zajec5@gmail.com>  wrote:
>>>
>>> I think the best way for achieving this is to rework your patch to
>>> modify include/linux/bcm47xx_nvram.h. You could modify it the same way
>>> you did in your patch for MIPS tree, except for
>>> bcm47xx_nvram_get_contents. Don't implement this function for real (in
>>> .c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
>>> static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>> {
>>>          /* TODO: Implement in .c file */
>>>          return NULL;
>>> }
>>
>>
>> One more note.
>> This of course will lead to conflict at some point, but I believe
>> Linus will handle it.
>
>
> I prefer to avoid tricks so I will ask to drop this patch and wait for it to
> land in the next kernel, ie. 4.2, and resubmit this patch for 4.3. I am not
> in a hurry.

OK :)

-- 
Rafał

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

[Kalle Valo]

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

> On 05/21/15 10:28, Rafał Miłecki wrote:
>> On 20 May 2015 at 16:33, Rafał Miłecki<zajec5@gmail.com>  wrote:
>>> I think the best way for achieving this is to rework your patch to
>>> modify include/linux/bcm47xx_nvram.h. You could modify it the same way
>>> you did in your patch for MIPS tree, except for
>>> bcm47xx_nvram_get_contents. Don't implement this function for real (in
>>> .c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
>>> static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>> {
>>>          /* TODO: Implement in .c file */
>>>          return NULL;
>>> }
>>
>> One more note.
>> This of course will lead to conflict at some point, but I believe
>> Linus will handle it.
>
> I prefer to avoid tricks so I will ask to drop this patch and wait for
> it to land in the next kernel, ie. 4.2, and resubmit this patch for
> 4.3. I am not in a hurry.

Yes, please :) Let's try to avoid conflicts as much as possible.

-- 
Kalle Valo

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

[Arend van Spriel]

On 05/21/15 12:16, Kalle Valo wrote:
> Arend van Spriel<arend@broadcom.com>  writes:
>
>> On 05/21/15 10:28, Rafał Miłecki wrote:
>>> On 20 May 2015 at 16:33, Rafał Miłecki<zajec5@gmail.com>   wrote:
>>>> I think the best way for achieving this is to rework your patch to
>>>> modify include/linux/bcm47xx_nvram.h. You could modify it the same way
>>>> you did in your patch for MIPS tree, except for
>>>> bcm47xx_nvram_get_contents. Don't implement this function for real (in
>>>> .c file), but instead make in dummy inline in a bcm47xx_nvram.h like:
>>>> static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>>> {
>>>>           /* TODO: Implement in .c file */
>>>>           return NULL;
>>>> }
>>>
>>> One more note.
>>> This of course will lead to conflict at some point, but I believe
>>> Linus will handle it.
>>
>> I prefer to avoid tricks so I will ask to drop this patch and wait for
>> it to land in the next kernel, ie. 4.2, and resubmit this patch for
>> 4.3. I am not in a hurry.
>
> Yes, please :) Let's try to avoid conflicts as much as possible.

Ok. Let's make it official. Kalle, can you drop patch 6 from the series 
(if not already done so ;-) ).

Regards,
Arend

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

[Kalle Valo]

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

>>> I prefer to avoid tricks so I will ask to drop this patch and wait for
>>> it to land in the next kernel, ie. 4.2, and resubmit this patch for
>>> 4.3. I am not in a hurry.
>>
>> Yes, please :) Let's try to avoid conflicts as much as possible.
>
> Ok. Let's make it official. Kalle, can you drop patch 6 from the
> series (if not already done so ;-) ).

Ok, dropped.

-- 
Kalle Valo

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

[Rafał Miłecki]

On 20 May 2015 at 14:09, Arend van Spriel <arend@broadcom.com> wrote:
> @@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>         char *ekv;
>         u32 cplen;
>
> -       c = nvp->fwnv->data[nvp->pos];
> -       if (!is_nvram_char(c)) {
> +       c = nvp->data[nvp->pos];
> +       if (!is_nvram_char(c) && (c != ' ')) {

Don't smuggle behavior changes in patches doing something else!


> @@ -406,19 +434,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)) {

if (fw && fw->data)
will work just fine, I'm surprised checkpatch doesn't complain.

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

[Arend van Spriel]

The patch is dropped, but I still owe you a response to this. I missed 
it because you sent 2 or 3 replies confusing me.

On 05/20/15 17:02, Rafał Miłecki wrote:
> On 20 May 2015 at 14:09, Arend van Spriel<arend@broadcom.com>  wrote:
>> @@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>          char *ekv;
>>          u32 cplen;
>>
>> -       c = nvp->fwnv->data[nvp->pos];
>> -       if (!is_nvram_char(c)) {
>> +       c = nvp->data[nvp->pos];
>> +       if (!is_nvram_char(c)&&  (c != ' ')) {
>
> Don't smuggle behavior changes in patches doing something else!

The subject is "Add support for host platform NVRAM loading" and guess 
what. That type of NVRAM turned out to have spaces in the entries so in 
my opinion it is related to this patch. I can split it up if you feel 
strongly about this.

>> @@ -406,19 +434,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)) {
>
> if (fw&&  fw->data)
> will work just fine, I'm surprised checkpatch doesn't complain.

I ran checkpatch.pl --strict and did not get complaint about this change.

Regards,
Arend

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

[Rafał Miłecki]

On 22 May 2015 at 10:31, Arend van Spriel <arend@broadcom.com> wrote:
> On 05/20/15 17:02, Rafał Miłecki wrote:
>>
>> On 20 May 2015 at 14:09, Arend van Spriel<arend@broadcom.com>  wrote:
>>>
>>> @@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>>          char *ekv;
>>>          u32 cplen;
>>>
>>> -       c = nvp->fwnv->data[nvp->pos];
>>> -       if (!is_nvram_char(c)) {
>>> +       c = nvp->data[nvp->pos];
>>> +       if (!is_nvram_char(c)&&  (c != ' ')) {
>>
>>
>> Don't smuggle behavior changes in patches doing something else!
>
>
> The subject is "Add support for host platform NVRAM loading" and guess what.
> That type of NVRAM turned out to have spaces in the entries so in my opinion
> it is related to this patch. I can split it up if you feel strongly about
> this.

I'd expect such patch to just implement *loading* from different
source and nothing else. If there are additional changes needed, I
think they should go in separated patch if possible.

I noticed the same problem with parsing NVRAM values and sent
[PATCH] brcmfmac: allow NVRAM values to contain space and '#' chars
, so you should be able to drop this patch of your patch anyway.
You may give me an Ack if you have a moment :)


>>> @@ -406,19 +434,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)) {
>>
>>
>> if (fw&&  fw->data)
>> will work just fine, I'm surprised checkpatch doesn't complain.
>
> I ran checkpatch.pl --strict and did not get complaint about this change.

I know, it's weird. Maybe I'll report this an improvement idea to
checkpatch maintainer.

-- 
Rafał

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

[Arend van Spriel]

On 05/22/15 11:05, Rafał Miłecki wrote:
> On 22 May 2015 at 10:31, Arend van Spriel<arend@broadcom.com>  wrote:
>> On 05/20/15 17:02, Rafał Miłecki wrote:
>>>
>>> On 20 May 2015 at 14:09, Arend van Spriel<arend@broadcom.com>   wrote:
>>>>
>>>> @@ -139,11 +165,11 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>>>>           char *ekv;
>>>>           u32 cplen;
>>>>
>>>> -       c = nvp->fwnv->data[nvp->pos];
>>>> -       if (!is_nvram_char(c)) {
>>>> +       c = nvp->data[nvp->pos];
>>>> +       if (!is_nvram_char(c)&&   (c != ' ')) {
>>>
>>>
>>> Don't smuggle behavior changes in patches doing something else!
>>
>>
>> The subject is "Add support for host platform NVRAM loading" and guess what.
>> That type of NVRAM turned out to have spaces in the entries so in my opinion
>> it is related to this patch. I can split it up if you feel strongly about
>> this.
>
> I'd expect such patch to just implement *loading* from different
> source and nothing else. If there are additional changes needed, I
> think they should go in separated patch if possible.
>
> I noticed the same problem with parsing NVRAM values and sent
> [PATCH] brcmfmac: allow NVRAM values to contain space and '#' chars
> , so you should be able to drop this patch of your patch anyway.
> You may give me an Ack if you have a moment :)

Whoops. I did not :-p I don't want to deal with '#' in value field as it 
is either invalid or irrelevant to firmware on the device.

Regards,
Arend

>>>> @@ -406,19 +434,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)) {
>>>
>>>
>>> if (fw&&   fw->data)
>>> will work just fine, I'm surprised checkpatch doesn't complain.
>>
>> I ran checkpatch.pl --strict and did not get complaint about this change.
>
> I know, it's weird. Maybe I'll report this an improvement idea to
> checkpatch maintainer.
>