On 05/21/2015 03:27 PM, Arend van Spriel wrote:
> From: Hante Meuleman <meuleman@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Reviewed-by: Franky (Zhenhui) Lin <frankyl@xxxxxxxxxxxx>
> Reviewed-by: Pieter-Paul Giesberts <pieterpg@xxxxxxxxxxxx>
> Reviewed-by: Daniel (Deognyoun) Kim <dekim@xxxxxxxxxxxx>
> Signed-off-by: Hante Meuleman <meuleman@xxxxxxxxxxxx>
> Signed-off-by: Arend van Spriel <arend@xxxxxxxxxxxx>
> ---
> 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 */
>
