| Message ID | F51492713EF10846800D8C0ED37A7DCE019445A4@SJEXCHMB15.corp.ad.broadcom.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Rafał Miłecki |
| Headers | show |
On 12 May 2015 at 15:10, Hante Meuleman <meuleman@broadcom.com> wrote: > Added two functions to the bcm47xx_nvram module to get and release copies > of the complete nvram contents. This can be used by for example brcmfmac > to get complete nvram contents which will after some parsing be sent to > device. Thanks for sending this PATCH in a friendly way. It's plain text and seems to be well formatted, nice! Two comments: 1) This patch does more than you say. You modify initialization and reading. Please keep patch per change (and explain them). 2) You modify OpenWrt's copy of mainline driver code. I'm pretty sure I explained it to you. I don't want these driver to start differ again. I want to submit all change to mainline version and just copy most recent version to bcm53xx. So instead of this patch against OpenWrt's copy, please send a patch against mainline's nvram.c in the first place. > +char *bcm47xx_nvram_get_contents(size_t *nvram_size) > +{ > + int err; > + char *nvram; > + > + if (!nvram_buf[0]) { > + err = nvram_init(); > + if (err) > + return NULL; > + } > + > + *nvram_size = nvram_len - sizeof(struct nvram_header); > + nvram = vmalloc(*nvram_size); I can see you switched to vmalloc. Just to confirm, I'm OK with either way. > + if (nvram == NULL) > + return NULL; > + memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size); > + > + return nvram; > +} > +EXPORT_SYMBOL(bcm47xx_nvram_get_contents); > + > +void bcm47xx_nvram_release_contents(char *nvram) > +{ > + vfree(nvram); > +} > +EXPORT_SYMBOL(bcm47xx_nvram_release_contents); > + > + > MODULE_LICENSE("GPLv2"); One empty line will be enough.
On 2015-05-12 15:10, Hante Meuleman wrote: > Added two functions to the bcm47xx_nvram module to get and release copies > of the complete nvram contents. This can be used by for example brcmfmac > to get complete nvram contents which will after some parsing be sent to > device. > > Signed-off-by: Hante Meuleman <meuleman@broadcom.com> > --- > diff --git a/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c b/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c > index d0055a4..96d2308 100644 > --- a/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c > +++ b/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c > @@ -222,4 +222,32 @@ int bcm47xx_nvram_gpio_pin(const char *name) > } > EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin); > > +char *bcm47xx_nvram_get_contents(size_t *nvram_size) > +{ > + int err; > + char *nvram; > + > + if (!nvram_buf[0]) { > + err = nvram_init(); > + if (err) > + return NULL; > + } > + > + *nvram_size = nvram_len - sizeof(struct nvram_header); > + nvram = vmalloc(*nvram_size); > + if (nvram == NULL) > + return NULL; > + memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size); > + > + return nvram; > +} > +EXPORT_SYMBOL(bcm47xx_nvram_get_contents); > + > +void bcm47xx_nvram_release_contents(char *nvram) > +{ > + vfree(nvram); > +} > +EXPORT_SYMBOL(bcm47xx_nvram_release_contents); How about making this one inline instead of adding yet another export? - Felix
On Tue, 2015-05-12 at 19:31 +0200, Rafał Miłecki wrote: > On 12 May 2015 at 15:10, Hante Meuleman <meuleman@broadcom.com> wrote: > > Added two functions to the bcm47xx_nvram module to get and release copies > > of the complete nvram contents. This can be used by for example brcmfmac > > to get complete nvram contents which will after some parsing be sent to > > device. > > Thanks for sending this PATCH in a friendly way. It's plain text and > seems to be well formatted, nice! > > Two comments: > > 1) This patch does more than you say. You modify initialization and > reading. Please keep patch per change (and explain them). > > 2) You modify OpenWrt's copy of mainline driver code. I'm pretty sure > I explained it to you. I don't want these driver to start differ > again. I want to submit all change to mainline version and just copy > most recent version to bcm53xx. So instead of this patch against > OpenWrt's copy, please send a patch against mainline's nvram.c in the > first place. > > > > > +char *bcm47xx_nvram_get_contents(size_t *nvram_size) > > +{ > > + int err; > > + char *nvram; > > + > > + if (!nvram_buf[0]) { > > + err = nvram_init(); > > + if (err) > > + return NULL; > > + } > > + > > + *nvram_size = nvram_len - sizeof(struct nvram_header); > > + nvram = vmalloc(*nvram_size); > > I can see you switched to vmalloc. Just to confirm, I'm OK with either way. kmalloc() can fail on larger allocation requests, especially on low memory systems, due to chunk table fragmentation and there's a limit on size, 128k I think. I've seen order 4 allocation failures when trying to kmalloc() 64k chunks in the past. But I also agree that, this being done early in the boot process, those fails shouldn't occur, but nevertheless .... > > > > + if (nvram == NULL) > > + return NULL; > > + memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size); > > + > > + return nvram; > > +} > > +EXPORT_SYMBOL(bcm47xx_nvram_get_contents); > > + > > +void bcm47xx_nvram_release_contents(char *nvram) > > +{ > > + vfree(nvram); > > +} > > +EXPORT_SYMBOL(bcm47xx_nvram_release_contents); > > + > > + > > MODULE_LICENSE("GPLv2"); > > One empty line will be enough. > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
On 13 May 2015 at 03:49, Ian Kent <raven@themaw.net> wrote: > On Tue, 2015-05-12 at 19:31 +0200, Rafał Miłecki wrote: >> On 12 May 2015 at 15:10, Hante Meuleman <meuleman@broadcom.com> wrote: >> > Added two functions to the bcm47xx_nvram module to get and release copies >> > of the complete nvram contents. This can be used by for example brcmfmac >> > to get complete nvram contents which will after some parsing be sent to >> > device. >> >> Thanks for sending this PATCH in a friendly way. It's plain text and >> seems to be well formatted, nice! >> >> Two comments: >> >> 1) This patch does more than you say. You modify initialization and >> reading. Please keep patch per change (and explain them). >> >> 2) You modify OpenWrt's copy of mainline driver code. I'm pretty sure >> I explained it to you. I don't want these driver to start differ >> again. I want to submit all change to mainline version and just copy >> most recent version to bcm53xx. So instead of this patch against >> OpenWrt's copy, please send a patch against mainline's nvram.c in the >> first place. >> >> >> >> > +char *bcm47xx_nvram_get_contents(size_t *nvram_size) >> > +{ >> > + int err; >> > + char *nvram; >> > + >> > + if (!nvram_buf[0]) { >> > + err = nvram_init(); >> > + if (err) >> > + return NULL; >> > + } >> > + >> > + *nvram_size = nvram_len - sizeof(struct nvram_header); >> > + nvram = vmalloc(*nvram_size); >> >> I can see you switched to vmalloc. Just to confirm, I'm OK with either way. > > kmalloc() can fail on larger allocation requests, especially on low > memory systems, due to chunk table fragmentation and there's a limit on > size, 128k I think. > > I've seen order 4 allocation failures when trying to kmalloc() 64k > chunks in the past. But I also agree that, this being done early in the > boot process, those fails shouldn't occur, but nevertheless .... I was thinking about vmalloc vs. returning nvram_buf address :)
On Wed, 2015-05-13 at 06:34 +0200, Rafał Miłecki wrote: > On 13 May 2015 at 03:49, Ian Kent <raven@themaw.net> wrote: > > On Tue, 2015-05-12 at 19:31 +0200, Rafał Miłecki wrote: > >> On 12 May 2015 at 15:10, Hante Meuleman <meuleman@broadcom.com> wrote: > >> > Added two functions to the bcm47xx_nvram module to get and release copies > >> > of the complete nvram contents. This can be used by for example brcmfmac > >> > to get complete nvram contents which will after some parsing be sent to > >> > device. > >> > >> Thanks for sending this PATCH in a friendly way. It's plain text and > >> seems to be well formatted, nice! > >> > >> Two comments: > >> > >> 1) This patch does more than you say. You modify initialization and > >> reading. Please keep patch per change (and explain them). > >> > >> 2) You modify OpenWrt's copy of mainline driver code. I'm pretty sure > >> I explained it to you. I don't want these driver to start differ > >> again. I want to submit all change to mainline version and just copy > >> most recent version to bcm53xx. So instead of this patch against > >> OpenWrt's copy, please send a patch against mainline's nvram.c in the > >> first place. > >> > >> > >> > >> > +char *bcm47xx_nvram_get_contents(size_t *nvram_size) > >> > +{ > >> > + int err; > >> > + char *nvram; > >> > + > >> > + if (!nvram_buf[0]) { > >> > + err = nvram_init(); > >> > + if (err) > >> > + return NULL; > >> > + } > >> > + > >> > + *nvram_size = nvram_len - sizeof(struct nvram_header); > >> > + nvram = vmalloc(*nvram_size); > >> > >> I can see you switched to vmalloc. Just to confirm, I'm OK with either way. > > > > kmalloc() can fail on larger allocation requests, especially on low > > memory systems, due to chunk table fragmentation and there's a limit on > > size, 128k I think. > > > > I've seen order 4 allocation failures when trying to kmalloc() 64k > > chunks in the past. But I also agree that, this being done early in the > > boot process, those fails shouldn't occur, but nevertheless .... > > I was thinking about vmalloc vs. returning nvram_buf address :) Ha, right, still I believe in not using a data structure owned by someone else when you don't "really" know what might be happening with it along the way. Ian
Initially I made the free an inline from header file, but it doesn't compile out of the box as it needs the definition for kfree and not all c files which include this header file had this, as I don't like the inclusion of header files from header files I choose to make an implementation in the c file. -----Original Message----- From: Felix Fietkau [mailto:nbd@openwrt.org] Sent: dinsdag 12 mei 2015 22:03 To: Hante Meuleman; OpenWrt Development List Subject: Re: [OpenWrt-Devel] [PATCH] bcm53xx: Add functions to get/release copies of nvram contents. On 2015-05-12 15:10, Hante Meuleman wrote: > Added two functions to the bcm47xx_nvram module to get and release copies > of the complete nvram contents. This can be used by for example brcmfmac > to get complete nvram contents which will after some parsing be sent to > device. > > Signed-off-by: Hante Meuleman <meuleman@broadcom.com> > --- > diff --git a/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c b/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c > index d0055a4..96d2308 100644 > --- a/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c > +++ b/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c > @@ -222,4 +222,32 @@ int bcm47xx_nvram_gpio_pin(const char *name) > } > EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin); > > +char *bcm47xx_nvram_get_contents(size_t *nvram_size) > +{ > + int err; > + char *nvram; > + > + if (!nvram_buf[0]) { > + err = nvram_init(); > + if (err) > + return NULL; > + } > + > + *nvram_size = nvram_len - sizeof(struct nvram_header); > + nvram = vmalloc(*nvram_size); > + if (nvram == NULL) > + return NULL; > + memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size); > + > + return nvram; > +} > +EXPORT_SYMBOL(bcm47xx_nvram_get_contents); > + > +void bcm47xx_nvram_release_contents(char *nvram) > +{ > + vfree(nvram); > +} > +EXPORT_SYMBOL(bcm47xx_nvram_release_contents); How about making this one inline instead of adding yet another export? - Felix
On 2015-05-13 09:48, Hante Meuleman wrote: > Initially I made the free an inline from header file, but it doesn't compile > out of the box as it needs the definition for kfree and not all c files which > include this header file had this, as I don't like the inclusion of header > files from header files I choose to make an implementation in the c file. I don't understand your reasons at all. How does it make more sense to make the code more bloated instead of placing a simple #include statement (which doesn't cost anything)? - Felix
On 13 May 2015 at 18:45, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-05-13 09:48, Hante Meuleman wrote: >> Initially I made the free an inline from header file, but it doesn't compile >> out of the box as it needs the definition for kfree and not all c files which >> include this header file had this, as I don't like the inclusion of header >> files from header files I choose to make an implementation in the c file. > I don't understand your reasons at all. How does it make more sense to > make the code more bloated instead of placing a simple #include > statement (which doesn't cost anything)? I agree with Felix, I don't think there is anything forbidden about including header in a header :)
On 2015-05-13 20:14, Hante Meuleman wrote:
> Bloated? Seriously? Putting the lines in either C or H and either have an additional include or a prototype (both 1 line).
Having the function in the .c file along with the EXPORT_SYMBOL makes
the kernel size bigger. Including the header file with an inline
function doesn't.
- Felix
On 12 May 2015 at 19:31, Rafał Miłecki <zajec5@gmail.com> wrote: > On 12 May 2015 at 15:10, Hante Meuleman <meuleman@broadcom.com> wrote: >> Added two functions to the bcm47xx_nvram module to get and release copies >> of the complete nvram contents. This can be used by for example brcmfmac >> to get complete nvram contents which will after some parsing be sent to >> device. > > 2) You modify OpenWrt's copy of mainline driver code. I'm pretty sure > I explained it to you. I don't want these driver to start differ > again. I want to submit all change to mainline version and just copy > most recent version to bcm53xx. So instead of this patch against > OpenWrt's copy, please send a patch against mainline's nvram.c in the > first place. Hante, can I expect you to provide appropriate against MIPS upstream-sft tree?
diff --git a/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c b/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c index d0055a4..96d2308 100644 --- a/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c +++ b/target/linux/bcm53xx/files/drivers/firmware/broadcom/bcm47xx_nvram.c @@ -18,6 +18,7 @@ #include <linux/string.h> #include <linux/mtd/mtd.h> #include <linux/bcm47xx_nvram.h> +#include <linux/vmalloc.h> #define NVRAM_MAGIC 0x48534C46 /* 'FLSH' */ #define NVRAM_SPACE 0x10000 @@ -35,6 +36,7 @@ struct nvram_header { }; static char nvram_buf[NVRAM_SPACE]; +static size_t nvram_len; static const u32 nvram_sizes[] = {0x8000, 0xF000, 0x10000}; static u32 find_nvram_size(void __iomem *end) @@ -147,19 +149,19 @@ static int nvram_init(void) 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) { + nvram_len = header.len; + if (nvram_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; + nvram_len = NVRAM_SPACE - 2; } - err = mtd_read(mtd, 0, len, &bytes_read, dst); + err = mtd_read(mtd, 0, nvram_len, &bytes_read, + (u8 *)nvram_buf); if (err) return err; - + nvram_buf[nvram_len] = 0; + nvram_buf[nvram_len + 1] = 0; return 0; } #endif @@ -183,9 +185,7 @@ int bcm47xx_nvram_getenv(const char *name, char *val, size_t val_len) /* Look for name=value and return value */ var = &nvram_buf[sizeof(struct nvram_header)]; - end = nvram_buf + sizeof(nvram_buf) - 2; - end[0] = '\0'; - end[1] = '\0'; + end = nvram_buf + nvram_len; for (; *var; var = value + strlen(value) + 1) { data_left = end - var; @@ -222,4 +222,32 @@ int bcm47xx_nvram_gpio_pin(const char *name) } EXPORT_SYMBOL(bcm47xx_nvram_gpio_pin); +char *bcm47xx_nvram_get_contents(size_t *nvram_size) +{ + int err; + char *nvram; + + if (!nvram_buf[0]) { + err = nvram_init(); + if (err) + return NULL; + } + + *nvram_size = nvram_len - sizeof(struct nvram_header); + nvram = vmalloc(*nvram_size); + if (nvram == NULL) + return NULL; + memcpy(nvram, &nvram_buf[sizeof(struct nvram_header)], *nvram_size); + + return nvram; +} +EXPORT_SYMBOL(bcm47xx_nvram_get_contents); + +void bcm47xx_nvram_release_contents(char *nvram) +{ + vfree(nvram); +} +EXPORT_SYMBOL(bcm47xx_nvram_release_contents); + + MODULE_LICENSE("GPLv2"); diff --git a/target/linux/bcm53xx/files/include/linux/bcm47xx_nvram.h b/target/linux/bcm53xx/files/include/linux/bcm47xx_nvram.h index 0e52a92..2f3f48f 100644 --- a/target/linux/bcm53xx/files/include/linux/bcm47xx_nvram.h +++ b/target/linux/bcm53xx/files/include/linux/bcm47xx_nvram.h @@ -15,6 +15,8 @@ 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); +void bcm47xx_nvram_release_contents(char *nvram); #else static inline int bcm47xx_nvram_init_from_mem(u32 base, u32 lim) { @@ -29,6 +31,13 @@ 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 */
Added two functions to the bcm47xx_nvram module to get and release copies of the complete nvram contents. This can be used by for example brcmfmac to get complete nvram contents which will after some parsing be sent to device. Signed-off-by: Hante Meuleman <meuleman@broadcom.com> ---