Received-SPF: pass (google.com: domain of linux-kernel+bounces-143679-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <406eb283-6ac0-4917-9dbf-e45d033bf3de@linaro.org>
Date: Sat, 13 Apr 2024 11:35:50 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] nvmem: core: add sysfs cell write support
To: Marco Felsch <m.felsch@pengutronix.de>, miquel.raynal@bootlin.com,
 michael@walle.cc
Cc: linux-kernel@vger.kernel.org, kernel@pengutronix.de
References: <20240223154129.1902905-1-m.felsch@pengutronix.de>
Content-Language: en-US
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
In-Reply-To: <20240223154129.1902905-1-m.felsch@pengutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Thanks Marco  for the work,

On 23/02/2024 15:41, Marco Felsch wrote:
> Add the sysfs cell write support to make it possible to write to exposed
> cells from sysfs as well e.g. for device provisioning. The write support

Which device are you testing this on?

AFAIU, Normally all the device provisioning happens early stages at 
production line, not after OS is up and running. I might be wrong.

Can you provide more details on what type of device provisioning that 
you are referring to.

Write support should not be enabled by default, this has to be an 
explicit Kconfig with a big WARNING that it could potentially corrupt 
the nvmem by rouge writes.

I would also like this to be an optional feature from providers side 
too, as not all nvmem providers want to have device provisioning support 
from Linux side.


> is limited to EEPROM based nvmem devices and to nvmem-cells which don't
> require post-processing.

> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>   drivers/nvmem/core.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 980123fb4dde..b1f86cb431ef 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -336,6 +336,40 @@ static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
>   	return read_len;
>   }
>   
> +static ssize_t nvmem_cell_attr_write(struct file *filp, struct kobject *kobj,
> +				     struct bin_attribute *attr, char *buf,
> +				     loff_t pos, size_t count)
> +{
> +	struct nvmem_cell_entry *entry;
> +	struct nvmem_cell *cell;
> +	int ret;
> +
> +	entry = attr->private;
> +
> +	if (!entry->nvmem->reg_write)

nvmem->read_only ?

> +		return -EPERM;
> +
> +	if (pos >= entry->bytes)
> +		return -EFBIG;
> +
> +	if (pos + count > entry->bytes)
> +		count = entry->bytes - pos;
> +
> +	cell = nvmem_create_cell(entry, entry->name, 0);
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	if (!cell)
> +		return -EINVAL;
> +
> +	ret = nvmem_cell_write(cell, buf, count);
> +
> +	kfree_const(cell->id);
> +	kfree(cell);
> +
> +	return ret;
> +}
> +
>   /* default read/write permissions */
>   static struct bin_attribute bin_attr_rw_nvmem = {
>   	.attr	= {
> @@ -458,13 +492,20 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
>   
>   	/* Initialize each attribute to take the name and size of the cell */
>   	list_for_each_entry(entry, &nvmem->cells, node) {
> +		umode_t mode = nvmem_bin_attr_get_umode(nvmem);
> +
> +		/* Limit cell-write support to EEPROMs at the moment */
> +		if (entry->read_post_process || nvmem->type != NVMEM_TYPE_EEPROM)
> +			mode &= ~0222;
> +
>   		sysfs_bin_attr_init(&attrs[i]);
>   		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
>   						    "%s@%x", entry->name,
>   						    entry->offset);
> -		attrs[i].attr.mode = 0444;
> +		attrs[i].attr.mode = mode;
>   		attrs[i].size = entry->bytes;
>   		attrs[i].read = &nvmem_cell_attr_read;
can we not make this conditional based on read_only flag?

> +		attrs[i].write = &nvmem_cell_attr_write;
>   		attrs[i].private = entry;
>   		if (!attrs[i].attr.name) {
>   			ret = -ENOMEM;

thanks,
Srini