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@xxxxxxxxxxxxxx>
> > ---
> > 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
>
