On 05/20/15 14:31, Rafał Miłecki wrote:
On 20 May 2015 at 13:50, Arend van Spriel<arend@xxxxxxxxxxxx> wrote:
From: Hante Meuleman<meuleman@xxxxxxxxxxxx>
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.
