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

[Arend van Spriel <arend@broadcom.com>]

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