[PATCH] brcmfmac: allow NVRAM values to contain space and '#' chars

[PATCH] brcmfmac: allow NVRAM values to contain space and '#' chars

[Rafał Miłecki]

Both chars often require special handling (and brcmf_nvram_handle_idle
already takes care of them) but they should be allowed when parsing
entry value. Some example entries from SR400ac device NVRAM:
1:ccode=#a
wl_realmlist=mail.example.com+0+21=2,4#5,7?cisco.com+0+21=2,4#5,7

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 45d7191..64e2491 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -66,14 +66,15 @@ struct nvram_parser {
 	bool multi_dev_v2;
 };
 
-static bool is_nvram_char(char c)
+/**
+ * is_printable_char() - check if char is ASCII printable one
+ *
+ * Please note that '#' may require different handling depending on the context.
+ * It's used as comment beginning and it's not allowed in key name.
+ */
+static bool is_printable_char(char c)
 {
-	/* comment marker excluded */
-	if (c == '#')
-		return false;
-
-	/* key and value may have any other readable character */
-	return (c > 0x20 && c < 0x7f);
+	return (c >= 0x20 && c < 0x7f);
 }
 
 static bool is_whitespace(char c)
@@ -92,7 +93,7 @@ static enum nvram_parser_state brcmf_nvram_handle_idle(struct nvram_parser *nvp)
 		goto proceed;
 	if (c == '#')
 		return COMMENT;
-	if (is_nvram_char(c)) {
+	if (is_printable_char(c)) {
 		nvp->entry = nvp->pos;
 		return KEY;
 	}
@@ -120,7 +121,7 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 			nvp->multi_dev_v1 = true;
 		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
 			nvp->multi_dev_v2 = true;
-	} else if (!is_nvram_char(c)) {
+	} else if (!is_printable_char(c) || c == ' ' || c == '#') {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
 			  nvp->line, nvp->column);
 		return COMMENT;
@@ -140,7 +141,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
 	u32 cplen;
 
 	c = nvp->fwnv->data[nvp->pos];
-	if (!is_nvram_char(c)) {
+	if (!is_printable_char(c)) {
 		/* key,value pair complete */
 		ekv = (u8 *)&nvp->fwnv->data[nvp->pos];
 		skv = (u8 *)&nvp->fwnv->data[nvp->entry];
-- 
1.8.4.5

[PATCH V2] brcmfmac: allow NVRAM values to contain space and '#' chars

[Rafał Miłecki]

Both chars often require special handling (and brcmf_nvram_handle_idle
already takes care of them) but they should be allowed when parsing
entry value. Some example entries from SR400ac device NVRAM:
1:ccode=#a
wl0_version=7.14.43.16 (r)

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Minor change in commit message only. Provide an example of NVRAM entry
    using space (not just a '#'). It's unprefixed (shouldn't be uploaded to
    hardware anyway), but proves it's allowed in general.
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 45d7191..64e2491 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -66,14 +66,15 @@ struct nvram_parser {
 	bool multi_dev_v2;
 };
 
-static bool is_nvram_char(char c)
+/**
+ * is_printable_char() - check if char is ASCII printable one
+ *
+ * Please note that '#' may require different handling depending on the context.
+ * It's used as comment beginning and it's not allowed in key name.
+ */
+static bool is_printable_char(char c)
 {
-	/* comment marker excluded */
-	if (c == '#')
-		return false;
-
-	/* key and value may have any other readable character */
-	return (c > 0x20 && c < 0x7f);
+	return (c >= 0x20 && c < 0x7f);
 }
 
 static bool is_whitespace(char c)
@@ -92,7 +93,7 @@ static enum nvram_parser_state brcmf_nvram_handle_idle(struct nvram_parser *nvp)
 		goto proceed;
 	if (c == '#')
 		return COMMENT;
-	if (is_nvram_char(c)) {
+	if (is_printable_char(c)) {
 		nvp->entry = nvp->pos;
 		return KEY;
 	}
@@ -120,7 +121,7 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 			nvp->multi_dev_v1 = true;
 		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
 			nvp->multi_dev_v2 = true;
-	} else if (!is_nvram_char(c)) {
+	} else if (!is_printable_char(c) || c == ' ' || c == '#') {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
 			  nvp->line, nvp->column);
 		return COMMENT;
@@ -140,7 +141,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
 	u32 cplen;
 
 	c = nvp->fwnv->data[nvp->pos];
-	if (!is_nvram_char(c)) {
+	if (!is_printable_char(c)) {
 		/* key,value pair complete */
 		ekv = (u8 *)&nvp->fwnv->data[nvp->pos];
 		skv = (u8 *)&nvp->fwnv->data[nvp->entry];
-- 
1.8.4.5

Re: [PATCH] brcmfmac: allow NVRAM values to contain space and '#' chars

[Arend van Spriel]

On 05/20/15 20:10, Rafał Miłecki wrote:
> Both chars often require special handling (and brcmf_nvram_handle_idle
> already takes care of them) but they should be allowed when parsing
> entry value. Some example entries from SR400ac device NVRAM:
> 1:ccode=#a
> wl_realmlist=mail.example.com+0+21=2,4#5,7?cisco.com+0+21=2,4#5,7

Actually, if ccode=#a is sent to the firmware it will be ignored as it 
is an invalid value. The other entry is not intended to be sent to the 
device so it will be discarded anyway. So I don't see value in accepting 
the '#' sign.

Regards,
Arend

> Signed-off-by: Rafał Miłecki<zajec5@gmail.com>
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> index 45d7191..64e2491 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> @@ -66,14 +66,15 @@ struct nvram_parser {
>   	bool multi_dev_v2;
>   };
>
> -static bool is_nvram_char(char c)
> +/**
> + * is_printable_char() - check if char is ASCII printable one
> + *
> + * Please note that '#' may require different handling depending on the context.
> + * It's used as comment beginning and it's not allowed in key name.
> + */
> +static bool is_printable_char(char c)
>   {
> -	/* comment marker excluded */
> -	if (c == '#')
> -		return false;
> -
> -	/* key and value may have any other readable character */
> -	return (c>  0x20&&  c<  0x7f);
> +	return (c>= 0x20&&  c<  0x7f);
>   }
>
>   static bool is_whitespace(char c)
> @@ -92,7 +93,7 @@ static enum nvram_parser_state brcmf_nvram_handle_idle(struct nvram_parser *nvp)
>   		goto proceed;
>   	if (c == '#')
>   		return COMMENT;
> -	if (is_nvram_char(c)) {
> +	if (is_printable_char(c)) {
>   		nvp->entry = nvp->pos;
>   		return KEY;
>   	}
> @@ -120,7 +121,7 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
>   			nvp->multi_dev_v1 = true;
>   		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
>   			nvp->multi_dev_v2 = true;
> -	} else if (!is_nvram_char(c)) {
> +	} else if (!is_printable_char(c) || c == ' ' || c == '#') {
>   		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
>   			  nvp->line, nvp->column);
>   		return COMMENT;
> @@ -140,7 +141,7 @@ brcmf_nvram_handle_value(struct nvram_parser *nvp)
>   	u32 cplen;
>
>   	c = nvp->fwnv->data[nvp->pos];
> -	if (!is_nvram_char(c)) {
> +	if (!is_printable_char(c)) {
>   		/* key,value pair complete */
>   		ekv = (u8 *)&nvp->fwnv->data[nvp->pos];
>   		skv = (u8 *)&nvp->fwnv->data[nvp->entry];

[PATCH V3] brcmfmac: allow NVRAM values to contain spaces

[Rafał Miłecki]

Platform NVRAMs often contain values with spaces. Even if right now most
firmware-supported entries are simple values, we shouldn't reject these
with spaces. It was semi-confirmed by Broadocm in the early patch adding
support for platform NVRAMs.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Minor change in commit message only. Provide an example of NVRAM entry
    using space (not just a '#'). It's unprefixed (shouldn't be uploaded to
    hardware anyway), but proves it's allowed in general.
V3: As commented by Arend, accept space only (not a '#').
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 45d7191..baad939 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -66,6 +66,12 @@ struct nvram_parser {
 	bool multi_dev_v2;
 };
 
+/**
+ * is_nvram_char() - check if char is a valid one for NVRAM entry
+ *
+ * It accepts all printable ASCII chars except for '#' which opens a comment.
+ * Please note that ' ' (space) while accepted is not a valid key name char.
+ */
 static bool is_nvram_char(char c)
 {
 	/* comment marker excluded */
@@ -73,7 +79,7 @@ static bool is_nvram_char(char c)
 		return false;
 
 	/* key and value may have any other readable character */
-	return (c > 0x20 && c < 0x7f);
+	return (c >= 0x20 && c < 0x7f);
 }
 
 static bool is_whitespace(char c)
@@ -120,7 +126,7 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 			nvp->multi_dev_v1 = true;
 		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
 			nvp->multi_dev_v2 = true;
-	} else if (!is_nvram_char(c)) {
+	} else if (!is_nvram_char(c) || c == ' ') {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
 			  nvp->line, nvp->column);
 		return COMMENT;
-- 
1.8.4.5

Re: [PATCH V3] brcmfmac: allow NVRAM values to contain spaces

[Arend van Spriel]

On 05/22/15 23:45, Rafał Miłecki wrote:
> Platform NVRAMs often contain values with spaces. Even if right now most
> firmware-supported entries are simple values, we shouldn't reject these
> with spaces. It was semi-confirmed by Broadocm in the early patch adding
> support for platform NVRAMs.

Well, here is my full confirmation ;-) and thanks.

Acked-by: Arend van Spriel <arend@broadcom.com>
> Signed-off-by: Rafał Miłecki<zajec5@gmail.com>
> ---
> V2: Minor change in commit message only. Provide an example of NVRAM entry
>      using space (not just a '#'). It's unprefixed (shouldn't be uploaded to
>      hardware anyway), but proves it's allowed in general.
> V3: As commented by Arend, accept space only (not a '#').
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> index 45d7191..baad939 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
> @@ -66,6 +66,12 @@ struct nvram_parser {
>   	bool multi_dev_v2;
>   };
>
> +/**
> + * is_nvram_char() - check if char is a valid one for NVRAM entry
> + *
> + * It accepts all printable ASCII chars except for '#' which opens a comment.
> + * Please note that ' ' (space) while accepted is not a valid key name char.
> + */
>   static bool is_nvram_char(char c)
>   {
>   	/* comment marker excluded */
> @@ -73,7 +79,7 @@ static bool is_nvram_char(char c)
>   		return false;
>
>   	/* key and value may have any other readable character */
> -	return (c>  0x20&&  c<  0x7f);
> +	return (c>= 0x20&&  c<  0x7f);
>   }
>
>   static bool is_whitespace(char c)
> @@ -120,7 +126,7 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
>   			nvp->multi_dev_v1 = true;
>   		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
>   			nvp->multi_dev_v2 = true;
> -	} else if (!is_nvram_char(c)) {
> +	} else if (!is_nvram_char(c) || c == ' ') {
>   		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
>   			  nvp->line, nvp->column);
>   		return COMMENT;

[PATCH V4] brcmfmac: allow NVRAM values to contain spaces

[Rafał Miłecki]

Platform NVRAMs often contain values with spaces. Even if right now most
firmware-supported entries are simple values, we shouldn't reject these
with spaces. It was semi-confirmed by Broadcom in the early patch adding
support for platform NVRAMs.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
Acked-by: Arend van Spriel <arend@broadcom.com>
---
V2: Minor change in commit message only. Provide an example of NVRAM entry
    using space (not just a '#'). It's unprefixed (shouldn't be uploaded to
    hardware anyway), but proves it's allowed in general.
V3: As commented by Arend, accept space only (not a '#').
V4: Fix typo in "Broadcom" in commit message
---
 drivers/net/wireless/brcm80211/brcmfmac/firmware.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
index 45d7191..baad939 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/firmware.c
@@ -66,6 +66,12 @@ struct nvram_parser {
 	bool multi_dev_v2;
 };
 
+/**
+ * is_nvram_char() - check if char is a valid one for NVRAM entry
+ *
+ * It accepts all printable ASCII chars except for '#' which opens a comment.
+ * Please note that ' ' (space) while accepted is not a valid key name char.
+ */
 static bool is_nvram_char(char c)
 {
 	/* comment marker excluded */
@@ -73,7 +79,7 @@ static bool is_nvram_char(char c)
 		return false;
 
 	/* key and value may have any other readable character */
-	return (c > 0x20 && c < 0x7f);
+	return (c >= 0x20 && c < 0x7f);
 }
 
 static bool is_whitespace(char c)
@@ -120,7 +126,7 @@ static enum nvram_parser_state brcmf_nvram_handle_key(struct nvram_parser *nvp)
 			nvp->multi_dev_v1 = true;
 		if (strncmp(&nvp->fwnv->data[nvp->entry], "pcie/", 5) == 0)
 			nvp->multi_dev_v2 = true;
-	} else if (!is_nvram_char(c)) {
+	} else if (!is_nvram_char(c) || c == ' ') {
 		brcmf_dbg(INFO, "warning: ln=%d:col=%d: '=' expected, skip invalid key entry\n",
 			  nvp->line, nvp->column);
 		return COMMENT;
-- 
1.8.4.5

Re: [PATCH V4] brcmfmac: allow NVRAM values to contain spaces

[Kalle Valo]

Rafał Miłecki <zajec5@gmail.com> writes:

> Platform NVRAMs often contain values with spaces. Even if right now most
> firmware-supported entries are simple values, we shouldn't reject these
> with spaces. It was semi-confirmed by Broadcom in the early patch adding
> support for platform NVRAMs.
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> Acked-by: Arend van Spriel <arend@broadcom.com>

Thanks, applied manually.

-- 
Kalle Valo