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

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

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