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

[Hauke Mehrtens <hauke@hauke-m.de>]

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 */
>