[PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[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.
The brcmfmac for one requires the complete nvram contents
to select what needs to be sent to wireless device.

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>
---
Change log:
-----------
V2:	- correct header length for nvram_find_and_copy().
	- get rid of 'NVRAM_SPACE - 2' magic.
---
 arch/mips/bcm47xx/nvram.c     | 61 ++++++++++++++++++++++++++++++++-----------
 include/linux/bcm47xx_nvram.h | 15 +++++++++++
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
index 95d028c..f4f62d3 100644
--- a/arch/mips/bcm47xx/nvram.c
+++ b/arch/mips/bcm47xx/nvram.c
@@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
 	return -ENXIO;
 
 found:
-	if (header->len > size)
-		pr_err("The nvram size accoridng to the header seems to be bigger than the partition on flash\n");
-	if (header->len > NVRAM_SPACE)
-		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
-		       header->len, NVRAM_SPACE - 1);
-
 	src = (u32 *)header;
 	dst = (u32 *)nvram_buf;
 	for (i = 0; i < sizeof(struct nvram_header); i += 4)
 		*dst++ = __raw_readl(src++);
-	for (; i < header->len && i < NVRAM_SPACE && i < size; i += 4)
+	header = (struct nvram_header *)nvram_buf;
+	if (header->len > size) {
+		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
+		header->len = size;
+	}
+	if (header->len >= NVRAM_SPACE) {
+		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
+		       header->len, NVRAM_SPACE - 1);
+		header->len = NVRAM_SPACE - 1;
+	}
+	/* proceed reading data after header */
+	for (; i < header->len; i += 4)
 		*dst++ = readl(src++);
 	nvram_buf[NVRAM_SPACE - 1] = '\0';
 
@@ -139,6 +144,7 @@ static int nvram_init(void)
 #ifdef CONFIG_MTD
 	struct mtd_info *mtd;
 	struct nvram_header header;
+	struct nvram_header *pheader;
 	size_t bytes_read;
 	int err;
 
@@ -147,20 +153,21 @@ static int nvram_init(void)
 		return -ENODEV;
 
 	err = mtd_read(mtd, 0, sizeof(header), &bytes_read, (uint8_t *)&header);
-	if (!err && header.magic == NVRAM_MAGIC) {
-		u8 *dst = (uint8_t *)nvram_buf;
-		size_t len = header.len;
-
-		if (len >= NVRAM_SPACE) {
-			len = NVRAM_SPACE - 1;
+	if (!err && header.magic == NVRAM_MAGIC &&
+	    header.len > sizeof(header)) {
+		if (header.len >= NVRAM_SPACE) {
 			pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
-				header.len, len);
+				header.len, NVRAM_SPACE);
+			header.len = NVRAM_SPACE - 1;
 		}
 
-		err = mtd_read(mtd, 0, len, &bytes_read, dst);
+		err = mtd_read(mtd, 0, header.len, &bytes_read,
+			       (u8 *)nvram_buf);
 		if (err)
 			return err;
 
+		pheader = (struct nvram_header *)nvram_buf;
+		pheader->len = header.len;
 		return 0;
 	}
 #endif
@@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
 	return -ENOENT;
 }
 EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
+
+char *bcm47xx_nvram_get_contents(size_t *nvram_size)
+{
+	int err;
+	char *nvram;
+	struct nvram_header *header;
+
+	if (!nvram_buf[0]) {
+		err = nvram_init();
+		if (err)
+			return NULL;
+	}
+
+	header = (struct nvram_header *)nvram_buf;
+	*nvram_size = header->len - sizeof(struct nvram_header);
+	nvram = vmalloc(*nvram_size);
+	if (!nvram)
+		return NULL;
+	memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size);
+
+	return nvram;
+}
+EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
+
diff --git a/include/linux/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
index b12b07e..c73927c 100644
--- a/include/linux/bcm47xx_nvram.h
+++ b/include/linux/bcm47xx_nvram.h
@@ -10,11 +10,17 @@
 
 #include <linux/types.h>
 #include <linux/kernel.h>
+#include <linux/vmalloc.h>
 
 #ifdef CONFIG_BCM47XX
 int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
 int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
 int bcm47xx_nvram_gpio_pin(const char *name);
+char *bcm47xx_nvram_get_contents(size_t *val_len);
+static inline void bcm47xx_nvram_release_contents(char *nvram)
+{
+	vfree(nvram);
+};
 #else
 static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
 {
@@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const char *name)
 {
 	return -ENOTSUPP;
 };
+
+static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
+{
+	return NULL;
+};
+
+static inline void bcm47xx_nvram_release_contents(char *nvram)
+{
+};
 #endif
 
 #endif /* __BCM47XX_NVRAM_H */
-- 
1.9.1

[PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[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.
The brcmfmac for one requires the complete nvram contents
to select what needs to be sent to wireless device.

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>
---
Change log:
-----------
V2:	- correct header length for nvram_find_and_copy().
	- get rid of 'NVRAM_SPACE - 2' magic.
---
 arch/mips/bcm47xx/nvram.c     | 61 ++++++++++++++++++++++++++++++++-----------
 include/linux/bcm47xx_nvram.h | 15 +++++++++++
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
index 95d028c..f4f62d3 100644
--- a/arch/mips/bcm47xx/nvram.c
+++ b/arch/mips/bcm47xx/nvram.c
@@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
 	return -ENXIO;
 
 found:
-	if (header->len > size)
-		pr_err("The nvram size accoridng to the header seems to be bigger than the partition on flash\n");
-	if (header->len > NVRAM_SPACE)
-		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
-		       header->len, NVRAM_SPACE - 1);
-
 	src = (u32 *)header;
 	dst = (u32 *)nvram_buf;
 	for (i = 0; i < sizeof(struct nvram_header); i += 4)
 		*dst++ = __raw_readl(src++);
-	for (; i < header->len && i < NVRAM_SPACE && i < size; i += 4)
+	header = (struct nvram_header *)nvram_buf;
+	if (header->len > size) {
+		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
+		header->len = size;
+	}
+	if (header->len >= NVRAM_SPACE) {
+		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
+		       header->len, NVRAM_SPACE - 1);
+		header->len = NVRAM_SPACE - 1;
+	}
+	/* proceed reading data after header */
+	for (; i < header->len; i += 4)
 		*dst++ = readl(src++);
 	nvram_buf[NVRAM_SPACE - 1] = '\0';
 
@@ -139,6 +144,7 @@ static int nvram_init(void)
 #ifdef CONFIG_MTD
 	struct mtd_info *mtd;
 	struct nvram_header header;
+	struct nvram_header *pheader;
 	size_t bytes_read;
 	int err;
 
@@ -147,20 +153,21 @@ static int nvram_init(void)
 		return -ENODEV;
 
 	err = mtd_read(mtd, 0, sizeof(header), &bytes_read, (uint8_t *)&header);
-	if (!err && header.magic == NVRAM_MAGIC) {
-		u8 *dst = (uint8_t *)nvram_buf;
-		size_t len = header.len;
-
-		if (len >= NVRAM_SPACE) {
-			len = NVRAM_SPACE - 1;
+	if (!err && header.magic == NVRAM_MAGIC &&
+	    header.len > sizeof(header)) {
+		if (header.len >= NVRAM_SPACE) {
 			pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
-				header.len, len);
+				header.len, NVRAM_SPACE);
+			header.len = NVRAM_SPACE - 1;
 		}
 
-		err = mtd_read(mtd, 0, len, &bytes_read, dst);
+		err = mtd_read(mtd, 0, header.len, &bytes_read,
+			       (u8 *)nvram_buf);
 		if (err)
 			return err;
 
+		pheader = (struct nvram_header *)nvram_buf;
+		pheader->len = header.len;
 		return 0;
 	}
 #endif
@@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
 	return -ENOENT;
 }
 EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
+
+char *bcm47xx_nvram_get_contents(size_t *nvram_size)
+{
+	int err;
+	char *nvram;
+	struct nvram_header *header;
+
+	if (!nvram_buf[0]) {
+		err = nvram_init();
+		if (err)
+			return NULL;
+	}
+
+	header = (struct nvram_header *)nvram_buf;
+	*nvram_size = header->len - sizeof(struct nvram_header);
+	nvram = vmalloc(*nvram_size);
+	if (!nvram)
+		return NULL;
+	memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size);
+
+	return nvram;
+}
+EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
+
diff --git a/include/linux/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
index b12b07e..c73927c 100644
--- a/include/linux/bcm47xx_nvram.h
+++ b/include/linux/bcm47xx_nvram.h
@@ -10,11 +10,17 @@
 
 #include <linux/types.h>
 #include <linux/kernel.h>
+#include <linux/vmalloc.h>
 
 #ifdef CONFIG_BCM47XX
 int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
 int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
 int bcm47xx_nvram_gpio_pin(const char *name);
+char *bcm47xx_nvram_get_contents(size_t *val_len);
+static inline void bcm47xx_nvram_release_contents(char *nvram)
+{
+	vfree(nvram);
+};
 #else
 static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
 {
@@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const char *name)
 {
 	return -ENOTSUPP;
 };
+
+static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
+{
+	return NULL;
+};
+
+static inline void bcm47xx_nvram_release_contents(char *nvram)
+{
+};
 #endif
 
 #endif /* __BCM47XX_NVRAM_H */
-- 
1.9.1

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Hauke Mehrtens]

On 05/21/2015 03:27 PM, Arend van Spriel wrote:
> From: Hante Meuleman <meuleman@broadcom.com>
> 
> Host platforms such as routers supported by OpenWrt can
> support NVRAM reading directly from internal NVRAM store.
> The brcmfmac for one requires the complete nvram contents
> to select what needs to be sent to wireless device.
> 
> 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>
> ---
> Change log:
> -----------
> V2:	- correct header length for nvram_find_and_copy().
> 	- get rid of 'NVRAM_SPACE - 2' magic.
> ---
>  arch/mips/bcm47xx/nvram.c     | 61 ++++++++++++++++++++++++++++++++-----------
>  include/linux/bcm47xx_nvram.h | 15 +++++++++++
>  2 files changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
> index 95d028c..f4f62d3 100644
> --- a/arch/mips/bcm47xx/nvram.c
> +++ b/arch/mips/bcm47xx/nvram.c
> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
>  	return -ENXIO;
>  
>  found:
> -	if (header->len > size)
> -		pr_err("The nvram size accoridng to the header seems to be bigger than the partition on flash\n");
> -	if (header->len > NVRAM_SPACE)
> -		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
> -		       header->len, NVRAM_SPACE - 1);
> -
>  	src = (u32 *)header;
>  	dst = (u32 *)nvram_buf;
>  	for (i = 0; i < sizeof(struct nvram_header); i += 4)
>  		*dst++ = __raw_readl(src++);
> -	for (; i < header->len && i < NVRAM_SPACE && i < size; i += 4)
> +	header = (struct nvram_header *)nvram_buf;
> +	if (header->len > size) {
> +		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");

Thanks for fixing a typo. ;-)

> +		header->len = size;

I haven't seen this error case, I just added it to be save.

> +	}
> +	if (header->len >= NVRAM_SPACE) {
> +		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
> +		       header->len, NVRAM_SPACE - 1);
> +		header->len = NVRAM_SPACE - 1;
> +	}

After fixing the length the header is better, but it also contains a
CRC, which is still wrong, but we ignore it and I do not think we have
to check.

I still think that the best approach would be to remove the nvram header
and add a extra variable storing the actual size of the nvram buf. I
also think this would make some of this code easier.

> +	/* proceed reading data after header */
> +	for (; i < header->len; i += 4)
>  		*dst++ = readl(src++);
>  	nvram_buf[NVRAM_SPACE - 1] = '\0';
>  
> @@ -139,6 +144,7 @@ static int nvram_init(void)
>  #ifdef CONFIG_MTD
>  	struct mtd_info *mtd;
>  	struct nvram_header header;
> +	struct nvram_header *pheader;
>  	size_t bytes_read;
>  	int err;
>  
> @@ -147,20 +153,21 @@ static int nvram_init(void)
>  		return -ENODEV;
>  
>  	err = mtd_read(mtd, 0, sizeof(header), &bytes_read, (uint8_t *)&header);
> -	if (!err && header.magic == NVRAM_MAGIC) {
> -		u8 *dst = (uint8_t *)nvram_buf;
> -		size_t len = header.len;
> -
> -		if (len >= NVRAM_SPACE) {
> -			len = NVRAM_SPACE - 1;
> +	if (!err && header.magic == NVRAM_MAGIC &&
> +	    header.len > sizeof(header)) {
> +		if (header.len >= NVRAM_SPACE) {
>  			pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
> -				header.len, len);
> +				header.len, NVRAM_SPACE);
> +			header.len = NVRAM_SPACE - 1;
>  		}
>  
> -		err = mtd_read(mtd, 0, len, &bytes_read, dst);
> +		err = mtd_read(mtd, 0, header.len, &bytes_read,
> +			       (u8 *)nvram_buf);
>  		if (err)
>  			return err;
>  
> +		pheader = (struct nvram_header *)nvram_buf;
> +		pheader->len = header.len;
>  		return 0;
>  	}
>  #endif
> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>  	return -ENOENT;
>  }
>  EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
> +
> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
> +{
> +	int err;
> +	char *nvram;
> +	struct nvram_header *header;
> +
> +	if (!nvram_buf[0]) {
> +		err = nvram_init();
> +		if (err)
> +			return NULL;
> +	}
> +
> +	header = (struct nvram_header *)nvram_buf;
> +	*nvram_size = header->len - sizeof(struct nvram_header);
> +	nvram = vmalloc(*nvram_size);
> +	if (!nvram)
> +		return NULL;
> +	memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size);

I have no real opinion if we need to copy it or provide a pointer to the
nvram buf. I trust brcmfmac that it does not screw around with this
data. ;-)

> +
> +	return nvram;
> +}
> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
> +
> diff --git a/include/linux/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
> index b12b07e..c73927c 100644
> --- a/include/linux/bcm47xx_nvram.h
> +++ b/include/linux/bcm47xx_nvram.h
> @@ -10,11 +10,17 @@
>  
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> +#include <linux/vmalloc.h>
>  
>  #ifdef CONFIG_BCM47XX
>  int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>  int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>  int bcm47xx_nvram_gpio_pin(const char *name);
> +char *bcm47xx_nvram_get_contents(size_t *val_len);
> +static inline void bcm47xx_nvram_release_contents(char *nvram)
> +{
> +	vfree(nvram);
> +};
>  #else
>  static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>  {
> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const char *name)
>  {
>  	return -ENOTSUPP;
>  };
> +
> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
> +{
> +	return NULL;
> +};
> +
> +static inline void bcm47xx_nvram_release_contents(char *nvram)
> +{
> +};
>  #endif
>  
>  #endif /* __BCM47XX_NVRAM_H */
> 

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 16:59, Hauke Mehrtens wrote:
>
>
> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>> From: Hante Meuleman<meuleman@broadcom.com>
>>
>> Host platforms such as routers supported by OpenWrt can
>> support NVRAM reading directly from internal NVRAM store.
>> The brcmfmac for one requires the complete nvram contents
>> to select what needs to be sent to wireless device.
>>
>> 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>
>> ---
>> Change log:
>> -----------
>> V2:	- correct header length for nvram_find_and_copy().
>> 	- get rid of 'NVRAM_SPACE - 2' magic.
>> ---
>>   arch/mips/bcm47xx/nvram.c     | 61 ++++++++++++++++++++++++++++++++-----------
>>   include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>   2 files changed, 61 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>> index 95d028c..f4f62d3 100644
>> --- a/arch/mips/bcm47xx/nvram.c
>> +++ b/arch/mips/bcm47xx/nvram.c
>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
>>   	return -ENXIO;
>>
>>   found:
>> -	if (header->len>  size)
>> -		pr_err("The nvram size accoridng to the header seems to be bigger than the partition on flash\n");
>> -	if (header->len>  NVRAM_SPACE)
>> -		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
>> -		       header->len, NVRAM_SPACE - 1);
>> -
>>   	src = (u32 *)header;
>>   	dst = (u32 *)nvram_buf;
>>   	for (i = 0; i<  sizeof(struct nvram_header); i += 4)
>>   		*dst++ = __raw_readl(src++);
>> -	for (; i<  header->len&&  i<  NVRAM_SPACE&&  i<  size; i += 4)
>> +	header = (struct nvram_header *)nvram_buf;
>> +	if (header->len>  size) {
>> +		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
>
> Thanks for fixing a typo. ;-)
>
>> +		header->len = size;
>
> I haven't seen this error case, I just added it to be save.

It does seem an unlikely error case. At least it would probably been 
noticed during production writing the content to flash.

>> +	}
>> +	if (header->len>= NVRAM_SPACE) {
>> +		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
>> +		       header->len, NVRAM_SPACE - 1);
>> +		header->len = NVRAM_SPACE - 1;
>> +	}
>
> After fixing the length the header is better, but it also contains a
> CRC, which is still wrong, but we ignore it and I do not think we have
> to check.
>
> I still think that the best approach would be to remove the nvram header
> and add a extra variable storing the actual size of the nvram buf. I
> also think this would make some of this code easier.

Let's do that then.

>> +	/* proceed reading data after header */
>> +	for (; i<  header->len; i += 4)
>>   		*dst++ = readl(src++);
>>   	nvram_buf[NVRAM_SPACE - 1] = '\0';
>>
>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>   #ifdef CONFIG_MTD
>>   	struct mtd_info *mtd;
>>   	struct nvram_header header;
>> +	struct nvram_header *pheader;
>>   	size_t bytes_read;
>>   	int err;
>>
>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>   		return -ENODEV;
>>
>>   	err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>> -	if (!err&&  header.magic == NVRAM_MAGIC) {
>> -		u8 *dst = (uint8_t *)nvram_buf;
>> -		size_t len = header.len;
>> -
>> -		if (len>= NVRAM_SPACE) {
>> -			len = NVRAM_SPACE - 1;
>> +	if (!err&&  header.magic == NVRAM_MAGIC&&
>> +	    header.len>  sizeof(header)) {
>> +		if (header.len>= NVRAM_SPACE) {
>>   			pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
>> -				header.len, len);
>> +				header.len, NVRAM_SPACE);
>> +			header.len = NVRAM_SPACE - 1;
>>   		}
>>
>> -		err = mtd_read(mtd, 0, len,&bytes_read, dst);
>> +		err = mtd_read(mtd, 0, header.len,&bytes_read,
>> +			       (u8 *)nvram_buf);
>>   		if (err)
>>   			return err;
>>
>> +		pheader = (struct nvram_header *)nvram_buf;
>> +		pheader->len = header.len;
>>   		return 0;
>>   	}
>>   #endif
>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>   	return -ENOENT;
>>   }
>>   EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>> +
>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>> +{
>> +	int err;
>> +	char *nvram;
>> +	struct nvram_header *header;
>> +
>> +	if (!nvram_buf[0]) {
>> +		err = nvram_init();
>> +		if (err)
>> +			return NULL;
>> +	}
>> +
>> +	header = (struct nvram_header *)nvram_buf;
>> +	*nvram_size = header->len - sizeof(struct nvram_header);
>> +	nvram = vmalloc(*nvram_size);
>> +	if (!nvram)
>> +		return NULL;
>> +	memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>
> I have no real opinion if we need to copy it or provide a pointer to the
> nvram buf. I trust brcmfmac that it does not screw around with this
> data. ;-)

I would not dare. Most variables are pitch black magic to me.

Regards,
Arend

>> +
>> +	return nvram;
>> +}
>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>> +
>> diff --git a/include/linux/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
>> index b12b07e..c73927c 100644
>> --- a/include/linux/bcm47xx_nvram.h
>> +++ b/include/linux/bcm47xx_nvram.h
>> @@ -10,11 +10,17 @@
>>
>>   #include<linux/types.h>
>>   #include<linux/kernel.h>
>> +#include<linux/vmalloc.h>
>>
>>   #ifdef CONFIG_BCM47XX
>>   int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>   int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>   int bcm47xx_nvram_gpio_pin(const char *name);
>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>> +{
>> +	vfree(nvram);
>> +};
>>   #else
>>   static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>   {
>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const char *name)
>>   {
>>   	return -ENOTSUPP;
>>   };
>> +
>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>> +{
>> +	return NULL;
>> +};
>> +
>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>> +{
>> +};
>>   #endif
>>
>>   #endif /* __BCM47XX_NVRAM_H */
>>

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 16:59, Hauke Mehrtens wrote:
>
>
> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>> From: Hante Meuleman<meuleman@broadcom.com>
>>
>> Host platforms such as routers supported by OpenWrt can
>> support NVRAM reading directly from internal NVRAM store.
>> The brcmfmac for one requires the complete nvram contents
>> to select what needs to be sent to wireless device.
>>
>> 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>
>> ---
>> Change log:
>> -----------
>> V2:	- correct header length for nvram_find_and_copy().
>> 	- get rid of 'NVRAM_SPACE - 2' magic.
>> ---
>>   arch/mips/bcm47xx/nvram.c     | 61 ++++++++++++++++++++++++++++++++-----------
>>   include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>   2 files changed, 61 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>> index 95d028c..f4f62d3 100644
>> --- a/arch/mips/bcm47xx/nvram.c
>> +++ b/arch/mips/bcm47xx/nvram.c
>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
>>   	return -ENXIO;
>>
>>   found:
>> -	if (header->len>  size)
>> -		pr_err("The nvram size accoridng to the header seems to be bigger than the partition on flash\n");
>> -	if (header->len>  NVRAM_SPACE)
>> -		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
>> -		       header->len, NVRAM_SPACE - 1);
>> -
>>   	src = (u32 *)header;
>>   	dst = (u32 *)nvram_buf;
>>   	for (i = 0; i<  sizeof(struct nvram_header); i += 4)
>>   		*dst++ = __raw_readl(src++);
>> -	for (; i<  header->len&&  i<  NVRAM_SPACE&&  i<  size; i += 4)
>> +	header = (struct nvram_header *)nvram_buf;
>> +	if (header->len>  size) {
>> +		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
>
> Thanks for fixing a typo. ;-)
>
>> +		header->len = size;
>
> I haven't seen this error case, I just added it to be save.

It does seem an unlikely error case. At least it would probably been 
noticed during production writing the content to flash.

>> +	}
>> +	if (header->len>= NVRAM_SPACE) {
>> +		pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
>> +		       header->len, NVRAM_SPACE - 1);
>> +		header->len = NVRAM_SPACE - 1;
>> +	}
>
> After fixing the length the header is better, but it also contains a
> CRC, which is still wrong, but we ignore it and I do not think we have
> to check.
>
> I still think that the best approach would be to remove the nvram header
> and add a extra variable storing the actual size of the nvram buf. I
> also think this would make some of this code easier.

Let's do that then.

>> +	/* proceed reading data after header */
>> +	for (; i<  header->len; i += 4)
>>   		*dst++ = readl(src++);
>>   	nvram_buf[NVRAM_SPACE - 1] = '\0';
>>
>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>   #ifdef CONFIG_MTD
>>   	struct mtd_info *mtd;
>>   	struct nvram_header header;
>> +	struct nvram_header *pheader;
>>   	size_t bytes_read;
>>   	int err;
>>
>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>   		return -ENODEV;
>>
>>   	err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>> -	if (!err&&  header.magic == NVRAM_MAGIC) {
>> -		u8 *dst = (uint8_t *)nvram_buf;
>> -		size_t len = header.len;
>> -
>> -		if (len>= NVRAM_SPACE) {
>> -			len = NVRAM_SPACE - 1;
>> +	if (!err&&  header.magic == NVRAM_MAGIC&&
>> +	    header.len>  sizeof(header)) {
>> +		if (header.len>= NVRAM_SPACE) {
>>   			pr_err("nvram on flash (%i bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
>> -				header.len, len);
>> +				header.len, NVRAM_SPACE);
>> +			header.len = NVRAM_SPACE - 1;
>>   		}
>>
>> -		err = mtd_read(mtd, 0, len,&bytes_read, dst);
>> +		err = mtd_read(mtd, 0, header.len,&bytes_read,
>> +			       (u8 *)nvram_buf);
>>   		if (err)
>>   			return err;
>>
>> +		pheader = (struct nvram_header *)nvram_buf;
>> +		pheader->len = header.len;
>>   		return 0;
>>   	}
>>   #endif
>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>   	return -ENOENT;
>>   }
>>   EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>> +
>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>> +{
>> +	int err;
>> +	char *nvram;
>> +	struct nvram_header *header;
>> +
>> +	if (!nvram_buf[0]) {
>> +		err = nvram_init();
>> +		if (err)
>> +			return NULL;
>> +	}
>> +
>> +	header = (struct nvram_header *)nvram_buf;
>> +	*nvram_size = header->len - sizeof(struct nvram_header);
>> +	nvram = vmalloc(*nvram_size);
>> +	if (!nvram)
>> +		return NULL;
>> +	memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>
> I have no real opinion if we need to copy it or provide a pointer to the
> nvram buf. I trust brcmfmac that it does not screw around with this
> data. ;-)

I would not dare. Most variables are pitch black magic to me.

Regards,
Arend

>> +
>> +	return nvram;
>> +}
>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>> +
>> diff --git a/include/linux/bcm47xx_nvram.h b/include/linux/bcm47xx_nvram.h
>> index b12b07e..c73927c 100644
>> --- a/include/linux/bcm47xx_nvram.h
>> +++ b/include/linux/bcm47xx_nvram.h
>> @@ -10,11 +10,17 @@
>>
>>   #include<linux/types.h>
>>   #include<linux/kernel.h>
>> +#include<linux/vmalloc.h>
>>
>>   #ifdef CONFIG_BCM47XX
>>   int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>   int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>   int bcm47xx_nvram_gpio_pin(const char *name);
>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>> +{
>> +	vfree(nvram);
>> +};
>>   #else
>>   static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>   {
>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const char *name)
>>   {
>>   	return -ENOTSUPP;
>>   };
>> +
>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>> +{
>> +	return NULL;
>> +};
>> +
>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>> +{
>> +};
>>   #endif
>>
>>   #endif /* __BCM47XX_NVRAM_H */
>>

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 18:57, Arend van Spriel wrote:
> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>
>>
>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>
>>> Host platforms such as routers supported by OpenWrt can
>>> support NVRAM reading directly from internal NVRAM store.
>>> The brcmfmac for one requires the complete nvram contents
>>> to select what needs to be sent to wireless device.
>>>
>>> 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>
>>> ---
>>> Change log:
>>> -----------
>>> V2: - correct header length for nvram_find_and_copy().
>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>> ---
>>> arch/mips/bcm47xx/nvram.c | 61
>>> ++++++++++++++++++++++++++++++++-----------
>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>> index 95d028c..f4f62d3 100644
>>> --- a/arch/mips/bcm47xx/nvram.c
>>> +++ b/arch/mips/bcm47xx/nvram.c
>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>> *iobase, u32 lim)
>>> return -ENXIO;
>>>
>>> found:
>>> - if (header->len> size)
>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>> than the partition on flash\n");
>>> - if (header->len> NVRAM_SPACE)
>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>> in memory, will just copy the first %i bytes\n",
>>> - header->len, NVRAM_SPACE - 1);
>>> -
>>> src = (u32 *)header;
>>> dst = (u32 *)nvram_buf;
>>> for (i = 0; i< sizeof(struct nvram_header); i += 4)
>>> *dst++ = __raw_readl(src++);
>>> - for (; i< header->len&& i< NVRAM_SPACE&& i< size; i += 4)
>>> + header = (struct nvram_header *)nvram_buf;
>>> + if (header->len> size) {
>>> + pr_err("The nvram size according to the header seems to be bigger
>>> than the partition on flash\n");
>>
>> Thanks for fixing a typo. ;-)
>>
>>> + header->len = size;
>>
>> I haven't seen this error case, I just added it to be save.
>
> It does seem an unlikely error case. At least it would probably been
> noticed during production writing the content to flash.
>
>>> + }
>>> + if (header->len>= NVRAM_SPACE) {
>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>> in memory, will just copy the first %i bytes\n",
>>> + header->len, NVRAM_SPACE - 1);
>>> + header->len = NVRAM_SPACE - 1;
>>> + }
>>
>> After fixing the length the header is better, but it also contains a
>> CRC, which is still wrong, but we ignore it and I do not think we have
>> to check.
>>
>> I still think that the best approach would be to remove the nvram header
>> and add a extra variable storing the actual size of the nvram buf. I
>> also think this would make some of this code easier.
>
> Let's do that then.

Hi Hauke,

Forgot to ask. What is the reason for using the fixed nvram space. Could 
we allocate it using vzalloc()?

Regards,
Arend

>>> + /* proceed reading data after header */
>>> + for (; i< header->len; i += 4)
>>> *dst++ = readl(src++);
>>> nvram_buf[NVRAM_SPACE - 1] = '\0';
>>>
>>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>> #ifdef CONFIG_MTD
>>> struct mtd_info *mtd;
>>> struct nvram_header header;
>>> + struct nvram_header *pheader;
>>> size_t bytes_read;
>>> int err;
>>>
>>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>> return -ENODEV;
>>>
>>> err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>>> - if (!err&& header.magic == NVRAM_MAGIC) {
>>> - u8 *dst = (uint8_t *)nvram_buf;
>>> - size_t len = header.len;
>>> -
>>> - if (len>= NVRAM_SPACE) {
>>> - len = NVRAM_SPACE - 1;
>>> + if (!err&& header.magic == NVRAM_MAGIC&&
>>> + header.len> sizeof(header)) {
>>> + if (header.len>= NVRAM_SPACE) {
>>> pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>> in memory, will just copy the first %i bytes\n",
>>> - header.len, len);
>>> + header.len, NVRAM_SPACE);
>>> + header.len = NVRAM_SPACE - 1;
>>> }
>>>
>>> - err = mtd_read(mtd, 0, len,&bytes_read, dst);
>>> + err = mtd_read(mtd, 0, header.len,&bytes_read,
>>> + (u8 *)nvram_buf);
>>> if (err)
>>> return err;
>>>
>>> + pheader = (struct nvram_header *)nvram_buf;
>>> + pheader->len = header.len;
>>> return 0;
>>> }
>>> #endif
>>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>> return -ENOENT;
>>> }
>>> EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>>> +
>>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>>> +{
>>> + int err;
>>> + char *nvram;
>>> + struct nvram_header *header;
>>> +
>>> + if (!nvram_buf[0]) {
>>> + err = nvram_init();
>>> + if (err)
>>> + return NULL;
>>> + }
>>> +
>>> + header = (struct nvram_header *)nvram_buf;
>>> + *nvram_size = header->len - sizeof(struct nvram_header);
>>> + nvram = vmalloc(*nvram_size);
>>> + if (!nvram)
>>> + return NULL;
>>> + memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>>
>> I have no real opinion if we need to copy it or provide a pointer to the
>> nvram buf. I trust brcmfmac that it does not screw around with this
>> data. ;-)
>
> I would not dare. Most variables are pitch black magic to me.
>
> Regards,
> Arend
>
>>> +
>>> + return nvram;
>>> +}
>>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>>> +
>>> diff --git a/include/linux/bcm47xx_nvram.h
>>> b/include/linux/bcm47xx_nvram.h
>>> index b12b07e..c73927c 100644
>>> --- a/include/linux/bcm47xx_nvram.h
>>> +++ b/include/linux/bcm47xx_nvram.h
>>> @@ -10,11 +10,17 @@
>>>
>>> #include<linux/types.h>
>>> #include<linux/kernel.h>
>>> +#include<linux/vmalloc.h>
>>>
>>> #ifdef CONFIG_BCM47XX
>>> int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>> int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>> int bcm47xx_nvram_gpio_pin(const char *name);
>>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>> +{
>>> + vfree(nvram);
>>> +};
>>> #else
>>> static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>> {
>>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const
>>> char *name)
>>> {
>>> return -ENOTSUPP;
>>> };
>>> +
>>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>> +{
>>> + return NULL;
>>> +};
>>> +
>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>> +{
>>> +};
>>> #endif
>>>
>>> #endif /* __BCM47XX_NVRAM_H */
>>>
>
>

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 18:57, Arend van Spriel wrote:
> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>
>>
>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>
>>> Host platforms such as routers supported by OpenWrt can
>>> support NVRAM reading directly from internal NVRAM store.
>>> The brcmfmac for one requires the complete nvram contents
>>> to select what needs to be sent to wireless device.
>>>
>>> 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>
>>> ---
>>> Change log:
>>> -----------
>>> V2: - correct header length for nvram_find_and_copy().
>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>> ---
>>> arch/mips/bcm47xx/nvram.c | 61
>>> ++++++++++++++++++++++++++++++++-----------
>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>> index 95d028c..f4f62d3 100644
>>> --- a/arch/mips/bcm47xx/nvram.c
>>> +++ b/arch/mips/bcm47xx/nvram.c
>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>> *iobase, u32 lim)
>>> return -ENXIO;
>>>
>>> found:
>>> - if (header->len> size)
>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>> than the partition on flash\n");
>>> - if (header->len> NVRAM_SPACE)
>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>> in memory, will just copy the first %i bytes\n",
>>> - header->len, NVRAM_SPACE - 1);
>>> -
>>> src = (u32 *)header;
>>> dst = (u32 *)nvram_buf;
>>> for (i = 0; i< sizeof(struct nvram_header); i += 4)
>>> *dst++ = __raw_readl(src++);
>>> - for (; i< header->len&& i< NVRAM_SPACE&& i< size; i += 4)
>>> + header = (struct nvram_header *)nvram_buf;
>>> + if (header->len> size) {
>>> + pr_err("The nvram size according to the header seems to be bigger
>>> than the partition on flash\n");
>>
>> Thanks for fixing a typo. ;-)
>>
>>> + header->len = size;
>>
>> I haven't seen this error case, I just added it to be save.
>
> It does seem an unlikely error case. At least it would probably been
> noticed during production writing the content to flash.
>
>>> + }
>>> + if (header->len>= NVRAM_SPACE) {
>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>> in memory, will just copy the first %i bytes\n",
>>> + header->len, NVRAM_SPACE - 1);
>>> + header->len = NVRAM_SPACE - 1;
>>> + }
>>
>> After fixing the length the header is better, but it also contains a
>> CRC, which is still wrong, but we ignore it and I do not think we have
>> to check.
>>
>> I still think that the best approach would be to remove the nvram header
>> and add a extra variable storing the actual size of the nvram buf. I
>> also think this would make some of this code easier.
>
> Let's do that then.

Hi Hauke,

Forgot to ask. What is the reason for using the fixed nvram space. Could 
we allocate it using vzalloc()?

Regards,
Arend

>>> + /* proceed reading data after header */
>>> + for (; i< header->len; i += 4)
>>> *dst++ = readl(src++);
>>> nvram_buf[NVRAM_SPACE - 1] = '\0';
>>>
>>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>> #ifdef CONFIG_MTD
>>> struct mtd_info *mtd;
>>> struct nvram_header header;
>>> + struct nvram_header *pheader;
>>> size_t bytes_read;
>>> int err;
>>>
>>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>> return -ENODEV;
>>>
>>> err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>>> - if (!err&& header.magic == NVRAM_MAGIC) {
>>> - u8 *dst = (uint8_t *)nvram_buf;
>>> - size_t len = header.len;
>>> -
>>> - if (len>= NVRAM_SPACE) {
>>> - len = NVRAM_SPACE - 1;
>>> + if (!err&& header.magic == NVRAM_MAGIC&&
>>> + header.len> sizeof(header)) {
>>> + if (header.len>= NVRAM_SPACE) {
>>> pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>> in memory, will just copy the first %i bytes\n",
>>> - header.len, len);
>>> + header.len, NVRAM_SPACE);
>>> + header.len = NVRAM_SPACE - 1;
>>> }
>>>
>>> - err = mtd_read(mtd, 0, len,&bytes_read, dst);
>>> + err = mtd_read(mtd, 0, header.len,&bytes_read,
>>> + (u8 *)nvram_buf);
>>> if (err)
>>> return err;
>>>
>>> + pheader = (struct nvram_header *)nvram_buf;
>>> + pheader->len = header.len;
>>> return 0;
>>> }
>>> #endif
>>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>> return -ENOENT;
>>> }
>>> EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>>> +
>>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>>> +{
>>> + int err;
>>> + char *nvram;
>>> + struct nvram_header *header;
>>> +
>>> + if (!nvram_buf[0]) {
>>> + err = nvram_init();
>>> + if (err)
>>> + return NULL;
>>> + }
>>> +
>>> + header = (struct nvram_header *)nvram_buf;
>>> + *nvram_size = header->len - sizeof(struct nvram_header);
>>> + nvram = vmalloc(*nvram_size);
>>> + if (!nvram)
>>> + return NULL;
>>> + memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>>
>> I have no real opinion if we need to copy it or provide a pointer to the
>> nvram buf. I trust brcmfmac that it does not screw around with this
>> data. ;-)
>
> I would not dare. Most variables are pitch black magic to me.
>
> Regards,
> Arend
>
>>> +
>>> + return nvram;
>>> +}
>>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>>> +
>>> diff --git a/include/linux/bcm47xx_nvram.h
>>> b/include/linux/bcm47xx_nvram.h
>>> index b12b07e..c73927c 100644
>>> --- a/include/linux/bcm47xx_nvram.h
>>> +++ b/include/linux/bcm47xx_nvram.h
>>> @@ -10,11 +10,17 @@
>>>
>>> #include<linux/types.h>
>>> #include<linux/kernel.h>
>>> +#include<linux/vmalloc.h>
>>>
>>> #ifdef CONFIG_BCM47XX
>>> int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>> int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>> int bcm47xx_nvram_gpio_pin(const char *name);
>>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>> +{
>>> + vfree(nvram);
>>> +};
>>> #else
>>> static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>> {
>>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const
>>> char *name)
>>> {
>>> return -ENOTSUPP;
>>> };
>>> +
>>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>> +{
>>> + return NULL;
>>> +};
>>> +
>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>> +{
>>> +};
>>> #endif
>>>
>>> #endif /* __BCM47XX_NVRAM_H */
>>>
>
>

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Hauke Mehrtens]

On 05/29/2015 08:26 PM, Arend van Spriel wrote:
> On 05/29/15 18:57, Arend van Spriel wrote:
>> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>>
>>>
>>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>>
>>>> Host platforms such as routers supported by OpenWrt can
>>>> support NVRAM reading directly from internal NVRAM store.
>>>> The brcmfmac for one requires the complete nvram contents
>>>> to select what needs to be sent to wireless device.
>>>>
>>>> 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>
>>>> ---
>>>> Change log:
>>>> -----------
>>>> V2: - correct header length for nvram_find_and_copy().
>>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>>> ---
>>>> arch/mips/bcm47xx/nvram.c | 61
>>>> ++++++++++++++++++++++++++++++++-----------
>>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>>> index 95d028c..f4f62d3 100644
>>>> --- a/arch/mips/bcm47xx/nvram.c
>>>> +++ b/arch/mips/bcm47xx/nvram.c
>>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>>> *iobase, u32 lim)
>>>> return -ENXIO;
>>>>
>>>> found:
>>>> - if (header->len> size)
>>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>>> than the partition on flash\n");
>>>> - if (header->len> NVRAM_SPACE)
>>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>> in memory, will just copy the first %i bytes\n",
>>>> - header->len, NVRAM_SPACE - 1);
>>>> -
>>>> src = (u32 *)header;
>>>> dst = (u32 *)nvram_buf;
>>>> for (i = 0; i< sizeof(struct nvram_header); i += 4)
>>>> *dst++ = __raw_readl(src++);
>>>> - for (; i< header->len&& i< NVRAM_SPACE&& i< size; i += 4)
>>>> + header = (struct nvram_header *)nvram_buf;
>>>> + if (header->len> size) {
>>>> + pr_err("The nvram size according to the header seems to be bigger
>>>> than the partition on flash\n");
>>>
>>> Thanks for fixing a typo. ;-)
>>>
>>>> + header->len = size;
>>>
>>> I haven't seen this error case, I just added it to be save.
>>
>> It does seem an unlikely error case. At least it would probably been
>> noticed during production writing the content to flash.
>>
>>>> + }
>>>> + if (header->len>= NVRAM_SPACE) {
>>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>> in memory, will just copy the first %i bytes\n",
>>>> + header->len, NVRAM_SPACE - 1);
>>>> + header->len = NVRAM_SPACE - 1;
>>>> + }
>>>
>>> After fixing the length the header is better, but it also contains a
>>> CRC, which is still wrong, but we ignore it and I do not think we have
>>> to check.
>>>
>>> I still think that the best approach would be to remove the nvram header
>>> and add a extra variable storing the actual size of the nvram buf. I
>>> also think this would make some of this code easier.
>>
>> Let's do that then.
> 
> Hi Hauke,
> 
> Forgot to ask. What is the reason for using the fixed nvram space. Could
> we allocate it using vzalloc()?

On mips this is needed very early in the boot process at least for the
older SoCs using ssb. In this state we can not allocate memory.

Hauke

> 
> Regards,
> Arend
> 
>>>> + /* proceed reading data after header */
>>>> + for (; i< header->len; i += 4)
>>>> *dst++ = readl(src++);
>>>> nvram_buf[NVRAM_SPACE - 1] = '\0';
>>>>
>>>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>>> #ifdef CONFIG_MTD
>>>> struct mtd_info *mtd;
>>>> struct nvram_header header;
>>>> + struct nvram_header *pheader;
>>>> size_t bytes_read;
>>>> int err;
>>>>
>>>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>>> return -ENODEV;
>>>>
>>>> err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>>>> - if (!err&& header.magic == NVRAM_MAGIC) {
>>>> - u8 *dst = (uint8_t *)nvram_buf;
>>>> - size_t len = header.len;
>>>> -
>>>> - if (len>= NVRAM_SPACE) {
>>>> - len = NVRAM_SPACE - 1;
>>>> + if (!err&& header.magic == NVRAM_MAGIC&&
>>>> + header.len> sizeof(header)) {
>>>> + if (header.len>= NVRAM_SPACE) {
>>>> pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>> in memory, will just copy the first %i bytes\n",
>>>> - header.len, len);
>>>> + header.len, NVRAM_SPACE);
>>>> + header.len = NVRAM_SPACE - 1;
>>>> }
>>>>
>>>> - err = mtd_read(mtd, 0, len,&bytes_read, dst);
>>>> + err = mtd_read(mtd, 0, header.len,&bytes_read,
>>>> + (u8 *)nvram_buf);
>>>> if (err)
>>>> return err;
>>>>
>>>> + pheader = (struct nvram_header *)nvram_buf;
>>>> + pheader->len = header.len;
>>>> return 0;
>>>> }
>>>> #endif
>>>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>>> return -ENOENT;
>>>> }
>>>> EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>>>> +
>>>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>>>> +{
>>>> + int err;
>>>> + char *nvram;
>>>> + struct nvram_header *header;
>>>> +
>>>> + if (!nvram_buf[0]) {
>>>> + err = nvram_init();
>>>> + if (err)
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + header = (struct nvram_header *)nvram_buf;
>>>> + *nvram_size = header->len - sizeof(struct nvram_header);
>>>> + nvram = vmalloc(*nvram_size);
>>>> + if (!nvram)
>>>> + return NULL;
>>>> + memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>>>
>>> I have no real opinion if we need to copy it or provide a pointer to the
>>> nvram buf. I trust brcmfmac that it does not screw around with this
>>> data. ;-)
>>
>> I would not dare. Most variables are pitch black magic to me.
>>
>> Regards,
>> Arend
>>
>>>> +
>>>> + return nvram;
>>>> +}
>>>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>>>> +
>>>> diff --git a/include/linux/bcm47xx_nvram.h
>>>> b/include/linux/bcm47xx_nvram.h
>>>> index b12b07e..c73927c 100644
>>>> --- a/include/linux/bcm47xx_nvram.h
>>>> +++ b/include/linux/bcm47xx_nvram.h
>>>> @@ -10,11 +10,17 @@
>>>>
>>>> #include<linux/types.h>
>>>> #include<linux/kernel.h>
>>>> +#include<linux/vmalloc.h>
>>>>
>>>> #ifdef CONFIG_BCM47XX
>>>> int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>>> int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>>> int bcm47xx_nvram_gpio_pin(const char *name);
>>>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>>> +{
>>>> + vfree(nvram);
>>>> +};
>>>> #else
>>>> static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>>> {
>>>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const
>>>> char *name)
>>>> {
>>>> return -ENOTSUPP;
>>>> };
>>>> +
>>>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>>> +{
>>>> + return NULL;
>>>> +};
>>>> +
>>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>>> +{
>>>> +};
>>>> #endif
>>>>
>>>> #endif /* __BCM47XX_NVRAM_H */
>>>>
>>
>>
> 

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 20:44, Hauke Mehrtens wrote:
>
>
> On 05/29/2015 08:26 PM, Arend van Spriel wrote:
>> On 05/29/15 18:57, Arend van Spriel wrote:
>>> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>>>
>>>>
>>>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>>>
>>>>> Host platforms such as routers supported by OpenWrt can
>>>>> support NVRAM reading directly from internal NVRAM store.
>>>>> The brcmfmac for one requires the complete nvram contents
>>>>> to select what needs to be sent to wireless device.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> Change log:
>>>>> -----------
>>>>> V2: - correct header length for nvram_find_and_copy().
>>>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>>>> ---
>>>>> arch/mips/bcm47xx/nvram.c | 61
>>>>> ++++++++++++++++++++++++++++++++-----------
>>>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>>>> index 95d028c..f4f62d3 100644
>>>>> --- a/arch/mips/bcm47xx/nvram.c
>>>>> +++ b/arch/mips/bcm47xx/nvram.c
>>>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>>>> *iobase, u32 lim)
>>>>> return -ENXIO;
>>>>>
>>>>> found:
>>>>> - if (header->len>  size)
>>>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>>>> than the partition on flash\n");
>>>>> - if (header->len>  NVRAM_SPACE)
>>>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>> in memory, will just copy the first %i bytes\n",
>>>>> - header->len, NVRAM_SPACE - 1);
>>>>> -
>>>>> src = (u32 *)header;
>>>>> dst = (u32 *)nvram_buf;
>>>>> for (i = 0; i<  sizeof(struct nvram_header); i += 4)
>>>>> *dst++ = __raw_readl(src++);
>>>>> - for (; i<  header->len&&  i<  NVRAM_SPACE&&  i<  size; i += 4)
>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>> + if (header->len>  size) {
>>>>> + pr_err("The nvram size according to the header seems to be bigger
>>>>> than the partition on flash\n");
>>>>
>>>> Thanks for fixing a typo. ;-)
>>>>
>>>>> + header->len = size;
>>>>
>>>> I haven't seen this error case, I just added it to be save.
>>>
>>> It does seem an unlikely error case. At least it would probably been
>>> noticed during production writing the content to flash.
>>>
>>>>> + }
>>>>> + if (header->len>= NVRAM_SPACE) {
>>>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>> in memory, will just copy the first %i bytes\n",
>>>>> + header->len, NVRAM_SPACE - 1);
>>>>> + header->len = NVRAM_SPACE - 1;
>>>>> + }
>>>>
>>>> After fixing the length the header is better, but it also contains a
>>>> CRC, which is still wrong, but we ignore it and I do not think we have
>>>> to check.
>>>>
>>>> I still think that the best approach would be to remove the nvram header
>>>> and add a extra variable storing the actual size of the nvram buf. I
>>>> also think this would make some of this code easier.
>>>
>>> Let's do that then.
>>
>> Hi Hauke,
>>
>> Forgot to ask. What is the reason for using the fixed nvram space. Could
>> we allocate it using vzalloc()?
>
> On mips this is needed very early in the boot process at least for the
> older SoCs using ssb. In this state we can not allocate memory.

I suspected that, but wanted to be sure.

Thanks,
Arend

> Hauke
>
>>
>> Regards,
>> Arend
>>
>>>>> + /* proceed reading data after header */
>>>>> + for (; i<  header->len; i += 4)
>>>>> *dst++ = readl(src++);
>>>>> nvram_buf[NVRAM_SPACE - 1] = '\0';
>>>>>
>>>>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>>>> #ifdef CONFIG_MTD
>>>>> struct mtd_info *mtd;
>>>>> struct nvram_header header;
>>>>> + struct nvram_header *pheader;
>>>>> size_t bytes_read;
>>>>> int err;
>>>>>
>>>>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>>>> return -ENODEV;
>>>>>
>>>>> err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>>>>> - if (!err&&  header.magic == NVRAM_MAGIC) {
>>>>> - u8 *dst = (uint8_t *)nvram_buf;
>>>>> - size_t len = header.len;
>>>>> -
>>>>> - if (len>= NVRAM_SPACE) {
>>>>> - len = NVRAM_SPACE - 1;
>>>>> + if (!err&&  header.magic == NVRAM_MAGIC&&
>>>>> + header.len>  sizeof(header)) {
>>>>> + if (header.len>= NVRAM_SPACE) {
>>>>> pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>> in memory, will just copy the first %i bytes\n",
>>>>> - header.len, len);
>>>>> + header.len, NVRAM_SPACE);
>>>>> + header.len = NVRAM_SPACE - 1;
>>>>> }
>>>>>
>>>>> - err = mtd_read(mtd, 0, len,&bytes_read, dst);
>>>>> + err = mtd_read(mtd, 0, header.len,&bytes_read,
>>>>> + (u8 *)nvram_buf);
>>>>> if (err)
>>>>> return err;
>>>>>
>>>>> + pheader = (struct nvram_header *)nvram_buf;
>>>>> + pheader->len = header.len;
>>>>> return 0;
>>>>> }
>>>>> #endif
>>>>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>>>> return -ENOENT;
>>>>> }
>>>>> EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>>>>> +
>>>>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>>>>> +{
>>>>> + int err;
>>>>> + char *nvram;
>>>>> + struct nvram_header *header;
>>>>> +
>>>>> + if (!nvram_buf[0]) {
>>>>> + err = nvram_init();
>>>>> + if (err)
>>>>> + return NULL;
>>>>> + }
>>>>> +
>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>> + *nvram_size = header->len - sizeof(struct nvram_header);
>>>>> + nvram = vmalloc(*nvram_size);
>>>>> + if (!nvram)
>>>>> + return NULL;
>>>>> + memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>>>>
>>>> I have no real opinion if we need to copy it or provide a pointer to the
>>>> nvram buf. I trust brcmfmac that it does not screw around with this
>>>> data. ;-)
>>>
>>> I would not dare. Most variables are pitch black magic to me.
>>>
>>> Regards,
>>> Arend
>>>
>>>>> +
>>>>> + return nvram;
>>>>> +}
>>>>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>>>>> +
>>>>> diff --git a/include/linux/bcm47xx_nvram.h
>>>>> b/include/linux/bcm47xx_nvram.h
>>>>> index b12b07e..c73927c 100644
>>>>> --- a/include/linux/bcm47xx_nvram.h
>>>>> +++ b/include/linux/bcm47xx_nvram.h
>>>>> @@ -10,11 +10,17 @@
>>>>>
>>>>> #include<linux/types.h>
>>>>> #include<linux/kernel.h>
>>>>> +#include<linux/vmalloc.h>
>>>>>
>>>>> #ifdef CONFIG_BCM47XX
>>>>> int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>>>> int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>>>> int bcm47xx_nvram_gpio_pin(const char *name);
>>>>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>>>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>>>> +{
>>>>> + vfree(nvram);
>>>>> +};
>>>>> #else
>>>>> static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>>>> {
>>>>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const
>>>>> char *name)
>>>>> {
>>>>> return -ENOTSUPP;
>>>>> };
>>>>> +
>>>>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>>>> +{
>>>>> + return NULL;
>>>>> +};
>>>>> +
>>>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>>>> +{
>>>>> +};
>>>>> #endif
>>>>>
>>>>> #endif /* __BCM47XX_NVRAM_H */
>>>>>
>>>
>>>
>>

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 20:44, Hauke Mehrtens wrote:
>
>
> On 05/29/2015 08:26 PM, Arend van Spriel wrote:
>> On 05/29/15 18:57, Arend van Spriel wrote:
>>> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>>>
>>>>
>>>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>>>
>>>>> Host platforms such as routers supported by OpenWrt can
>>>>> support NVRAM reading directly from internal NVRAM store.
>>>>> The brcmfmac for one requires the complete nvram contents
>>>>> to select what needs to be sent to wireless device.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> Change log:
>>>>> -----------
>>>>> V2: - correct header length for nvram_find_and_copy().
>>>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>>>> ---
>>>>> arch/mips/bcm47xx/nvram.c | 61
>>>>> ++++++++++++++++++++++++++++++++-----------
>>>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>>>> index 95d028c..f4f62d3 100644
>>>>> --- a/arch/mips/bcm47xx/nvram.c
>>>>> +++ b/arch/mips/bcm47xx/nvram.c
>>>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>>>> *iobase, u32 lim)
>>>>> return -ENXIO;
>>>>>
>>>>> found:
>>>>> - if (header->len>  size)
>>>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>>>> than the partition on flash\n");
>>>>> - if (header->len>  NVRAM_SPACE)
>>>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>> in memory, will just copy the first %i bytes\n",
>>>>> - header->len, NVRAM_SPACE - 1);
>>>>> -
>>>>> src = (u32 *)header;
>>>>> dst = (u32 *)nvram_buf;
>>>>> for (i = 0; i<  sizeof(struct nvram_header); i += 4)
>>>>> *dst++ = __raw_readl(src++);
>>>>> - for (; i<  header->len&&  i<  NVRAM_SPACE&&  i<  size; i += 4)
>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>> + if (header->len>  size) {
>>>>> + pr_err("The nvram size according to the header seems to be bigger
>>>>> than the partition on flash\n");
>>>>
>>>> Thanks for fixing a typo. ;-)
>>>>
>>>>> + header->len = size;
>>>>
>>>> I haven't seen this error case, I just added it to be save.
>>>
>>> It does seem an unlikely error case. At least it would probably been
>>> noticed during production writing the content to flash.
>>>
>>>>> + }
>>>>> + if (header->len>= NVRAM_SPACE) {
>>>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>> in memory, will just copy the first %i bytes\n",
>>>>> + header->len, NVRAM_SPACE - 1);
>>>>> + header->len = NVRAM_SPACE - 1;
>>>>> + }
>>>>
>>>> After fixing the length the header is better, but it also contains a
>>>> CRC, which is still wrong, but we ignore it and I do not think we have
>>>> to check.
>>>>
>>>> I still think that the best approach would be to remove the nvram header
>>>> and add a extra variable storing the actual size of the nvram buf. I
>>>> also think this would make some of this code easier.
>>>
>>> Let's do that then.
>>
>> Hi Hauke,
>>
>> Forgot to ask. What is the reason for using the fixed nvram space. Could
>> we allocate it using vzalloc()?
>
> On mips this is needed very early in the boot process at least for the
> older SoCs using ssb. In this state we can not allocate memory.

I suspected that, but wanted to be sure.

Thanks,
Arend

> Hauke
>
>>
>> Regards,
>> Arend
>>
>>>>> + /* proceed reading data after header */
>>>>> + for (; i<  header->len; i += 4)
>>>>> *dst++ = readl(src++);
>>>>> nvram_buf[NVRAM_SPACE - 1] = '\0';
>>>>>
>>>>> @@ -139,6 +144,7 @@ static int nvram_init(void)
>>>>> #ifdef CONFIG_MTD
>>>>> struct mtd_info *mtd;
>>>>> struct nvram_header header;
>>>>> + struct nvram_header *pheader;
>>>>> size_t bytes_read;
>>>>> int err;
>>>>>
>>>>> @@ -147,20 +153,21 @@ static int nvram_init(void)
>>>>> return -ENODEV;
>>>>>
>>>>> err = mtd_read(mtd, 0, sizeof(header),&bytes_read, (uint8_t *)&header);
>>>>> - if (!err&&  header.magic == NVRAM_MAGIC) {
>>>>> - u8 *dst = (uint8_t *)nvram_buf;
>>>>> - size_t len = header.len;
>>>>> -
>>>>> - if (len>= NVRAM_SPACE) {
>>>>> - len = NVRAM_SPACE - 1;
>>>>> + if (!err&&  header.magic == NVRAM_MAGIC&&
>>>>> + header.len>  sizeof(header)) {
>>>>> + if (header.len>= NVRAM_SPACE) {
>>>>> pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>> in memory, will just copy the first %i bytes\n",
>>>>> - header.len, len);
>>>>> + header.len, NVRAM_SPACE);
>>>>> + header.len = NVRAM_SPACE - 1;
>>>>> }
>>>>>
>>>>> - err = mtd_read(mtd, 0, len,&bytes_read, dst);
>>>>> + err = mtd_read(mtd, 0, header.len,&bytes_read,
>>>>> + (u8 *)nvram_buf);
>>>>> if (err)
>>>>> return err;
>>>>>
>>>>> + pheader = (struct nvram_header *)nvram_buf;
>>>>> + pheader->len = header.len;
>>>>> return 0;
>>>>> }
>>>>> #endif
>>>>> @@ -219,3 +226,27 @@ int bcm47xx_nvram_gpio_pin(const char *name)
>>>>> return -ENOENT;
>>>>> }
>>>>> EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin);
>>>>> +
>>>>> +char *bcm47xx_nvram_get_contents(size_t *nvram_size)
>>>>> +{
>>>>> + int err;
>>>>> + char *nvram;
>>>>> + struct nvram_header *header;
>>>>> +
>>>>> + if (!nvram_buf[0]) {
>>>>> + err = nvram_init();
>>>>> + if (err)
>>>>> + return NULL;
>>>>> + }
>>>>> +
>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>> + *nvram_size = header->len - sizeof(struct nvram_header);
>>>>> + nvram = vmalloc(*nvram_size);
>>>>> + if (!nvram)
>>>>> + return NULL;
>>>>> + memcpy(nvram,&nvram_buf[sizeof(struct nvram_header)], *nvram_size);
>>>>
>>>> I have no real opinion if we need to copy it or provide a pointer to the
>>>> nvram buf. I trust brcmfmac that it does not screw around with this
>>>> data. ;-)
>>>
>>> I would not dare. Most variables are pitch black magic to me.
>>>
>>> Regards,
>>> Arend
>>>
>>>>> +
>>>>> + return nvram;
>>>>> +}
>>>>> +EXPORT_SYMBOL(bcm47xx_nvram_get_contents);
>>>>> +
>>>>> diff --git a/include/linux/bcm47xx_nvram.h
>>>>> b/include/linux/bcm47xx_nvram.h
>>>>> index b12b07e..c73927c 100644
>>>>> --- a/include/linux/bcm47xx_nvram.h
>>>>> +++ b/include/linux/bcm47xx_nvram.h
>>>>> @@ -10,11 +10,17 @@
>>>>>
>>>>> #include<linux/types.h>
>>>>> #include<linux/kernel.h>
>>>>> +#include<linux/vmalloc.h>
>>>>>
>>>>> #ifdef CONFIG_BCM47XX
>>>>> int bcm47xx_nvram_init_from_mem(u32 base, u32 lim);
>>>>> int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len);
>>>>> int bcm47xx_nvram_gpio_pin(const char *name);
>>>>> +char *bcm47xx_nvram_get_contents(size_t *val_len);
>>>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>>>> +{
>>>>> + vfree(nvram);
>>>>> +};
>>>>> #else
>>>>> static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
>>>>> {
>>>>> @@ -29,6 +35,15 @@ static inline int bcm47xx_nvram_gpio_pin(const
>>>>> char *name)
>>>>> {
>>>>> return -ENOTSUPP;
>>>>> };
>>>>> +
>>>>> +static inline char *bcm47xx_nvram_get_contents(size_t *val_len)
>>>>> +{
>>>>> + return NULL;
>>>>> +};
>>>>> +
>>>>> +static inline void bcm47xx_nvram_release_contents(char *nvram)
>>>>> +{
>>>>> +};
>>>>> #endif
>>>>>
>>>>> #endif /* __BCM47XX_NVRAM_H */
>>>>>
>>>
>>>
>>

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Hauke Mehrtens]

On 05/29/2015 09:09 PM, Arend van Spriel wrote:
> On 05/29/15 20:44, Hauke Mehrtens wrote:
>>
>>
>> On 05/29/2015 08:26 PM, Arend van Spriel wrote:
>>> On 05/29/15 18:57, Arend van Spriel wrote:
>>>> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>>>>
>>>>>
>>>>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>>>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>>>>
>>>>>> Host platforms such as routers supported by OpenWrt can
>>>>>> support NVRAM reading directly from internal NVRAM store.
>>>>>> The brcmfmac for one requires the complete nvram contents
>>>>>> to select what needs to be sent to wireless device.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> Change log:
>>>>>> -----------
>>>>>> V2: - correct header length for nvram_find_and_copy().
>>>>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>>>>> ---
>>>>>> arch/mips/bcm47xx/nvram.c | 61
>>>>>> ++++++++++++++++++++++++++++++++-----------
>>>>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>>>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>>>>> index 95d028c..f4f62d3 100644
>>>>>> --- a/arch/mips/bcm47xx/nvram.c
>>>>>> +++ b/arch/mips/bcm47xx/nvram.c
>>>>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>>>>> *iobase, u32 lim)
>>>>>> return -ENXIO;
>>>>>>
>>>>>> found:
>>>>>> - if (header->len>  size)
>>>>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>>>>> than the partition on flash\n");
>>>>>> - if (header->len>  NVRAM_SPACE)
>>>>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>>> in memory, will just copy the first %i bytes\n",
>>>>>> - header->len, NVRAM_SPACE - 1);
>>>>>> -
>>>>>> src = (u32 *)header;
>>>>>> dst = (u32 *)nvram_buf;
>>>>>> for (i = 0; i<  sizeof(struct nvram_header); i += 4)
>>>>>> *dst++ = __raw_readl(src++);
>>>>>> - for (; i<  header->len&&  i<  NVRAM_SPACE&&  i<  size; i += 4)
>>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>>> + if (header->len>  size) {
>>>>>> + pr_err("The nvram size according to the header seems to be bigger
>>>>>> than the partition on flash\n");
>>>>>
>>>>> Thanks for fixing a typo. ;-)
>>>>>
>>>>>> + header->len = size;
>>>>>
>>>>> I haven't seen this error case, I just added it to be save.
>>>>
>>>> It does seem an unlikely error case. At least it would probably been
>>>> noticed during production writing the content to flash.
>>>>
>>>>>> + }
>>>>>> + if (header->len>= NVRAM_SPACE) {
>>>>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>>> in memory, will just copy the first %i bytes\n",
>>>>>> + header->len, NVRAM_SPACE - 1);
>>>>>> + header->len = NVRAM_SPACE - 1;
>>>>>> + }
>>>>>
>>>>> After fixing the length the header is better, but it also contains a
>>>>> CRC, which is still wrong, but we ignore it and I do not think we have
>>>>> to check.
>>>>>
>>>>> I still think that the best approach would be to remove the nvram
>>>>> header
>>>>> and add a extra variable storing the actual size of the nvram buf. I
>>>>> also think this would make some of this code easier.
>>>>
>>>> Let's do that then.
>>>
>>> Hi Hauke,
>>>
>>> Forgot to ask. What is the reason for using the fixed nvram space. Could
>>> we allocate it using vzalloc()?
>>
>> On mips this is needed very early in the boot process at least for the
>> older SoCs using ssb. In this state we can not allocate memory.
> 
> I suspected that, but wanted to be sure.
> 

We tried to use some kind of allocation mechanism, because the nvram
partitions of different devices have different sizes. The older devices
mostly have small partitions and the newer ons bigger. With the arm SoCs
we do not need the nvram so early and can use some normal allocation
mechanism.

Hauke

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 21:24, Hauke Mehrtens wrote:
>
>
> On 05/29/2015 09:09 PM, Arend van Spriel wrote:
>> On 05/29/15 20:44, Hauke Mehrtens wrote:
>>>
>>>
>>> On 05/29/2015 08:26 PM, Arend van Spriel wrote:
>>>> On 05/29/15 18:57, Arend van Spriel wrote:
>>>>> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>>>>>
>>>>>>
>>>>>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>>>>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>>>>>
>>>>>>> Host platforms such as routers supported by OpenWrt can
>>>>>>> support NVRAM reading directly from internal NVRAM store.
>>>>>>> The brcmfmac for one requires the complete nvram contents
>>>>>>> to select what needs to be sent to wireless device.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>> Change log:
>>>>>>> -----------
>>>>>>> V2: - correct header length for nvram_find_and_copy().
>>>>>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>>>>>> ---
>>>>>>> arch/mips/bcm47xx/nvram.c | 61
>>>>>>> ++++++++++++++++++++++++++++++++-----------
>>>>>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>>>>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>>>>>> index 95d028c..f4f62d3 100644
>>>>>>> --- a/arch/mips/bcm47xx/nvram.c
>>>>>>> +++ b/arch/mips/bcm47xx/nvram.c
>>>>>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>>>>>> *iobase, u32 lim)
>>>>>>> return -ENXIO;
>>>>>>>
>>>>>>> found:
>>>>>>> - if (header->len>   size)
>>>>>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>>>>>> than the partition on flash\n");
>>>>>>> - if (header->len>   NVRAM_SPACE)
>>>>>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>>>> in memory, will just copy the first %i bytes\n",
>>>>>>> - header->len, NVRAM_SPACE - 1);
>>>>>>> -
>>>>>>> src = (u32 *)header;
>>>>>>> dst = (u32 *)nvram_buf;
>>>>>>> for (i = 0; i<   sizeof(struct nvram_header); i += 4)
>>>>>>> *dst++ = __raw_readl(src++);
>>>>>>> - for (; i<   header->len&&   i<   NVRAM_SPACE&&   i<   size; i += 4)
>>>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>>>> + if (header->len>   size) {
>>>>>>> + pr_err("The nvram size according to the header seems to be bigger
>>>>>>> than the partition on flash\n");
>>>>>>
>>>>>> Thanks for fixing a typo. ;-)
>>>>>>
>>>>>>> + header->len = size;
>>>>>>
>>>>>> I haven't seen this error case, I just added it to be save.
>>>>>
>>>>> It does seem an unlikely error case. At least it would probably been
>>>>> noticed during production writing the content to flash.
>>>>>
>>>>>>> + }
>>>>>>> + if (header->len>= NVRAM_SPACE) {
>>>>>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>>>> in memory, will just copy the first %i bytes\n",
>>>>>>> + header->len, NVRAM_SPACE - 1);
>>>>>>> + header->len = NVRAM_SPACE - 1;
>>>>>>> + }
>>>>>>
>>>>>> After fixing the length the header is better, but it also contains a
>>>>>> CRC, which is still wrong, but we ignore it and I do not think we have
>>>>>> to check.
>>>>>>
>>>>>> I still think that the best approach would be to remove the nvram
>>>>>> header
>>>>>> and add a extra variable storing the actual size of the nvram buf. I
>>>>>> also think this would make some of this code easier.
>>>>>
>>>>> Let's do that then.
>>>>
>>>> Hi Hauke,
>>>>
>>>> Forgot to ask. What is the reason for using the fixed nvram space. Could
>>>> we allocate it using vzalloc()?
>>>
>>> On mips this is needed very early in the boot process at least for the
>>> older SoCs using ssb. In this state we can not allocate memory.
>>
>> I suspected that, but wanted to be sure.
>>
>
> We tried to use some kind of allocation mechanism, because the nvram
> partitions of different devices have different sizes. The older devices
> mostly have small partitions and the newer ons bigger. With the arm SoCs
> we do not need the nvram so early and can use some normal allocation
> mechanism.

Yes, but that means that static buffer sits there doing nothing or could 
that be put under Kconfig and only be used for the devices needing early 
access.

Regards,
Arend

Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents

[Arend van Spriel]

On 05/29/15 21:24, Hauke Mehrtens wrote:
>
>
> On 05/29/2015 09:09 PM, Arend van Spriel wrote:
>> On 05/29/15 20:44, Hauke Mehrtens wrote:
>>>
>>>
>>> On 05/29/2015 08:26 PM, Arend van Spriel wrote:
>>>> On 05/29/15 18:57, Arend van Spriel wrote:
>>>>> On 05/29/15 16:59, Hauke Mehrtens wrote:
>>>>>>
>>>>>>
>>>>>> On 05/21/2015 03:27 PM, Arend van Spriel wrote:
>>>>>>> From: Hante Meuleman<meuleman@broadcom.com>
>>>>>>>
>>>>>>> Host platforms such as routers supported by OpenWrt can
>>>>>>> support NVRAM reading directly from internal NVRAM store.
>>>>>>> The brcmfmac for one requires the complete nvram contents
>>>>>>> to select what needs to be sent to wireless device.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>> Change log:
>>>>>>> -----------
>>>>>>> V2: - correct header length for nvram_find_and_copy().
>>>>>>> - get rid of 'NVRAM_SPACE - 2' magic.
>>>>>>> ---
>>>>>>> arch/mips/bcm47xx/nvram.c | 61
>>>>>>> ++++++++++++++++++++++++++++++++-----------
>>>>>>> include/linux/bcm47xx_nvram.h | 15 +++++++++++
>>>>>>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c
>>>>>>> index 95d028c..f4f62d3 100644
>>>>>>> --- a/arch/mips/bcm47xx/nvram.c
>>>>>>> +++ b/arch/mips/bcm47xx/nvram.c
>>>>>>> @@ -94,17 +94,22 @@ static int nvram_find_and_copy(void __iomem
>>>>>>> *iobase, u32 lim)
>>>>>>> return -ENXIO;
>>>>>>>
>>>>>>> found:
>>>>>>> - if (header->len>   size)
>>>>>>> - pr_err("The nvram size accoridng to the header seems to be bigger
>>>>>>> than the partition on flash\n");
>>>>>>> - if (header->len>   NVRAM_SPACE)
>>>>>>> - pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>>>> in memory, will just copy the first %i bytes\n",
>>>>>>> - header->len, NVRAM_SPACE - 1);
>>>>>>> -
>>>>>>> src = (u32 *)header;
>>>>>>> dst = (u32 *)nvram_buf;
>>>>>>> for (i = 0; i<   sizeof(struct nvram_header); i += 4)
>>>>>>> *dst++ = __raw_readl(src++);
>>>>>>> - for (; i<   header->len&&   i<   NVRAM_SPACE&&   i<   size; i += 4)
>>>>>>> + header = (struct nvram_header *)nvram_buf;
>>>>>>> + if (header->len>   size) {
>>>>>>> + pr_err("The nvram size according to the header seems to be bigger
>>>>>>> than the partition on flash\n");
>>>>>>
>>>>>> Thanks for fixing a typo. ;-)
>>>>>>
>>>>>>> + header->len = size;
>>>>>>
>>>>>> I haven't seen this error case, I just added it to be save.
>>>>>
>>>>> It does seem an unlikely error case. At least it would probably been
>>>>> noticed during production writing the content to flash.
>>>>>
>>>>>>> + }
>>>>>>> + if (header->len>= NVRAM_SPACE) {
>>>>>>> + pr_err("nvram on flash (%i bytes) is bigger than the reserved space
>>>>>>> in memory, will just copy the first %i bytes\n",
>>>>>>> + header->len, NVRAM_SPACE - 1);
>>>>>>> + header->len = NVRAM_SPACE - 1;
>>>>>>> + }
>>>>>>
>>>>>> After fixing the length the header is better, but it also contains a
>>>>>> CRC, which is still wrong, but we ignore it and I do not think we have
>>>>>> to check.
>>>>>>
>>>>>> I still think that the best approach would be to remove the nvram
>>>>>> header
>>>>>> and add a extra variable storing the actual size of the nvram buf. I
>>>>>> also think this would make some of this code easier.
>>>>>
>>>>> Let's do that then.
>>>>
>>>> Hi Hauke,
>>>>
>>>> Forgot to ask. What is the reason for using the fixed nvram space. Could
>>>> we allocate it using vzalloc()?
>>>
>>> On mips this is needed very early in the boot process at least for the
>>> older SoCs using ssb. In this state we can not allocate memory.
>>
>> I suspected that, but wanted to be sure.
>>
>
> We tried to use some kind of allocation mechanism, because the nvram
> partitions of different devices have different sizes. The older devices
> mostly have small partitions and the newer ons bigger. With the arm SoCs
> we do not need the nvram so early and can use some normal allocation
> mechanism.

Yes, but that means that static buffer sits there doing nothing or could 
that be put under Kconfig and only be used for the devices needing early 
access.

Regards,
Arend