All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] nvmem: core: add sysfs cell write support
@ 2024-02-15 21:14 Marco Felsch
  2024-02-16  8:47 ` Michael Walle
  0 siblings, 1 reply; 10+ messages in thread
From: Marco Felsch @ 2024-02-15 21:14 UTC (permalink / raw)
  To: srinivas.kandagatla, gregkh, miquel.raynal, michael, rafal
  Cc: linux-kernel, kernel

Add the sysfs cell write support to make it possible to write to exposed
cells from sysfs as well e.g. for device provisioning.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Hi,

the purpose of this patch is to make the NVMEM cells exposed via the
testing ABI sysfs writeable, to allow changes during devie life-time.

Regards,
  Marco

 drivers/nvmem/core.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 980123fb4dde..5a497188cfea 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	= {
@@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 	struct bin_attribute **cells_attrs, *attrs;
 	struct nvmem_cell_entry *entry;
 	unsigned int ncells = 0, i = 0;
+	umode_t mode;
 	int ret = 0;
 
 	mutex_lock(&nvmem_mutex);
@@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 		goto unlock_mutex;
 	}
 
+	mode = nvmem_bin_attr_get_umode(nvmem);
+
 	/* Initialize each attribute to take the name and size of the cell */
 	list_for_each_entry(entry, &nvmem->cells, node) {
 		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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-15 21:14 [RFC PATCH] nvmem: core: add sysfs cell write support Marco Felsch
@ 2024-02-16  8:47 ` Michael Walle
  2024-02-16 10:07   ` Marco Felsch
  0 siblings, 1 reply; 10+ messages in thread
From: Michael Walle @ 2024-02-16  8:47 UTC (permalink / raw)
  To: Marco Felsch, srinivas.kandagatla, gregkh, miquel.raynal, rafal
  Cc: linux-kernel, kernel

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

Hi,

On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:
> @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
>  	struct bin_attribute **cells_attrs, *attrs;
>  	struct nvmem_cell_entry *entry;
>  	unsigned int ncells = 0, i = 0;
> +	umode_t mode;
>  	int ret = 0;
>  
>  	mutex_lock(&nvmem_mutex);
> @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
>  		goto unlock_mutex;
>  	}
>  
> +	mode = nvmem_bin_attr_get_umode(nvmem);
> +
>  	/* Initialize each attribute to take the name and size of the cell */
>  	list_for_each_entry(entry, &nvmem->cells, node) {
>  		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;

cells are not writable if there is a read post process hook, see
__nvmem_cell_entry_write().

if (entry->read_post_processing)
	mode &= ~0222;

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-16  8:47 ` Michael Walle
@ 2024-02-16 10:07   ` Marco Felsch
  2024-02-19 11:04     ` Miquel Raynal
  0 siblings, 1 reply; 10+ messages in thread
From: Marco Felsch @ 2024-02-16 10:07 UTC (permalink / raw)
  To: Michael Walle
  Cc: srinivas.kandagatla, gregkh, miquel.raynal, rafal, linux-kernel, kernel

Hi Michael,

On 24-02-16, Michael Walle wrote:
> Hi,
> 
> On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:
> > @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> >  	struct bin_attribute **cells_attrs, *attrs;
> >  	struct nvmem_cell_entry *entry;
> >  	unsigned int ncells = 0, i = 0;
> > +	umode_t mode;
> >  	int ret = 0;
> >  
> >  	mutex_lock(&nvmem_mutex);
> > @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> >  		goto unlock_mutex;
> >  	}
> >  
> > +	mode = nvmem_bin_attr_get_umode(nvmem);
> > +
> >  	/* Initialize each attribute to take the name and size of the cell */
> >  	list_for_each_entry(entry, &nvmem->cells, node) {
> >  		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;
> 
> cells are not writable if there is a read post process hook, see
> __nvmem_cell_entry_write().
> 
> if (entry->read_post_processing)
> 	mode &= ~0222;

good point, thanks for the hint :) I will add this and send a non-rfc
version if write-support is something you would like to have.

Regards,
  Marco

> 
> -michael



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-16 10:07   ` Marco Felsch
@ 2024-02-19 11:04     ` Miquel Raynal
  2024-02-19 11:53       ` Marco Felsch
  0 siblings, 1 reply; 10+ messages in thread
From: Miquel Raynal @ 2024-02-19 11:04 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Michael Walle, srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

Hi Marco,

m.felsch@pengutronix.de wrote on Fri, 16 Feb 2024 11:07:50 +0100:

> Hi Michael,
> 
> On 24-02-16, Michael Walle wrote:
> > Hi,
> > 
> > On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:  
> > > @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > >  	struct bin_attribute **cells_attrs, *attrs;
> > >  	struct nvmem_cell_entry *entry;
> > >  	unsigned int ncells = 0, i = 0;
> > > +	umode_t mode;
> > >  	int ret = 0;
> > >  
> > >  	mutex_lock(&nvmem_mutex);
> > > @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > >  		goto unlock_mutex;
> > >  	}
> > >  
> > > +	mode = nvmem_bin_attr_get_umode(nvmem);
> > > +
> > >  	/* Initialize each attribute to take the name and size of the cell */
> > >  	list_for_each_entry(entry, &nvmem->cells, node) {
> > >  		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;  
> > 
> > cells are not writable if there is a read post process hook, see
> > __nvmem_cell_entry_write().
> > 
> > if (entry->read_post_processing)
> > 	mode &= ~0222;  
> 
> good point, thanks for the hint :) I will add this and send a non-rfc
> version if write-support is something you would like to have.

I like the idea but, what about mtd devices (and soon maybe UBI
devices)? This may only work on EEPROM-like devices I guess, where each
area is fully independent and where no erasure is actually expected.

Thanks,
Miquèl

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-19 11:04     ` Miquel Raynal
@ 2024-02-19 11:53       ` Marco Felsch
  2024-02-19 13:26         ` Michael Walle
  0 siblings, 1 reply; 10+ messages in thread
From: Marco Felsch @ 2024-02-19 11:53 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Michael Walle, srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

On 24-02-19, Miquel Raynal wrote:
> Hi Marco,
> 
> m.felsch@pengutronix.de wrote on Fri, 16 Feb 2024 11:07:50 +0100:
> 
> > Hi Michael,
> > 
> > On 24-02-16, Michael Walle wrote:
> > > Hi,
> > > 
> > > On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:  
> > > > @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > >  	struct bin_attribute **cells_attrs, *attrs;
> > > >  	struct nvmem_cell_entry *entry;
> > > >  	unsigned int ncells = 0, i = 0;
> > > > +	umode_t mode;
> > > >  	int ret = 0;
> > > >  
> > > >  	mutex_lock(&nvmem_mutex);
> > > > @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > >  		goto unlock_mutex;
> > > >  	}
> > > >  
> > > > +	mode = nvmem_bin_attr_get_umode(nvmem);
> > > > +
> > > >  	/* Initialize each attribute to take the name and size of the cell */
> > > >  	list_for_each_entry(entry, &nvmem->cells, node) {
> > > >  		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;  
> > > 
> > > cells are not writable if there is a read post process hook, see
> > > __nvmem_cell_entry_write().
> > > 
> > > if (entry->read_post_processing)
> > > 	mode &= ~0222;  
> > 
> > good point, thanks for the hint :) I will add this and send a non-rfc
> > version if write-support is something you would like to have.
> 
> I like the idea but, what about mtd devices (and soon maybe UBI
> devices)? This may only work on EEPROM-like devices I guess, where each
> area is fully independent and where no erasure is actually expected.

For MTD I would say that you need to ensure that you need to align the
cells correctly. The cell-write should handle the page erase/write cycle
properly. E.g. an SPI-NOR need to align the cells to erase-page size or
the nvmem-cell-write need to read-copy-update the cells if they are not
erase-paged aligned.

Regarding UBI(FS) I'm not sure if this is required at all since you have
an filesystem. IMHO nvmem-cells are very lowelevel and are not made for
filesystem backed backends.

That beeing said: I have no problem if we provide write support for
EEPROMs only and adapt it later on to cover spi-nor/nand devices as
well.

Regards,
  Marco

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-19 11:53       ` Marco Felsch
@ 2024-02-19 13:26         ` Michael Walle
  2024-02-20  9:18           ` Miquel Raynal
  0 siblings, 1 reply; 10+ messages in thread
From: Michael Walle @ 2024-02-19 13:26 UTC (permalink / raw)
  To: Marco Felsch, Miquel Raynal
  Cc: srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

On Mon Feb 19, 2024 at 12:53 PM CET, Marco Felsch wrote:
> On 24-02-19, Miquel Raynal wrote:
> > Hi Marco,
> > 
> > m.felsch@pengutronix.de wrote on Fri, 16 Feb 2024 11:07:50 +0100:
> > 
> > > Hi Michael,
> > > 
> > > On 24-02-16, Michael Walle wrote:
> > > > Hi,
> > > > 
> > > > On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:  
> > > > > @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > > >  	struct bin_attribute **cells_attrs, *attrs;
> > > > >  	struct nvmem_cell_entry *entry;
> > > > >  	unsigned int ncells = 0, i = 0;
> > > > > +	umode_t mode;
> > > > >  	int ret = 0;
> > > > >  
> > > > >  	mutex_lock(&nvmem_mutex);
> > > > > @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > > >  		goto unlock_mutex;
> > > > >  	}
> > > > >  
> > > > > +	mode = nvmem_bin_attr_get_umode(nvmem);
> > > > > +
> > > > >  	/* Initialize each attribute to take the name and size of the cell */
> > > > >  	list_for_each_entry(entry, &nvmem->cells, node) {
> > > > >  		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;  
> > > > 
> > > > cells are not writable if there is a read post process hook, see
> > > > __nvmem_cell_entry_write().
> > > > 
> > > > if (entry->read_post_processing)
> > > > 	mode &= ~0222;  
> > > 
> > > good point, thanks for the hint :) I will add this and send a non-rfc
> > > version if write-support is something you would like to have.
> > 
> > I like the idea but, what about mtd devices (and soon maybe UBI
> > devices)? This may only work on EEPROM-like devices I guess, where each
> > area is fully independent and where no erasure is actually expected.
>
> For MTD I would say that you need to ensure that you need to align the
> cells correctly. The cell-write should handle the page erase/write cycle
> properly. E.g. an SPI-NOR need to align the cells to erase-page size or
> the nvmem-cell-write need to read-copy-update the cells if they are not
> erase-paged aligned.
>
> Regarding UBI(FS) I'm not sure if this is required at all since you have
> an filesystem. IMHO nvmem-cells are very lowelevel and are not made for
> filesystem backed backends.
>
> That beeing said: I have no problem if we provide write support for
> EEPROMs only and adapt it later on to cover spi-nor/nand devices as
> well.

Agreed. Honestly, I don't know how much sense this makes for MTD
devices. First, the operation itself, seems really dangerous, as
you'll have to delete the whole sector. Second, during initial
provisioning, I don't think it will make much sense to use the sysfs
cells because you cannot combine multiple writes into one. You'll
always end up with unnecessary erases.

What's your use case here?

Just my two cents..
-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-19 13:26         ` Michael Walle
@ 2024-02-20  9:18           ` Miquel Raynal
  2024-02-20  9:50             ` Marco Felsch
  0 siblings, 1 reply; 10+ messages in thread
From: Miquel Raynal @ 2024-02-20  9:18 UTC (permalink / raw)
  To: Michael Walle
  Cc: Marco Felsch, srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

Hi,

michael@walle.cc wrote on Mon, 19 Feb 2024 14:26:16 +0100:

> On Mon Feb 19, 2024 at 12:53 PM CET, Marco Felsch wrote:
> > On 24-02-19, Miquel Raynal wrote:  
> > > Hi Marco,
> > > 
> > > m.felsch@pengutronix.de wrote on Fri, 16 Feb 2024 11:07:50 +0100:
> > >   
> > > > Hi Michael,
> > > > 
> > > > On 24-02-16, Michael Walle wrote:  
> > > > > Hi,
> > > > > 
> > > > > On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:    
> > > > > > @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > > > >  	struct bin_attribute **cells_attrs, *attrs;
> > > > > >  	struct nvmem_cell_entry *entry;
> > > > > >  	unsigned int ncells = 0, i = 0;
> > > > > > +	umode_t mode;
> > > > > >  	int ret = 0;
> > > > > >  
> > > > > >  	mutex_lock(&nvmem_mutex);
> > > > > > @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > > > >  		goto unlock_mutex;
> > > > > >  	}
> > > > > >  
> > > > > > +	mode = nvmem_bin_attr_get_umode(nvmem);
> > > > > > +
> > > > > >  	/* Initialize each attribute to take the name and size of the cell */
> > > > > >  	list_for_each_entry(entry, &nvmem->cells, node) {
> > > > > >  		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;    
> > > > > 
> > > > > cells are not writable if there is a read post process hook, see
> > > > > __nvmem_cell_entry_write().
> > > > > 
> > > > > if (entry->read_post_processing)
> > > > > 	mode &= ~0222;    
> > > > 
> > > > good point, thanks for the hint :) I will add this and send a non-rfc
> > > > version if write-support is something you would like to have.  
> > > 
> > > I like the idea but, what about mtd devices (and soon maybe UBI
> > > devices)? This may only work on EEPROM-like devices I guess, where each
> > > area is fully independent and where no erasure is actually expected.  
> >
> > For MTD I would say that you need to ensure that you need to align the
> > cells correctly. The cell-write should handle the page erase/write cycle
> > properly. E.g. an SPI-NOR need to align the cells to erase-page size or
> > the nvmem-cell-write need to read-copy-update the cells if they are not
> > erase-paged aligned.
> >
> > Regarding UBI(FS) I'm not sure if this is required at all since you have
> > an filesystem. IMHO nvmem-cells are very lowelevel and are not made for
> > filesystem backed backends.

I'm really talking about UBI, not UBIFS. UBI is just like MTD but
handles wear leveling. There is a pending series for enabling nvmem
cells on top of UBI.

> > That beeing said: I have no problem if we provide write support for
> > EEPROMs only and adapt it later on to cover spi-nor/nand devices as
> > well.  
> 
> Agreed. Honestly, I don't know how much sense this makes for MTD
> devices. First, the operation itself, seems really dangerous, as
> you'll have to delete the whole sector. Second, during initial
> provisioning, I don't think it will make much sense to use the sysfs
> cells because you cannot combine multiple writes into one. You'll
> always end up with unnecessary erases.

One cell per erase block would be an immense waste.
Read-copy-update would probably work but would as well be very
sub-optimal. I guess we could live with it, but as for now there has
not been any real request for it, I'd also advise to keep this feature
out of the mtd world in general.

Thanks,
Miquèl

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-20  9:18           ` Miquel Raynal
@ 2024-02-20  9:50             ` Marco Felsch
  2024-02-20 10:07               ` Miquel Raynal
  2024-02-20 10:16               ` Michael Walle
  0 siblings, 2 replies; 10+ messages in thread
From: Marco Felsch @ 2024-02-20  9:50 UTC (permalink / raw)
  To: Miquel Raynal
  Cc: Michael Walle, srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

Hi Miquel, Michael,

On 24-02-20, Miquel Raynal wrote:
> Hi,
> 
> michael@walle.cc wrote on Mon, 19 Feb 2024 14:26:16 +0100:
> 
> > On Mon Feb 19, 2024 at 12:53 PM CET, Marco Felsch wrote:
> > > On 24-02-19, Miquel Raynal wrote:  
> > > > Hi Marco,
> > > > 
> > > > m.felsch@pengutronix.de wrote on Fri, 16 Feb 2024 11:07:50 +0100:
> > > >   
> > > > > Hi Michael,
> > > > > 
> > > > > On 24-02-16, Michael Walle wrote:  
> > > > > > Hi,
> > > > > > 
> > > > > > On Thu Feb 15, 2024 at 10:14 PM CET, Marco Felsch wrote:    
> > > > > > > @@ -432,6 +466,7 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > > > > >  	struct bin_attribute **cells_attrs, *attrs;
> > > > > > >  	struct nvmem_cell_entry *entry;
> > > > > > >  	unsigned int ncells = 0, i = 0;
> > > > > > > +	umode_t mode;
> > > > > > >  	int ret = 0;
> > > > > > >  
> > > > > > >  	mutex_lock(&nvmem_mutex);
> > > > > > > @@ -456,15 +491,18 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > > > > > >  		goto unlock_mutex;
> > > > > > >  	}
> > > > > > >  
> > > > > > > +	mode = nvmem_bin_attr_get_umode(nvmem);
> > > > > > > +
> > > > > > >  	/* Initialize each attribute to take the name and size of the cell */
> > > > > > >  	list_for_each_entry(entry, &nvmem->cells, node) {
> > > > > > >  		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;    
> > > > > > 
> > > > > > cells are not writable if there is a read post process hook, see
> > > > > > __nvmem_cell_entry_write().
> > > > > > 
> > > > > > if (entry->read_post_processing)
> > > > > > 	mode &= ~0222;    
> > > > > 
> > > > > good point, thanks for the hint :) I will add this and send a non-rfc
> > > > > version if write-support is something you would like to have.  
> > > > 
> > > > I like the idea but, what about mtd devices (and soon maybe UBI
> > > > devices)? This may only work on EEPROM-like devices I guess, where each
> > > > area is fully independent and where no erasure is actually expected.  
> > >
> > > For MTD I would say that you need to ensure that you need to align the
> > > cells correctly. The cell-write should handle the page erase/write cycle
> > > properly. E.g. an SPI-NOR need to align the cells to erase-page size or
> > > the nvmem-cell-write need to read-copy-update the cells if they are not
> > > erase-paged aligned.
> > >
> > > Regarding UBI(FS) I'm not sure if this is required at all since you have
> > > an filesystem. IMHO nvmem-cells are very lowelevel and are not made for
> > > filesystem backed backends.
> 
> I'm really talking about UBI, not UBIFS. UBI is just like MTD but
> handles wear leveling. There is a pending series for enabling nvmem
> cells on top of UBI.

Cells on-top of a wear leveling device? Interesting, the cell-api is
very lowlevel which means the specified cell will be at the exact same
place on the hardware device as specified in the dts. How do you know
that with wear leveling underneath the cell-api?

> > > That beeing said: I have no problem if we provide write support for
> > > EEPROMs only and adapt it later on to cover spi-nor/nand devices as
> > > well.  
> > 
> > Agreed. Honestly, I don't know how much sense this makes for MTD
> > devices. First, the operation itself, seems really dangerous, as
> > you'll have to delete the whole sector. Second, during initial
> > provisioning, I don't think it will make much sense to use the sysfs
> > cells because you cannot combine multiple writes into one. You'll
> > always end up with unnecessary erases.
> 
> One cell per erase block would be an immense waste.

Agree.

> Read-copy-update would probably work but would as well be very
> sub-optimal. I guess we could live with it, but as for now there has
> not been any real request for it, I'd also advise to keep this feature
> out of the mtd world in general.

SPI-NORs are very typical for storing production-data as well but as I
said this is another story. I'm fine with limiting it to EEPROMs since
this is my use-case :)

Regards,
  Marco

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-20  9:50             ` Marco Felsch
@ 2024-02-20 10:07               ` Miquel Raynal
  2024-02-20 10:16               ` Michael Walle
  1 sibling, 0 replies; 10+ messages in thread
From: Miquel Raynal @ 2024-02-20 10:07 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Michael Walle, srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

Hi Marco,

> > > > Regarding UBI(FS) I'm not sure if this is required at all since you have
> > > > an filesystem. IMHO nvmem-cells are very lowelevel and are not made for
> > > > filesystem backed backends.  
> > 
> > I'm really talking about UBI, not UBIFS. UBI is just like MTD but
> > handles wear leveling. There is a pending series for enabling nvmem
> > cells on top of UBI.  
> 
> Cells on-top of a wear leveling device? Interesting, the cell-api is
> very lowlevel which means the specified cell will be at the exact same
> place on the hardware device as specified in the dts. How do you know
> that with wear leveling underneath the cell-api?

https://lore.kernel.org/lkml/cover.1702952891.git.daniel@makrotopia.org/

I haven't tested it though.

Thanks,
Miquèl

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] nvmem: core: add sysfs cell write support
  2024-02-20  9:50             ` Marco Felsch
  2024-02-20 10:07               ` Miquel Raynal
@ 2024-02-20 10:16               ` Michael Walle
  1 sibling, 0 replies; 10+ messages in thread
From: Michael Walle @ 2024-02-20 10:16 UTC (permalink / raw)
  To: Marco Felsch, Miquel Raynal
  Cc: srinivas.kandagatla, gregkh, rafal, linux-kernel, kernel

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On Tue Feb 20, 2024 at 10:50 AM CET, Marco Felsch wrote:
> > Read-copy-update would probably work but would as well be very
> > sub-optimal. I guess we could live with it, but as for now there has
> > not been any real request for it, I'd also advise to keep this feature
> > out of the mtd world in general.
>
> SPI-NORs are very typical for storing production-data as well but as I
> said this is another story. I'm fine with limiting it to EEPROMs since
> this is my use-case :)

Right, that is just what we are doing on our boards. But we do that
in one go in our production environment, like just writing to the
mtd partition or the OTP region. The nvmem cells are then just for
connecting the devices to this information (like the nvmem-cells
property of an ethernet device).

Also usually, there is more to it, like removing write protection of
said flash (sometimes in a proprietary way).

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-02-20 10:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-15 21:14 [RFC PATCH] nvmem: core: add sysfs cell write support Marco Felsch
2024-02-16  8:47 ` Michael Walle
2024-02-16 10:07   ` Marco Felsch
2024-02-19 11:04     ` Miquel Raynal
2024-02-19 11:53       ` Marco Felsch
2024-02-19 13:26         ` Michael Walle
2024-02-20  9:18           ` Miquel Raynal
2024-02-20  9:50             ` Marco Felsch
2024-02-20 10:07               ` Miquel Raynal
2024-02-20 10:16               ` Michael Walle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.