| Message ID | 20210305055501.13099-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2,mips/linux.git] firmware: bcm47xx_nvram: refactor finding & reading NVRAM | expand |
Hi Rafał, On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote: > > From: Rafał Miłecki <rafal@milecki.pl> > > 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead > of e.g. "iobase", "end") > 2. Always operate on "offset" instead of mix of start, end, size, etc. "instead of a mix" > 3. Add helper checking for NVRAM to avoid duplicating code > 4. Use "found" variable instead of goto > 5. Use simpler checking of offsets and sizes (2 nested loops with > trivial check instead of extra function) This could be a series of trivial patches, why did you choose to make a mixed bag harder to review? > > This change has been tested on BCM4706. Updated code checks the same > offsets as before. Driver still finds & copies NVRAM content. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: Fix comment to match actual function name > Reported-by: kernel test robot <lkp@intel.com> > --- > drivers/firmware/broadcom/bcm47xx_nvram.c | 111 ++++++++++++---------- > 1 file changed, 63 insertions(+), 48 deletions(-)
Hi, On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote: > On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote: >> >> From: Rafał Miłecki <rafal@milecki.pl> >> >> 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead >> of e.g. "iobase", "end") >> 2. Always operate on "offset" instead of mix of start, end, size, etc. > > "instead of a mix" > >> 3. Add helper checking for NVRAM to avoid duplicating code >> 4. Use "found" variable instead of goto >> 5. Use simpler checking of offsets and sizes (2 nested loops with >> trivial check instead of extra function) > > This could be a series of trivial patches, why did you choose to make a mixed > bag harder to review? It's a subjective thing and often a matter of maintainer taste. I can say that after contributing to various Linux subsystems. If you split a similar patch for MTD subsystem you'll get complains about making changes too small & too hard to review (sic!). This isn't a bomb really: 63 insertions(+), 48 deletions(-) That said I admit I don't know MIPS tree habits. Thomas: do you prefer smaller patches in case like this?
On Fri, Mar 5, 2021 at 11:16 AM Rafał Miłecki <rafal@milecki.pl> wrote: > > Hi, > > On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote: > > On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote: > >> > >> From: Rafał Miłecki <rafal@milecki.pl> > >> > >> 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead > >> of e.g. "iobase", "end") > >> 2. Always operate on "offset" instead of mix of start, end, size, etc. > > > > "instead of a mix" > > > >> 3. Add helper checking for NVRAM to avoid duplicating code > >> 4. Use "found" variable instead of goto > >> 5. Use simpler checking of offsets and sizes (2 nested loops with > >> trivial check instead of extra function) > > > > This could be a series of trivial patches, why did you choose to make a mixed > > bag harder to review? > > It's a subjective thing and often a matter of maintainer taste. I can > say that after contributing to various Linux subsystems. If you split a > similar patch for MTD subsystem you'll get complains about making > changes too small & too hard to review (sic!). Fine. MTD subsystem developers are probably smarter than I'm :) > This isn't a bomb really: 63 insertions(+), 48 deletions(-) Too many changes at once for my brain stack doesn't mean others are willing to review it. But to me that means each time I'll have to pass over it while bisecting or reviewing git history I'll suffer the same overflow. Anyway, matter of taste as you said. > > That said I admit I don't know MIPS tree habits. Thomas: do you prefer > smaller patches in case like this?
On 05.03.2021 12:47, Philippe Mathieu-Daudé wrote: > On Fri, Mar 5, 2021 at 11:16 AM Rafał Miłecki <rafal@milecki.pl> wrote: >> On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote: >>> On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote: >>>> >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead >>>> of e.g. "iobase", "end") >>>> 2. Always operate on "offset" instead of mix of start, end, size, etc. >>> >>> "instead of a mix" >>> >>>> 3. Add helper checking for NVRAM to avoid duplicating code >>>> 4. Use "found" variable instead of goto >>>> 5. Use simpler checking of offsets and sizes (2 nested loops with >>>> trivial check instead of extra function) >>> >>> This could be a series of trivial patches, why did you choose to make a mixed >>> bag harder to review? >> >> It's a subjective thing and often a matter of maintainer taste. I can >> say that after contributing to various Linux subsystems. If you split a >> similar patch for MTD subsystem you'll get complains about making >> changes too small & too hard to review (sic!). > > Fine. MTD subsystem developers are probably smarter than I'm :) > >> This isn't a bomb really: 63 insertions(+), 48 deletions(-) > > Too many changes at once for my brain stack doesn't mean others are > willing to review it. But to me that means each time I'll have to pass over > it while bisecting or reviewing git history I'll suffer the same overflow. > Anyway, matter of taste as you said. If I hear another voice for splitting this change into smaller patches I'm 100% happy to do so. Honestly! I just don't know if by splitting I won't annoy other people by making changes too small. Please speak up! :)
On Fri, Mar 05, 2021 at 12:56:55PM +0100, Rafał Miłecki wrote: > On 05.03.2021 12:47, Philippe Mathieu-Daudé wrote: > > On Fri, Mar 5, 2021 at 11:16 AM Rafał Miłecki <rafal@milecki.pl> wrote: > > > On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote: > > > > On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote: > > > > > > > > > > From: Rafał Miłecki <rafal@milecki.pl> > > > > > > > > > > 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead > > > > > of e.g. "iobase", "end") > > > > > 2. Always operate on "offset" instead of mix of start, end, size, etc. > > > > > > > > "instead of a mix" > > > > > > > > > 3. Add helper checking for NVRAM to avoid duplicating code > > > > > 4. Use "found" variable instead of goto > > > > > 5. Use simpler checking of offsets and sizes (2 nested loops with > > > > > trivial check instead of extra function) > > > > > > > > This could be a series of trivial patches, why did you choose to make a mixed > > > > bag harder to review? > > > > > > It's a subjective thing and often a matter of maintainer taste. I can > > > say that after contributing to various Linux subsystems. If you split a > > > similar patch for MTD subsystem you'll get complains about making > > > changes too small & too hard to review (sic!). > > > > Fine. MTD subsystem developers are probably smarter than I'm :) > > > > > This isn't a bomb really: 63 insertions(+), 48 deletions(-) > > > > Too many changes at once for my brain stack doesn't mean others are > > willing to review it. But to me that means each time I'll have to pass over > > it while bisecting or reviewing git history I'll suffer the same overflow. > > Anyway, matter of taste as you said. > > If I hear another voice for splitting this change into smaller patches > I'm 100% happy to do so. Honestly! > > I just don't know if by splitting I won't annoy other people by making > changes too small. > > Please speak up! :) please split it. IMHO the current is patch is hard to review, because of the different changes mixed together. Thank you. Thomas.
On 2021-03-06 09:00, Thomas Bogendoerfer wrote: > On Fri, Mar 05, 2021 at 12:56:55PM +0100, Rafał Miłecki wrote: >> On 05.03.2021 12:47, Philippe Mathieu-Daudé wrote: >> > On Fri, Mar 5, 2021 at 11:16 AM Rafał Miłecki <rafal@milecki.pl> wrote: >> > > On 05.03.2021 10:58, Philippe Mathieu-Daudé wrote: >> > > > On Fri, Mar 5, 2021 at 6:55 AM Rafał Miłecki <zajec5@gmail.com> wrote: >> > > > > >> > > > > From: Rafał Miłecki <rafal@milecki.pl> >> > > > > >> > > > > 1. Use meaningful variable names (e.g. "flash_start", "res_size" instead >> > > > > of e.g. "iobase", "end") >> > > > > 2. Always operate on "offset" instead of mix of start, end, size, etc. >> > > > >> > > > "instead of a mix" >> > > > >> > > > > 3. Add helper checking for NVRAM to avoid duplicating code >> > > > > 4. Use "found" variable instead of goto >> > > > > 5. Use simpler checking of offsets and sizes (2 nested loops with >> > > > > trivial check instead of extra function) >> > > > >> > > > This could be a series of trivial patches, why did you choose to make a mixed >> > > > bag harder to review? >> > > >> > > It's a subjective thing and often a matter of maintainer taste. I can >> > > say that after contributing to various Linux subsystems. If you split a >> > > similar patch for MTD subsystem you'll get complains about making >> > > changes too small & too hard to review (sic!). >> > >> > Fine. MTD subsystem developers are probably smarter than I'm :) >> > >> > > This isn't a bomb really: 63 insertions(+), 48 deletions(-) >> > >> > Too many changes at once for my brain stack doesn't mean others are >> > willing to review it. But to me that means each time I'll have to pass over >> > it while bisecting or reviewing git history I'll suffer the same overflow. >> > Anyway, matter of taste as you said. >> >> If I hear another voice for splitting this change into smaller patches >> I'm 100% happy to do so. Honestly! >> >> I just don't know if by splitting I won't annoy other people by making >> changes too small. >> >> Please speak up! :) > > please split it. IMHO the current is patch is hard to review, because > of the > different changes mixed together. Will do, thank you for comments Philippe, Thomas!
diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c index 835ece9c00f1..b47c80a79358 100644 --- a/drivers/firmware/broadcom/bcm47xx_nvram.c +++ b/drivers/firmware/broadcom/bcm47xx_nvram.c @@ -34,26 +34,47 @@ static char nvram_buf[NVRAM_SPACE]; static size_t nvram_len; static const u32 nvram_sizes[] = {0x6000, 0x8000, 0xF000, 0x10000}; -static u32 find_nvram_size(void __iomem *end) +/** + * bcm47xx_nvram_is_valid - check for a valid NVRAM at specified memory + */ +static bool bcm47xx_nvram_is_valid(void __iomem *nvram) { - struct nvram_header __iomem *header; - int i; + return ((struct nvram_header *)nvram)->magic == NVRAM_MAGIC; +} - for (i = 0; i < ARRAY_SIZE(nvram_sizes); i++) { - header = (struct nvram_header *)(end - nvram_sizes[i]); - if (header->magic == NVRAM_MAGIC) - return nvram_sizes[i]; +/** + * bcm47xx_nvram_copy - copy NVRAM to internal buffer + */ +static void bcm47xx_nvram_copy(void __iomem *nvram_start, size_t res_size) +{ + struct nvram_header __iomem *header = nvram_start; + size_t copy_size; + + copy_size = header->len; + if (copy_size > res_size) { + pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n"); + copy_size = res_size; + } + if (copy_size >= NVRAM_SPACE) { + pr_err("nvram on flash (%zu bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n", + copy_size, NVRAM_SPACE - 1); + copy_size = NVRAM_SPACE - 1; } - return 0; + __ioread32_copy(nvram_buf, nvram_start, DIV_ROUND_UP(copy_size, 4)); + nvram_buf[NVRAM_SPACE - 1] = '\0'; + nvram_len = copy_size; } -/* Probe for NVRAM header */ -static int nvram_find_and_copy(void __iomem *iobase, u32 lim) +/** + * bcm47xx_nvram_find_and_copy - find NVRAM on flash mapping & copy it + */ +static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_size) { - struct nvram_header __iomem *header; - u32 off; - u32 size; + size_t flash_size; + size_t offset; + bool found; + int i; if (nvram_len) { pr_warn("nvram already initialized\n"); @@ -61,49 +82,43 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim) } /* TODO: when nvram is on nand flash check for bad blocks first. */ - off = FLASH_MIN; - while (off <= lim) { - /* Windowed flash access */ - size = find_nvram_size(iobase + off); - if (size) { - header = (struct nvram_header *)(iobase + off - size); - goto found; + + found = false; + + /* Try every possible flash size and check for NVRAM at its end */ + for (flash_size = FLASH_MIN; flash_size <= res_size; flash_size <<= 1) { + for (i = 0; i < ARRAY_SIZE(nvram_sizes); i++) { + offset = flash_size - nvram_sizes[i]; + if (bcm47xx_nvram_is_valid(flash_start + offset)) { + found = true; + break; + } } - off <<= 1; + + if (found) + break; } /* Try embedded NVRAM at 4 KB and 1 KB as last resorts */ - header = (struct nvram_header *)(iobase + 4096); - if (header->magic == NVRAM_MAGIC) { - size = NVRAM_SPACE; - goto found; - } - header = (struct nvram_header *)(iobase + 1024); - if (header->magic == NVRAM_MAGIC) { - size = NVRAM_SPACE; - goto found; + if (!found) { + offset = 4096; + if (bcm47xx_nvram_is_valid(flash_start + offset)) + found = true; } - pr_err("no nvram found\n"); - return -ENXIO; - -found: - __ioread32_copy(nvram_buf, header, sizeof(*header) / 4); - nvram_len = ((struct nvram_header *)(nvram_buf))->len; - if (nvram_len > size) { - pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n"); - nvram_len = size; + if (!found) { + offset = 1024; + if (bcm47xx_nvram_is_valid(flash_start + offset)) + found = true; } - if (nvram_len >= NVRAM_SPACE) { - pr_err("nvram on flash (%zu bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n", - nvram_len, NVRAM_SPACE - 1); - nvram_len = NVRAM_SPACE - 1; + + if (!found) { + pr_err("no nvram found\n"); + return -ENXIO; } - /* proceed reading data after header */ - __ioread32_copy(nvram_buf + sizeof(*header), header + 1, - DIV_ROUND_UP(nvram_len, 4)); - nvram_buf[NVRAM_SPACE - 1] = '\0'; + + bcm47xx_nvram_copy(flash_start + offset, res_size - offset); return 0; } @@ -124,7 +139,7 @@ int bcm47xx_nvram_init_from_mem(u32 base, u32 lim) if (!iobase) return -ENOMEM; - err = nvram_find_and_copy(iobase, lim); + err = bcm47xx_nvram_find_and_copy(iobase, lim); iounmap(iobase);