From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Fri, 29 May 2015 18:57:22 +0200 (CEST) Received: from mail-gw2-out.broadcom.com ([216.31.210.63]:63541 "EHLO mail-gw2-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27007676AbbE2Q5UFIaJh (ORCPT ); Fri, 29 May 2015 18:57:20 +0200 X-IronPort-AV: E=Sophos;i="5.13,518,1427785200"; d="scan'208";a="66145724" 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 10:08:36 -0700 Received: from IRVEXCHSMTP2.corp.ad.broadcom.com (10.9.207.52) by IRVEXCHCAS08.corp.ad.broadcom.com (10.9.208.57) with Microsoft SMTP Server (TLS) id 14.3.235.1; Fri, 29 May 2015 09:57:13 -0700 Received: from mail-sj1-12.sj.broadcom.com (10.10.10.20) by IRVEXCHSMTP2.corp.ad.broadcom.com (10.9.207.52) with Microsoft SMTP Server id 14.3.235.1; Fri, 29 May 2015 09:57:12 -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 09DE627A81; Fri, 29 May 2015 09:57:11 -0700 (PDT) Message-ID: <55689A67.9070904@broadcom.com> Date: Fri, 29 May 2015 18:57:11 +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 CC: Ralf Baechle , , "Hante Meuleman" 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> In-Reply-To: <55687EDA.9070909@hauke-m.de> 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: 47729 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 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. >> + /* 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]:63541 "EHLO mail-gw2-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27007676AbbE2Q5UFIaJh (ORCPT ); Fri, 29 May 2015 18:57:20 +0200 Message-ID: <55689A67.9070904@broadcom.com> Date: Fri, 29 May 2015 18:57:11 +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> In-Reply-To: <55687EDA.9070909@hauke-m.de> 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: Hauke Mehrtens Cc: Ralf Baechle , linux-mips@linux-mips.org, Hante Meuleman Message-ID: <20150529165711.k9ntrxfQ_kJkgVvCMzCER-dni9wLYjb6ouf3G89lW5o@z> 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. >> + /* 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 */ >>