From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Fri, 29 May 2015 20:26:59 +0200 (CEST) Received: from mail-gw2-out.broadcom.com ([216.31.210.63]:47906 "EHLO mail-gw2-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27013206AbbE2S06GolCv (ORCPT ); Fri, 29 May 2015 20:26:58 +0200 X-IronPort-AV: E=Sophos;i="5.13,518,1427785200"; d="scan'208";a="66156139" Received: from irvexchcas08.broadcom.com (HELO IRVEXCHCAS08.corp.ad.broadcom.com) ([10.9.208.57]) by mail-gw2-out.broadcom.com with ESMTP; 29 May 2015 11:38:12 -0700 Received: from IRVEXCHSMTP1.corp.ad.broadcom.com (10.9.207.51) by IRVEXCHCAS08.corp.ad.broadcom.com (10.9.208.57) with Microsoft SMTP Server (TLS) id 14.3.235.1; Fri, 29 May 2015 11:26:48 -0700 Received: from mail-sj1-12.sj.broadcom.com (10.10.10.20) by IRVEXCHSMTP1.corp.ad.broadcom.com (10.9.207.51) with Microsoft SMTP Server id 14.3.235.1; Fri, 29 May 2015 11:26:48 -0700 Received: from [10.176.128.60] (xl-bun-02.bun.broadcom.com [10.176.128.60]) by mail-sj1-12.sj.broadcom.com (Postfix) with ESMTP id D797627A81; Fri, 29 May 2015 11:26:47 -0700 (PDT) Message-ID: <5568AF66.3070403@broadcom.com> Date: Fri, 29 May 2015 20:26:46 +0200 From: Arend van Spriel User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.24) Gecko/20111103 Lightning/1.0b2 Thunderbird/3.1.16 MIME-Version: 1.0 To: , Hauke Mehrtens Subject: Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents References: <1432214843-16057-1-git-send-email-arend@broadcom.com> <55687EDA.9070909@hauke-m.de> <55689A67.9070904@broadcom.com> In-Reply-To: <55689A67.9070904@broadcom.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 47730 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: arend@broadcom.com Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips 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 >>> >>> 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 >>> Reviewed-by: Franky (Zhenhui) Lin >>> Reviewed-by: Pieter-Paul Giesberts >>> Reviewed-by: Daniel (Deognyoun) Kim >>> Signed-off-by: Hante Meuleman >>> Signed-off-by: Arend van Spriel >>> --- >>> 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 >>> #include >>> +#include >>> >>> #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 */ >>> > > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-gw2-out.broadcom.com ([216.31.210.63]:47906 "EHLO mail-gw2-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27013206AbbE2S06GolCv (ORCPT ); Fri, 29 May 2015 20:26:58 +0200 Message-ID: <5568AF66.3070403@broadcom.com> Date: Fri, 29 May 2015 20:26:46 +0200 From: Arend van Spriel MIME-Version: 1.0 Subject: Re: [PATCH V2] mips: bcm47xx: allow retrieval of complete nvram contents References: <1432214843-16057-1-git-send-email-arend@broadcom.com> <55687EDA.9070909@hauke-m.de> <55689A67.9070904@broadcom.com> In-Reply-To: <55689A67.9070904@broadcom.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: linux-mips@linux-mips.org, Hauke Mehrtens Message-ID: <20150529182646.cHDbpzT43rE2WyMnrXy-ei7j1f9DK46i7ZBycFHvxlU@z> 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 >>> >>> 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 >>> Reviewed-by: Franky (Zhenhui) Lin >>> Reviewed-by: Pieter-Paul Giesberts >>> Reviewed-by: Daniel (Deognyoun) Kim >>> Signed-off-by: Hante Meuleman >>> Signed-off-by: Arend van Spriel >>> --- >>> 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 >>> #include >>> +#include >>> >>> #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 */ >>> > >