diff mbox series

[mips/linux.git] firmware: bcm47xx_nvram: refactor finding & reading NVRAM

Message ID 20210304072357.31108-1-zajec5@gmail.com (mailing list archive)
State Superseded
Headers show
Series [mips/linux.git] firmware: bcm47xx_nvram: refactor finding & reading NVRAM | expand

Commit Message

Rafał Miłecki March 4, 2021, 7:23 a.m. UTC 
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.
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 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>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 111 ++++++++++++----------
 1 file changed, 63 insertions(+), 48 deletions(-)

Comments

kernel test robot March 4, 2021, 10:36 a.m. UTC | #1 
Hi "Rafał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.12-rc1 next-20210304]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/firmware-bcm47xx_nvram-refactor-finding-reading-NVRAM/20210304-153024
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f69d02e37a85645aa90d18cacfff36dba370f797
config: x86_64-randconfig-a011-20210304 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project eec7f8f7b1226be422a76542cb403d02538f453a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/10b445f4686138c7c84adb02aa521bbc6bef8ab7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rafa-Mi-ecki/firmware-bcm47xx_nvram-refactor-finding-reading-NVRAM/20210304-153024
        git checkout 10b445f4686138c7c84adb02aa521bbc6bef8ab7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/firmware/broadcom/bcm47xx_nvram.c:41: warning: Function parameter or member 'nvram' not described in 'bcm47xx_nvram_is_valid'
>> drivers/firmware/broadcom/bcm47xx_nvram.c:41: warning: expecting prototype for bcm47xx_nvram_validate(). Prototype was for bcm47xx_nvram_is_valid() instead
   drivers/firmware/broadcom/bcm47xx_nvram.c:49: warning: Function parameter or member 'nvram_start' not described in 'bcm47xx_nvram_copy'
   drivers/firmware/broadcom/bcm47xx_nvram.c:49: warning: Function parameter or member 'res_size' not described in 'bcm47xx_nvram_copy'
   drivers/firmware/broadcom/bcm47xx_nvram.c:73: warning: Function parameter or member 'flash_start' not described in 'bcm47xx_nvram_find_and_copy'
   drivers/firmware/broadcom/bcm47xx_nvram.c:73: warning: Function parameter or member 'res_size' not described in 'bcm47xx_nvram_find_and_copy'


vim +41 drivers/firmware/broadcom/bcm47xx_nvram.c

121915c4ee0812 arch/mips/bcm47xx/nvram.c                 Waldemar Brodkorb 2010-06-08  36  
10b445f4686138 drivers/firmware/broadcom/bcm47xx_nvram.c Rafał Miłecki     2021-03-04  37  /**
10b445f4686138 drivers/firmware/broadcom/bcm47xx_nvram.c Rafał Miłecki     2021-03-04  38   * bcm47xx_nvram_validate - check for a valid NVRAM at specified memory
10b445f4686138 drivers/firmware/broadcom/bcm47xx_nvram.c Rafał Miłecki     2021-03-04  39   */
10b445f4686138 drivers/firmware/broadcom/bcm47xx_nvram.c Rafał Miłecki     2021-03-04  40  static bool bcm47xx_nvram_is_valid(void __iomem *nvram)
f36738ddfeea02 arch/mips/bcm47xx/nvram.c                 Hauke Mehrtens    2012-12-26 @41  {
10b445f4686138 drivers/firmware/broadcom/bcm47xx_nvram.c Rafał Miłecki     2021-03-04  42  	return ((struct nvram_header *)nvram)->magic == NVRAM_MAGIC;
10b445f4686138 drivers/firmware/broadcom/bcm47xx_nvram.c Rafał Miłecki     2021-03-04  43  }
f36738ddfeea02 arch/mips/bcm47xx/nvram.c                 Hauke Mehrtens    2012-12-26  44  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 835ece9c00f1..7cfe857b3e98 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_validate - 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);