Received-SPF: pass (google.com: domain of linux-kernel+bounces-144082-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sun, 14 Apr 2024 11:48:30 +0200
From: Marco Felsch <m.felsch@pengutronix.de>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: miquel.raynal@bootlin.com, michael@walle.cc,
	linux-kernel@vger.kernel.org, kernel@pengutronix.de,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH] nvmem: core: add sysfs cell write support
Message-ID: <20240414094830.jxpqn33ew2us37t7@pengutronix.de>
References: <20240223154129.1902905-1-m.felsch@pengutronix.de>
 <406eb283-6ac0-4917-9dbf-e45d033bf3de@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <406eb283-6ac0-4917-9dbf-e45d033bf3de@linaro.org>

Hi Srinivas,

+Cc Rob, Krzysztof, Conor for below OF question.

On 24-04-13, Srinivas Kandagatla wrote:
> 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?

An EEPROM device.

> 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.

We do have production data and you're right about this. But we have also
cells which cover sw-feature switches and with the write support they
can be accessed easily from user-space.

> 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'm okay with a Kconfig but I'm not okay with the warning. If an user do
enable this feature on purpose we shouldn't print a warning. We do limit
the write support to EEPROM devices only and to cells which do not have
a special post processing. IMHO this is the simplest use-case and
corruption shouldn't occure. Of course there can be supply
interrruptions but in this case other storage devices can be corrupted
as well.

> 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.

You say instead of checking for NVMEM_TYPE_EEPROM, the nvmem-config
should have an option which to tell the core that write-support should
be exposed? I can do this but still it would expose the write support
for all at24 users. We could have an optional of-property but OF purpose
is to abstract hw and this clearly is not a hw-feature. What I can
imagine is an nvmem_core module param and the default is set via the
Kconfig option. Of course this way still all EEPROMs are either exposed
as ro/rw but it's the user/distro choice. So in the end an OF
abstraction would give us a more fine grained possibility to influence
the behavior.

@Rob, Krzysztof, Conor
Would it be okay to abstract this via an OF property?

> > 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 ?

In addition or as replacement?

> > +		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?

We do use nvmem_bin_attr_get_umode() to query the mode which covers the
read_only, but of course I can add it conditional.

Regards,
  Marco

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