| Message ID | 1432122655-3224-1-git-send-email-arend@broadcom.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On 20 May 2015 at 13:50, Arend van Spriel <arend@broadcom.com> 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. First of all, I have to ask you to rebase this patch on top of upstream-sfr. Mostly because of MIPS: BCM47XX: Make sure NVRAM buffer ends with \0 > @@ -146,20 +147,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 (header.len > NVRAM_SPACE) { > + if (!err && header.magic == NVRAM_MAGIC && > + header.len > sizeof(header)) { > + if (header.len > NVRAM_SPACE - 2) { > 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); > - len = NVRAM_SPACE; > + header.len = NVRAM_SPACE - 2; > } I guess I preferred having "len" helper, but it's a minor thing. What's the trick with this NVRAM_SPACE - 2? Requiring string I to be ended with double \0 sounds like a wrong design in some driver. I don't think it's anything common/any standard to mark the buffer end with an extra \0. I'm pretty sure bcm47xx_nvram_getenv doesn't need it and bcm47xx_nvram_get_contents you implemented provides buffer length anyway. Moreover this trick isn't compatible with what nvram_find_and_copy does. > - 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; I preferred your OpenWrt patch version with just keeping a buffer content length in separated variable. It won't kill us to have one more static size_t and we'll at least keep a real header copy without hacking it for implementation needs. Again, what you did here doesn't match nvram_find_and_copy, so please make sure you'll e.g. set content length variable in nvram_find_and_copy as well.
On 05/20/15 14:31, Rafa? Mi?ecki wrote: > On 20 May 2015 at 13:50, Arend van Spriel<arend@broadcom.com> 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. > > First of all, I have to ask you to rebase this patch on top of > upstream-sfr. Mostly because of > MIPS: BCM47XX: Make sure NVRAM buffer ends with \0 No idea what upstream-sfr is. I applied the patch on top of the master branch of linux-mips repo [1]. What am I missing here? Regards, Arend [1] http://git.linux-mips.org/cgit/ralf/linux.git >> @@ -146,20 +147,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 (header.len> NVRAM_SPACE) { >> + if (!err&& header.magic == NVRAM_MAGIC&& >> + header.len> sizeof(header)) { >> + if (header.len> NVRAM_SPACE - 2) { >> 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); >> - len = NVRAM_SPACE; >> + header.len = NVRAM_SPACE - 2; >> } > > I guess I preferred having "len" helper, but it's a minor thing. > What's the trick with this NVRAM_SPACE - 2? Requiring string I to be > ended with double \0 sounds like a wrong design in some driver. I > don't think it's anything common/any standard to mark the buffer end > with an extra \0. I'm pretty sure bcm47xx_nvram_getenv doesn't need it > and bcm47xx_nvram_get_contents you implemented provides buffer length > anyway. > Moreover this trick isn't compatible with what nvram_find_and_copy does. > > >> - 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; > > I preferred your OpenWrt patch version with just keeping a buffer > content length in separated variable. It won't kill us to have one > more static size_t and we'll at least keep a real header copy without > hacking it for implementation needs. > Again, what you did here doesn't match nvram_find_and_copy, so please > make sure you'll e.g. set content length variable in > nvram_find_and_copy as well. > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 May 2015 at 14:54, Arend van Spriel <arend@broadcom.com> wrote: > On 05/20/15 14:31, Rafa? Mi?ecki wrote: >> >> On 20 May 2015 at 13:50, Arend van Spriel<arend@broadcom.com> 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. >> >> >> First of all, I have to ask you to rebase this patch on top of >> upstream-sfr. Mostly because of >> MIPS: BCM47XX: Make sure NVRAM buffer ends with \0 > > > No idea what upstream-sfr is. I applied the patch on top of the master > branch of linux-mips repo [1]. What am I missing here? > > [1] http://git.linux-mips.org/cgit/ralf/linux.git Just go a dir higher and you'll find it :) http://git.linux-mips.org/cgit/ Its a repo with mips-for-linux-next branch you're looking for. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/20/15 15:48, Rafa? Mi?ecki wrote: > On 20 May 2015 at 14:54, Arend van Spriel<arend@broadcom.com> wrote: >> On 05/20/15 14:31, Rafa? Mi?ecki wrote: >>> >>> On 20 May 2015 at 13:50, Arend van Spriel<arend@broadcom.com> 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. >>> >>> >>> First of all, I have to ask you to rebase this patch on top of >>> upstream-sfr. Mostly because of >>> MIPS: BCM47XX: Make sure NVRAM buffer ends with \0 >> >> >> No idea what upstream-sfr is. I applied the patch on top of the master >> branch of linux-mips repo [1]. What am I missing here? >> >> [1] http://git.linux-mips.org/cgit/ralf/linux.git > > Just go a dir higher and you'll find it :) > http://git.linux-mips.org/cgit/ > > Its a repo with mips-for-linux-next branch you're looking for. Thanks. Found it. Regards, Arend -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/mips/bcm47xx/nvram.c b/arch/mips/bcm47xx/nvram.c index ba632ff..3bb7f41 100644 --- a/arch/mips/bcm47xx/nvram.c +++ b/arch/mips/bcm47xx/nvram.c @@ -138,6 +138,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; @@ -146,20 +147,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 (header.len > NVRAM_SPACE) { + if (!err && header.magic == NVRAM_MAGIC && + header.len > sizeof(header)) { + if (header.len > NVRAM_SPACE - 2) { 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); - len = NVRAM_SPACE; + header.len = NVRAM_SPACE - 2; } - 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 @@ -221,3 +223,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); + + 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 */