nvmem: brcm_nvram: store a copy of NVRAM content

Commit Message

Rafał Miłecki Sept. 14, 2023, 6:49 a.m. UTC

From: Rafał Miłecki <[e-mail]>

This driver uses MMIO access for reading NVRAM from a flash device.
Underneath there is a flash controller that reads data and provides
mapping window.

Using MMIO interface affects controller configuration and may break real
controller driver. It was reported by multiple users of devices with
NVRAM stored on NAND.

Modify driver to read & cache all NVRAM content during init and use that
copy to provide NVMEM data when requested.

Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
Cc: Arınç ÜNAL <[e-mail]>
Cc: Florian Fainelli <[e-mail]>
Cc: Scott Branden <[e-mail]>
Signed-off-by: Rafał Miłecki <[e-mail]>
---
 drivers/nvmem/brcm_nvram.c | 77 +++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 34 deletions(-)

Comments

Arınç ÜNAL Sept. 14, 2023, 7:56 a.m. UTC | #1

On 14.09.2023 09:49, Rafał Miłecki wrote:
> From: Rafał Miłecki <[e-mail]>
> 
> This driver uses MMIO access for reading NVRAM from a flash device.
> Underneath there is a flash controller that reads data and provides
> mapping window.
> 
> Using MMIO interface affects controller configuration and may break real
> controller driver. It was reported by multiple users of devices with
> NVRAM stored on NAND.
> 
> Modify driver to read & cache all NVRAM content during init and use that
> copy to provide NVMEM data when requested.
> 
> Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
> Cc: Arınç ÜNAL <[e-mail]>
> Cc: Florian Fainelli <[e-mail]>
> Cc: Scott Branden <[e-mail]>
> Signed-off-by: Rafał Miłecki <[e-mail]>

This is a bug fix, could you add a "Fixes" tag so it will be backported to
stable releases?

Arınç

Rafał Miłecki Sept. 14, 2023, 8:02 a.m. UTC | #2

On 2023-09-14 09:56, Arınç ÜNAL wrote:
> On 14.09.2023 09:49, Rafał Miłecki wrote:
>> From: Rafał Miłecki <[e-mail]>
>> 
>> This driver uses MMIO access for reading NVRAM from a flash device.
>> Underneath there is a flash controller that reads data and provides
>> mapping window.
>> 
>> Using MMIO interface affects controller configuration and may break 
>> real
>> controller driver. It was reported by multiple users of devices with
>> NVRAM stored on NAND.
>> 
>> Modify driver to read & cache all NVRAM content during init and use 
>> that
>> copy to provide NVMEM data when requested.
>> 
>> Link: 
>> https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
>> Cc: Arınç ÜNAL <[e-mail]>
>> Cc: Florian Fainelli <[e-mail]>
>> Cc: Scott Branden <[e-mail]>
>> Signed-off-by: Rafał Miłecki <[e-mail]>
> 
> This is a bug fix, could you add a "Fixes" tag so it will be backported 
> to
> stable releases?

I suppose we could try:
Fixes: 3fef9ed0627a ("nvmem: brcm_nvram: new driver exposing Broadcom's 
NVRAM")

Srini: can you append it while applying if you find it OK?

Florian Fainelli Sept. 14, 2023, 9:26 p.m. UTC | #3

On 9/13/23 23:49, Rafał Miłecki wrote:
> From: Rafał Miłecki <[e-mail]>
> 
> This driver uses MMIO access for reading NVRAM from a flash device.
> Underneath there is a flash controller that reads data and provides
> mapping window.
> 
> Using MMIO interface affects controller configuration and may break real
> controller driver. It was reported by multiple users of devices with
> NVRAM stored on NAND.
> 
> Modify driver to read & cache all NVRAM content during init and use that
> copy to provide NVMEM data when requested.
> 
> Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
> Cc: Arınç ÜNAL <[e-mail]>
> Cc: Florian Fainelli <[e-mail]>
> Cc: Scott Branden <[e-mail]>
> Signed-off-by: Rafał Miłecki <[e-mail]>
> ---
[snip]
> -	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> -	if (IS_ERR(priv->base))
> -		return PTR_ERR(priv->base);
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->size = resource_size(res);
> +
> +	priv->data = devm_kzalloc(dev, priv->size, GFP_KERNEL);

These can conceivably quite big data structures, how about using kvmalloc()?

Scott Branden Sept. 14, 2023, 9:30 p.m. UTC | #4

On Thu, Sep 14, 2023 at 2:26 PM Florian Fainelli <[e-mail]> wrote:
>
> On 9/13/23 23:49, Rafał Miłecki wrote:
> > From: Rafał Miłecki <[e-mail]>
> >
> > This driver uses MMIO access for reading NVRAM from a flash device.
> > Underneath there is a flash controller that reads data and provides
> > mapping window.
> >
> > Using MMIO interface affects controller configuration and may break real
> > controller driver. It was reported by multiple users of devices with
> > NVRAM stored on NAND.
> >
> > Modify driver to read & cache all NVRAM content during init and use that
> > copy to provide NVMEM data when requested.
> >
> > Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
> > Cc: Arınç ÜNAL <[e-mail]>
> > Cc: Florian Fainelli <[e-mail]>
> > Cc: Scott Branden <[e-mail]>
> > Signed-off-by: Rafał Miłecki <[e-mail]>
> > ---
> [snip]
> > -     priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > -     if (IS_ERR(priv->base))
> > -             return PTR_ERR(priv->base);
> > +     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +     if (IS_ERR(base))
> > +             return PTR_ERR(base);
> > +
> > +     priv->size = resource_size(res);
> > +
> > +     priv->data = devm_kzalloc(dev, priv->size, GFP_KERNEL);
>
> These can conceivably quite big data structures, how about using kvmalloc()?
Why do we even need to expose MMIO interface to NAND though?  Why not
always go through the controller driver.  I don't see how the MMIO
access would be used given bad blocks aren't handled?
> --
> Florian
>

Rafał Miłecki Sept. 15, 2023, 7:25 a.m. UTC | #5

On 2023-09-14 23:26, Florian Fainelli wrote:
> On 9/13/23 23:49, Rafał Miłecki wrote:
>> From: Rafał Miłecki <[e-mail]>
>> 
>> This driver uses MMIO access for reading NVRAM from a flash device.
>> Underneath there is a flash controller that reads data and provides
>> mapping window.
>> 
>> Using MMIO interface affects controller configuration and may break 
>> real
>> controller driver. It was reported by multiple users of devices with
>> NVRAM stored on NAND.
>> 
>> Modify driver to read & cache all NVRAM content during init and use 
>> that
>> copy to provide NVMEM data when requested.
>> 
>> Link: 
>> https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
>> Cc: Arınç ÜNAL <[e-mail]>
>> Cc: Florian Fainelli <[e-mail]>
>> Cc: Scott Branden <[e-mail]>
>> Signed-off-by: Rafał Miłecki <[e-mail]>
>> ---
> [snip]
>> -	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> -	if (IS_ERR(priv->base))
>> -		return PTR_ERR(priv->base);
>> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	priv->size = resource_size(res);
>> +
>> +	priv->data = devm_kzalloc(dev, priv->size, GFP_KERNEL);
> 
> These can conceivably quite big data structures, how about using 
> kvmalloc()?

Good idea.

Actual NVRAM data usually doesn't take much space, but it reserves quite 
a lot and we need all of that to full expose NVMEM device content.

Rafał Miłecki Sept. 15, 2023, 7:28 a.m. UTC | #6

On 2023-09-14 23:30, Scott Branden wrote:
> On Thu, Sep 14, 2023 at 2:26 PM Florian Fainelli <[e-mail]> 
> wrote:
>> 
>> On 9/13/23 23:49, Rafał Miłecki wrote:
>> > From: Rafał Miłecki <[e-mail]>
>> >
>> > This driver uses MMIO access for reading NVRAM from a flash device.
>> > Underneath there is a flash controller that reads data and provides
>> > mapping window.
>> >
>> > Using MMIO interface affects controller configuration and may break real
>> > controller driver. It was reported by multiple users of devices with
>> > NVRAM stored on NAND.
>> >
>> > Modify driver to read & cache all NVRAM content during init and use that
>> > copy to provide NVMEM data when requested.
>> >
>> > Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
>> > Cc: Arınç ÜNAL <[e-mail]>
>> > Cc: Florian Fainelli <[e-mail]>
>> > Cc: Scott Branden <[e-mail]>
>> > Signed-off-by: Rafał Miłecki <[e-mail]>
>> > ---
>> [snip]
>> > -     priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> > -     if (IS_ERR(priv->base))
>> > -             return PTR_ERR(priv->base);
>> > +     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>> > +     if (IS_ERR(base))
>> > +             return PTR_ERR(base);
>> > +
>> > +     priv->size = resource_size(res);
>> > +
>> > +     priv->data = devm_kzalloc(dev, priv->size, GFP_KERNEL);
>> 
>> These can conceivably quite big data structures, how about using 
>> kvmalloc()?
> Why do we even need to expose MMIO interface to NAND though?  Why not
> always go through the controller driver.  I don't see how the MMIO
> access would be used given bad blocks aren't handled?

We need to read NVMRAM *early* for booting purposes. Some vendors store
there information about used firmware in case of having main one and
fallback one. That info is required for partitioning which happens
during mtd initialization before we have NAND driver fully initialized.

Scott Branden Sept. 15, 2023, 9:08 a.m. UTC | #7

On Fri, Sep 15, 2023 at 12:28 AM Rafał Miłecki <[e-mail]> wrote:
>
> On 2023-09-14 23:30, Scott Branden wrote:
> > On Thu, Sep 14, 2023 at 2:26 PM Florian Fainelli <[e-mail]>
> > wrote:
> >>
> >> On 9/13/23 23:49, Rafał Miłecki wrote:
> >> > From: Rafał Miłecki <[e-mail]>
> >> >
> >> > This driver uses MMIO access for reading NVRAM from a flash device.
> >> > Underneath there is a flash controller that reads data and provides
> >> > mapping window.
> >> >
> >> > Using MMIO interface affects controller configuration and may break real
> >> > controller driver. It was reported by multiple users of devices with
> >> > NVRAM stored on NAND.
> >> >
> >> > Modify driver to read & cache all NVRAM content during init and use that
> >> > copy to provide NVMEM data when requested.
> >> >
> >> > Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=[e-mail]/
> >> > Cc: Arınç ÜNAL <[e-mail]>
> >> > Cc: Florian Fainelli <[e-mail]>
> >> > Cc: Scott Branden <[e-mail]>
> >> > Signed-off-by: Rafał Miłecki <[e-mail]>
> >> > ---
> >> [snip]
> >> > -     priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> >> > -     if (IS_ERR(priv->base))
> >> > -             return PTR_ERR(priv->base);
> >> > +     base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> >> > +     if (IS_ERR(base))
> >> > +             return PTR_ERR(base);
> >> > +
> >> > +     priv->size = resource_size(res);
> >> > +
> >> > +     priv->data = devm_kzalloc(dev, priv->size, GFP_KERNEL);
> >>
> >> These can conceivably quite big data structures, how about using
> >> kvmalloc()?
> > Why do we even need to expose MMIO interface to NAND though?  Why not
> > always go through the controller driver.  I don't see how the MMIO
> > access would be used given bad blocks aren't handled?
>
> We need to read NVMRAM *early* for booting purposes. Some vendors store
> there information about used firmware in case of having main one and
> fallback one. That info is required for partitioning which happens
> during mtd initialization before we have NAND driver fully initialized.
OK, it would have been nice for the bootloader to sort out and
retrieve any information.  But if you need to access flash in the
kernel before the nand driver is loaded so be it.  Thanks for the
explanation.
>
> --
> Rafał Miłecki

Patch

diff --git a/drivers/nvmem/brcm_nvram.c b/drivers/nvmem/brcm_nvram.c
index 9737104f3b76..cfc1c485e20a 100644
--- a/drivers/nvmem/brcm_nvram.c
+++ b/drivers/nvmem/brcm_nvram.c
@@ -19,7 +19,8 @@ 
 
 struct brcm_nvram {
 	struct device *dev;
-	void __iomem *base;
+	uint8_t *data;
+	size_t size;
 	struct nvmem_cell_info *cells;
 	int ncells;
 };
@@ -36,10 +37,8 @@  static int brcm_nvram_read(void *context, unsigned int offset, void *val,
 			   size_t bytes)
 {
 	struct brcm_nvram *priv = context;
-	u8 *dst = val;
 
-	while (bytes--)
-		*dst++ = readb(priv->base + offset++);
+	memcpy(val, priv->data + offset, bytes);
 
 	return 0;
 }
@@ -68,7 +67,12 @@  static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
 {
 	struct device *dev = priv->dev;
 	char *var, *value, *eq;
+	uint8_t tmp;
 	int idx;
+	int err = 0;
+
+	tmp = priv->data[len - 1];
+	priv->data[len - 1] = '\0';
 
 	priv->ncells = 0;
 	for (var = data + sizeof(struct brcm_nvram_header);
@@ -78,8 +82,10 @@  static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
 	}
 
 	priv->cells = devm_kcalloc(dev, priv->ncells, sizeof(*priv->cells), GFP_KERNEL);
-	if (!priv->cells)
-		return -ENOMEM;
+	if (!priv->cells) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	for (var = data + sizeof(struct brcm_nvram_header), idx = 0;
 	     var < (char *)data + len && *var;
@@ -91,8 +97,10 @@  static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
 		value = eq + 1;
 
 		priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL);
-		if (!priv->cells[idx].name)
-			return -ENOMEM;
+		if (!priv->cells[idx].name) {
+			err = -ENOMEM;
+			goto out;
+		}
 		priv->cells[idx].offset = value - (char *)data;
 		priv->cells[idx].bytes = strlen(value);
 		priv->cells[idx].np = of_get_child_by_name(dev->of_node, priv->cells[idx].name);
@@ -105,40 +113,32 @@  static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
 		}
 	}
 
-	return 0;
+out:
+	priv->data[len - 1] = tmp;
+	return err;
 }
 
 static int brcm_nvram_parse(struct brcm_nvram *priv)
 {
+	struct brcm_nvram_header *header = (struct brcm_nvram_header *)priv->data;
 	struct device *dev = priv->dev;
-	struct brcm_nvram_header header;
-	uint8_t *data;
 	size_t len;
 	int err;
 
-	memcpy_fromio(&header, priv->base, sizeof(header));
-
-	if (memcmp(header.magic, NVRAM_MAGIC, 4)) {
+	if (memcmp(header->magic, NVRAM_MAGIC, 4)) {
 		dev_err(dev, "Invalid NVRAM magic\n");
 		return -EINVAL;
 	}
 
-	len = le32_to_cpu(header.len);
-
-	data = kzalloc(len, GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	memcpy_fromio(data, priv->base, len);
-	data[len - 1] = '\0';
-
-	err = brcm_nvram_add_cells(priv, data, len);
-	if (err) {
-		dev_err(dev, "Failed to add cells: %d\n", err);
-		return err;
+	len = le32_to_cpu(header->len);
+	if (len > priv->size) {
+		dev_err(dev, "NVRAM length (%zd) exceeds mapped size (%zd)\n", len, priv->size);
+		return -EINVAL;
 	}
 
-	kfree(data);
+	err = brcm_nvram_add_cells(priv, priv->data, len);
+	if (err)
+		dev_err(dev, "Failed to add cells: %d\n", err);
 
 	return 0;
 }
@@ -150,8 +150,9 @@  static int brcm_nvram_probe(struct platform_device *pdev)
 		.reg_read = brcm_nvram_read,
 	};
 	struct device *dev = &pdev->dev;
-	struct resource *res;
 	struct brcm_nvram *priv;
+	struct resource *res;
+	void __iomem *base;
 	int err;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -159,21 +160,29 @@  static int brcm_nvram_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	priv->dev = dev;
 
-	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->size = resource_size(res);
+
+	priv->data = devm_kzalloc(dev, priv->size, GFP_KERNEL);
+	if (!priv->data)
+		return -ENOMEM;
+
+	memcpy_fromio(priv->data, base, priv->size);
 
 	err = brcm_nvram_parse(priv);
 	if (err)
 		return err;
 
-	bcm47xx_nvram_init_from_iomem(priv->base, resource_size(res));
+	bcm47xx_nvram_init_from_iomem(base, priv->size);
 
 	config.dev = dev;
 	config.cells = priv->cells;
 	config.ncells = priv->ncells;
 	config.priv = priv;
-	config.size = resource_size(res);
+	config.size = priv->size;
 
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
 }