diff mbox series

[V3] nvmem: brcm_nvram: store a copy of NVRAM content

Message ID 20231102062848.23965-1-zajec5@gmail.com
State New
Headers show
Series [V3] nvmem: brcm_nvram: store a copy of NVRAM content | expand

Commit Message

Rafał Miłecki Nov. 2, 2023, 6:28 a.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

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 NVRAM content during init and use that
copy to provide NVMEM data when requested. On NAND flashes due to their
alignment NVRAM partitions can be quite big (1 MiB and more) while
actual NVRAM content stays quite small (usually 16 to 32 KiB). To avoid
allocating so much memory check for actual data length.

Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=K0EoWXuXcSMkJO1Vy_5vmVqA@mail.gmail.com/
Fixes: 3fef9ed0627a ("nvmem: brcm_nvram: new driver exposing Broadcom's NVRAM")
Cc: Arınç ÜNAL <arinc.unal@arinc9.com>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>
Cc: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
V2: Minialize amount of allocated memory (check for actual data length)
V3: Document missed fields in struct brcm_nvram

 drivers/nvmem/brcm_nvram.c | 134 ++++++++++++++++++++++++++-----------
 1 file changed, 94 insertions(+), 40 deletions(-)

Comments

Srinivas Kandagatla Dec. 8, 2023, 11:15 a.m. UTC | #1 
Thank you for the patch,

On 02/11/2023 06:28, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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 NVRAM content during init and use that
> copy to provide NVMEM data when requested. On NAND flashes due to their
> alignment NVRAM partitions can be quite big (1 MiB and more) while
> actual NVRAM content stays quite small (usually 16 to 32 KiB). To avoid
> allocating so much memory check for actual data length.
> 
> Link: https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=K0EoWXuXcSMkJO1Vy_5vmVqA@mail.gmail.com/
> Fixes: 3fef9ed0627a ("nvmem: brcm_nvram: new driver exposing Broadcom's NVRAM")

Any reason not to add
Cc: <Stable@vger.kernel.org>
?

--srini
> Cc: Arınç ÜNAL <arinc.unal@arinc9.com>
> Cc: Florian Fainelli <florian.fainelli@broadcom.com>
> Cc: Scott Branden <scott.branden@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> V2: Minialize amount of allocated memory (check for actual data length)
> V3: Document missed fields in struct brcm_nvram
> 
>   drivers/nvmem/brcm_nvram.c | 134 ++++++++++++++++++++++++++-----------
>   1 file changed, 94 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/nvmem/brcm_nvram.c b/drivers/nvmem/brcm_nvram.c
> index 9737104f3b76..5cdf339cfbec 100644
> --- a/drivers/nvmem/brcm_nvram.c
> +++ b/drivers/nvmem/brcm_nvram.c
> @@ -17,9 +17,23 @@
>   
>   #define NVRAM_MAGIC			"FLSH"
>   
> +/**
> + * struct brcm_nvram - driver state internal struct
> + *
> + * @dev:		NVMEM device pointer
> + * @nvmem_size:		Size of the whole space available for NVRAM
> + * @data:		NVRAM data copy stored to avoid poking underlaying flash controller
> + * @data_len:		NVRAM data size
> + * @padding_byte:	Padding value used to fill remaining space
> + * @cells:		Array of discovered NVMEM cells
> + * @ncells:		Number of elements in cells
> + */
>   struct brcm_nvram {
>   	struct device *dev;
> -	void __iomem *base;
> +	size_t nvmem_size;
> +	uint8_t *data;
> +	size_t data_len;
> +	uint8_t padding_byte;
>   	struct nvmem_cell_info *cells;
>   	int ncells;
>   };
> @@ -36,10 +50,47 @@ static int brcm_nvram_read(void *context, unsigned int offset, void *val,
>   			   size_t bytes)
>   {
>   	struct brcm_nvram *priv = context;
> -	u8 *dst = val;
> +	size_t to_copy;
> +
> +	if (offset + bytes > priv->data_len)
> +		to_copy = max_t(ssize_t, (ssize_t)priv->data_len - offset, 0);
> +	else
> +		to_copy = bytes;
> +
> +	memcpy(val, priv->data + offset, to_copy);
> +
> +	memset((uint8_t *)val + to_copy, priv->padding_byte, bytes - to_copy);
> +
> +	return 0;
> +}
> +
> +static int brcm_nvram_copy_data(struct brcm_nvram *priv, struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->nvmem_size = resource_size(res);
> +
> +	priv->padding_byte = readb(base + priv->nvmem_size - 1);
> +	for (priv->data_len = priv->nvmem_size;
> +	     priv->data_len;
> +	     priv->data_len--) {
> +		if (readb(base + priv->data_len - 1) != priv->padding_byte)
> +			break;
> +	}
> +	WARN(priv->data_len > SZ_128K, "Unexpected (big) NVRAM size: %zu B\n", priv->data_len);
> +
> +	priv->data = devm_kzalloc(priv->dev, priv->data_len, GFP_KERNEL);
> +	if (!priv->data)
> +		return -ENOMEM;
> +
> +	memcpy_fromio(priv->data, base, priv->data_len);
>   
> -	while (bytes--)
> -		*dst++ = readb(priv->base + offset++);
> +	bcm47xx_nvram_init_from_iomem(base, priv->data_len);
>   
>   	return 0;
>   }
> @@ -67,8 +118,13 @@ static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
>   				size_t len)
>   {
>   	struct device *dev = priv->dev;
> -	char *var, *value, *eq;
> +	char *var, *value;
> +	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,67 +134,68 @@ 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;
>   	     var = value + strlen(value) + 1, idx++) {
> +		char *eq, *name;
> +
>   		eq = strchr(var, '=');
>   		if (!eq)
>   			break;
>   		*eq = '\0';
> +		name = devm_kstrdup(dev, var, GFP_KERNEL);
> +		*eq = '=';
> +		if (!name) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
>   		value = eq + 1;
>   
> -		priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL);
> -		if (!priv->cells[idx].name)
> -			return -ENOMEM;
> +		priv->cells[idx].name = name;
>   		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);
> -		if (!strcmp(var, "et0macaddr") ||
> -		    !strcmp(var, "et1macaddr") ||
> -		    !strcmp(var, "et2macaddr")) {
> +		if (!strcmp(name, "et0macaddr") ||
> +		    !strcmp(name, "et1macaddr") ||
> +		    !strcmp(name, "et2macaddr")) {
>   			priv->cells[idx].raw_len = strlen(value);
>   			priv->cells[idx].bytes = ETH_ALEN;
>   			priv->cells[idx].read_post_process = brcm_nvram_read_post_process_macaddr;
>   		}
>   	}
>   
> -	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->nvmem_size) {
> +		dev_err(dev, "NVRAM length (%zd) exceeds mapped size (%zd)\n", len,
> +			priv->nvmem_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,7 +207,6 @@ 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;
>   	int err;
>   
> @@ -159,21 +215,19 @@ 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);
> +	err = brcm_nvram_copy_data(priv, pdev);
> +	if (err)
> +		return err;
>   
>   	err = brcm_nvram_parse(priv);
>   	if (err)
>   		return err;
>   
> -	bcm47xx_nvram_init_from_iomem(priv->base, resource_size(res));
> -
>   	config.dev = dev;
>   	config.cells = priv->cells;
>   	config.ncells = priv->ncells;
>   	config.priv = priv;
> -	config.size = resource_size(res);
> +	config.size = priv->nvmem_size;
>   
>   	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
>   }
Rafał Miłecki Dec. 8, 2023, 11:25 a.m. UTC | #2 
On 2023-12-08 12:15, Srinivas Kandagatla wrote:
> Thank you for the patch,
> 
> On 02/11/2023 06:28, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> 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 NVRAM content during init and use that
>> copy to provide NVMEM data when requested. On NAND flashes due to 
>> their
>> alignment NVRAM partitions can be quite big (1 MiB and more) while
>> actual NVRAM content stays quite small (usually 16 to 32 KiB). To 
>> avoid
>> allocating so much memory check for actual data length.
>> 
>> Link: 
>> https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=K0EoWXuXcSMkJO1Vy_5vmVqA@mail.gmail.com/
>> Fixes: 3fef9ed0627a ("nvmem: brcm_nvram: new driver exposing 
>> Broadcom's NVRAM")
> 
> Any reason not to add
> Cc: <Stable@vger.kernel.org>
> ?

Not really. I believe stable team would pick this fix anyway (thanks to
the Fixes:) but doing
Cc: stable@vger.kernel.org
is described as strongly preferred.

Do you think you can just ammend my patch while applying it and add
Cc: stable@vger.kernel.org
to it?
Srinivas Kandagatla Dec. 8, 2023, 11:26 a.m. UTC | #3 
On 08/12/2023 11:25, Rafał Miłecki wrote:
> On 2023-12-08 12:15, Srinivas Kandagatla wrote:
>> Thank you for the patch,
>>
>> On 02/11/2023 06:28, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> 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 NVRAM content during init and use that
>>> copy to provide NVMEM data when requested. On NAND flashes due to their
>>> alignment NVRAM partitions can be quite big (1 MiB and more) while
>>> actual NVRAM content stays quite small (usually 16 to 32 KiB). To avoid
>>> allocating so much memory check for actual data length.
>>>
>>> Link: 
>>> https://lore.kernel.org/linux-mtd/CACna6rwf3_9QVjYcM+847biTX=K0EoWXuXcSMkJO1Vy_5vmVqA@mail.gmail.com/
>>> Fixes: 3fef9ed0627a ("nvmem: brcm_nvram: new driver exposing 
>>> Broadcom's NVRAM")
>>
>> Any reason not to add
>> Cc: <Stable@vger.kernel.org>
>> ?
> 
> Not really. I believe stable team would pick this fix anyway (thanks to
> the Fixes:) but doing
> Cc: stable@vger.kernel.org
> is described as strongly preferred.
> 
> Do you think you can just ammend my patch while applying it and add
> Cc: stable@vger.kernel.org
> to it?
Will do.

--srini
>
Srinivas Kandagatla Dec. 8, 2023, 12:42 p.m. UTC | #4 
On Thu, 02 Nov 2023 07:28:48 +0100, Rafał Miłecki wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] nvmem: brcm_nvram: store a copy of NVRAM content
      commit: f019a0e9c19aaa532cb46142e1f333c9daff92ff

Best regards,
diff mbox series

Patch

diff --git a/drivers/nvmem/brcm_nvram.c b/drivers/nvmem/brcm_nvram.c
index 9737104f3b76..5cdf339cfbec 100644
--- a/drivers/nvmem/brcm_nvram.c
+++ b/drivers/nvmem/brcm_nvram.c
@@ -17,9 +17,23 @@ 
 
 #define NVRAM_MAGIC			"FLSH"
 
+/**
+ * struct brcm_nvram - driver state internal struct
+ *
+ * @dev:		NVMEM device pointer
+ * @nvmem_size:		Size of the whole space available for NVRAM
+ * @data:		NVRAM data copy stored to avoid poking underlaying flash controller
+ * @data_len:		NVRAM data size
+ * @padding_byte:	Padding value used to fill remaining space
+ * @cells:		Array of discovered NVMEM cells
+ * @ncells:		Number of elements in cells
+ */
 struct brcm_nvram {
 	struct device *dev;
-	void __iomem *base;
+	size_t nvmem_size;
+	uint8_t *data;
+	size_t data_len;
+	uint8_t padding_byte;
 	struct nvmem_cell_info *cells;
 	int ncells;
 };
@@ -36,10 +50,47 @@  static int brcm_nvram_read(void *context, unsigned int offset, void *val,
 			   size_t bytes)
 {
 	struct brcm_nvram *priv = context;
-	u8 *dst = val;
+	size_t to_copy;
+
+	if (offset + bytes > priv->data_len)
+		to_copy = max_t(ssize_t, (ssize_t)priv->data_len - offset, 0);
+	else
+		to_copy = bytes;
+
+	memcpy(val, priv->data + offset, to_copy);
+
+	memset((uint8_t *)val + to_copy, priv->padding_byte, bytes - to_copy);
+
+	return 0;
+}
+
+static int brcm_nvram_copy_data(struct brcm_nvram *priv, struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *base;
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->nvmem_size = resource_size(res);
+
+	priv->padding_byte = readb(base + priv->nvmem_size - 1);
+	for (priv->data_len = priv->nvmem_size;
+	     priv->data_len;
+	     priv->data_len--) {
+		if (readb(base + priv->data_len - 1) != priv->padding_byte)
+			break;
+	}
+	WARN(priv->data_len > SZ_128K, "Unexpected (big) NVRAM size: %zu B\n", priv->data_len);
+
+	priv->data = devm_kzalloc(priv->dev, priv->data_len, GFP_KERNEL);
+	if (!priv->data)
+		return -ENOMEM;
+
+	memcpy_fromio(priv->data, base, priv->data_len);
 
-	while (bytes--)
-		*dst++ = readb(priv->base + offset++);
+	bcm47xx_nvram_init_from_iomem(base, priv->data_len);
 
 	return 0;
 }
@@ -67,8 +118,13 @@  static int brcm_nvram_add_cells(struct brcm_nvram *priv, uint8_t *data,
 				size_t len)
 {
 	struct device *dev = priv->dev;
-	char *var, *value, *eq;
+	char *var, *value;
+	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,67 +134,68 @@  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;
 	     var = value + strlen(value) + 1, idx++) {
+		char *eq, *name;
+
 		eq = strchr(var, '=');
 		if (!eq)
 			break;
 		*eq = '\0';
+		name = devm_kstrdup(dev, var, GFP_KERNEL);
+		*eq = '=';
+		if (!name) {
+			err = -ENOMEM;
+			goto out;
+		}
 		value = eq + 1;
 
-		priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL);
-		if (!priv->cells[idx].name)
-			return -ENOMEM;
+		priv->cells[idx].name = name;
 		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);
-		if (!strcmp(var, "et0macaddr") ||
-		    !strcmp(var, "et1macaddr") ||
-		    !strcmp(var, "et2macaddr")) {
+		if (!strcmp(name, "et0macaddr") ||
+		    !strcmp(name, "et1macaddr") ||
+		    !strcmp(name, "et2macaddr")) {
 			priv->cells[idx].raw_len = strlen(value);
 			priv->cells[idx].bytes = ETH_ALEN;
 			priv->cells[idx].read_post_process = brcm_nvram_read_post_process_macaddr;
 		}
 	}
 
-	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->nvmem_size) {
+		dev_err(dev, "NVRAM length (%zd) exceeds mapped size (%zd)\n", len,
+			priv->nvmem_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,7 +207,6 @@  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;
 	int err;
 
@@ -159,21 +215,19 @@  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);
+	err = brcm_nvram_copy_data(priv, pdev);
+	if (err)
+		return err;
 
 	err = brcm_nvram_parse(priv);
 	if (err)
 		return err;
 
-	bcm47xx_nvram_init_from_iomem(priv->base, resource_size(res));
-
 	config.dev = dev;
 	config.cells = priv->cells;
 	config.ncells = priv->ncells;
 	config.priv = priv;
-	config.size = resource_size(res);
+	config.size = priv->nvmem_size;
 
 	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
 }