[PATCH] nvmem: core: add sysfs cell write support

[PATCH] nvmem: core: add sysfs cell write support

[Marco Felsch]

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
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)
+		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;
+		attrs[i].write = &nvmem_cell_attr_write;
 		attrs[i].private = entry;
 		if (!attrs[i].attr.name) {
 			ret = -ENOMEM;
-- 
2.39.2

Re: [PATCH] nvmem: core: add sysfs cell write support

[Marco Felsch]

Hi,

gentle ping.

Regards,
  Marco

On 24-02-23, 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
> 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)
> +		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;
> +		attrs[i].write = &nvmem_cell_attr_write;
>  		attrs[i].private = entry;
>  		if (!attrs[i].attr.name) {
>  			ret = -ENOMEM;
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH] nvmem: core: add sysfs cell write support

[Srinivas Kandagatla]

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

Re: [PATCH] nvmem: core: add sysfs cell write support

[Marco Felsch]

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
>