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

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

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