Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <29d2f463-90c2-4c96-8828-96806679db8e@linaro.org>
Date:   Fri, 8 Dec 2023 11:15:24 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V3] nvmem: brcm_nvram: store a copy of NVRAM content
Content-Language: en-US
To:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>
Cc:     linux-mtd@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>,
        =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        Florian Fainelli <florian.fainelli@broadcom.com>,
        Scott Branden <scott.branden@broadcom.com>
References: <20231102062848.23965-1-zajec5@gmail.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <20231102062848.23965-1-zajec5@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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));
>   }