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
> @@ -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ł
