linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents
@ 2015-05-20 11:50 Arend van Spriel
  2015-05-20 12:31 ` Rafał Miłecki
  0 siblings, 1 reply; 5+ messages in thread
From: Arend van Spriel @ 2015-05-20 11:50 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: linux-mips, linux-wireless, Hante Meuleman, Arend van Spriel

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.

Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Daniel (Deognyoun) Kim <dekim@broadcom.com>
Signed-off-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
Made a typo so it did not go the the linux-mips mailing list. Sorry for
the noise.

Regards,
Arend
---
 arch/mips/bcm47xx/nvram.c     | 40 +++++++++++++++++++++++++++++++++-------
 include/linux/bcm47xx_nvram.h | 15 +++++++++++++++
 2 files changed, 48 insertions(+), 7 deletions(-)

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 */
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents
  2015-05-20 11:50 [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents Arend van Spriel
@ 2015-05-20 12:31 ` Rafał Miłecki
  2015-05-20 12:54   ` Arend van Spriel
  0 siblings, 1 reply; 5+ messages in thread
From: Rafał Miłecki @ 2015-05-20 12:31 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: Ralf Baechle, linux-mips, linux-wireless, Hante Meuleman, Hauke Mehrtens

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.

-- 
Rafał

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents
  2015-05-20 12:31 ` Rafał Miłecki
@ 2015-05-20 12:54   ` Arend van Spriel
  2015-05-20 13:48     ` Rafał Miłecki
  0 siblings, 1 reply; 5+ messages in thread
From: Arend van Spriel @ 2015-05-20 12:54 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Ralf Baechle, linux-mips, linux-wireless, Hante Meuleman, Hauke Mehrtens

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.
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents
  2015-05-20 12:54   ` Arend van Spriel
@ 2015-05-20 13:48     ` Rafał Miłecki
  2015-05-20 14:17       ` Arend van Spriel
  0 siblings, 1 reply; 5+ messages in thread
From: Rafał Miłecki @ 2015-05-20 13:48 UTC (permalink / raw)
  To: Arend van Spriel
  Cc: Ralf Baechle, linux-mips, linux-wireless, Hante Meuleman, Hauke Mehrtens

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents
  2015-05-20 13:48     ` Rafał Miłecki
@ 2015-05-20 14:17       ` Arend van Spriel
  0 siblings, 0 replies; 5+ messages in thread
From: Arend van Spriel @ 2015-05-20 14:17 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Ralf Baechle, linux-mips, linux-wireless, Hante Meuleman, Hauke Mehrtens

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-05-20 14:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-20 11:50 [PATCH RESEND] mips: bcm47xx: allow retrieval of complete nvram contents Arend van Spriel
2015-05-20 12:31 ` Rafał Miłecki
2015-05-20 12:54   ` Arend van Spriel
2015-05-20 13:48     ` Rafał Miłecki
2015-05-20 14:17       ` Arend van Spriel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).