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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
>
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
>