Re: [PATCH 2/5] nvmem: expose nvmem cells as cdev

[Marco Felsch <m.felsch@pengutronix.de>]

On 24-03-25, Sascha Hauer wrote:
> On Fri, Mar 22, 2024 at 05:45:56PM +0100, Marco Felsch wrote:
> > Expose the nvmem cells via cdevs which is our equivalent to the Linux
> > sysfs exposure. This allows the easier user queries for board code and
> > shell. Keep the Linux function name scheme for
> > nvmem_populate_sysfs_cells() to reduce the diff for nvmem_register()
> > function.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/nvmem/core.c | 109 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 109 insertions(+)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 657025daddb3..b4a29e4b67f3 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -44,6 +44,8 @@ struct nvmem_cell_entry {
> >  	struct device_node	*np;
> >  	struct nvmem_device	*nvmem;
> >  	struct list_head	node;
> > +
> > +	struct cdev		cdev;
> >  };
> >  
> >  struct nvmem_cell {
> > @@ -144,6 +146,107 @@ static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
> >  	return NULL;
> >  }
> >  
> > +static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
> > +					    const char *id, int index);
> > +
> > +static ssize_t nvmem_cell_cdev_read(struct cdev *cdev, void *buf, size_t count,
> > +				    loff_t offset, unsigned long flags)
> > +{
> > +	struct nvmem_cell_entry *entry;
> > +	struct nvmem_cell *cell = NULL;
> > +	size_t cell_sz, read_len;
> > +	void *content;
> > +
> > +	entry = container_of(cdev, struct nvmem_cell_entry, cdev);
> > +	cell = nvmem_create_cell(entry, entry->name, 0);
> > +	if (IS_ERR(cell))
> > +		return PTR_ERR(cell);
> > +
> > +	if (!cell)
> > +		return -EINVAL;
> 
> From looking at the implementation of nvmem_create_cell() I'd say this
> can't happen.

Right, I took it from the Linux implementation and wanted to keep the
diff small. But I can change it.

> > +
> > +	content = nvmem_cell_read(cell, &cell_sz);
> > +	if (IS_ERR(content)) {
> > +		read_len = PTR_ERR(content);
> > +		goto destroy_cell;
> > +	}
> > +
> > +	read_len = min_t(unsigned int, cell_sz - offset, count);
> > +	memcpy(buf, content + offset, read_len);
> > +	kfree(content);
> > +
> > +destroy_cell:
> > +	kfree_const(cell->id);
> > +	kfree(cell);
> > +
> > +	return read_len;
> > +}
> > +
> > +static ssize_t nvmem_cell_cdev_write(struct cdev *cdev, const void *buf, size_t count,
> > +				     loff_t offset, unsigned long flags)
> > +{
> > +	struct nvmem_cell_entry *entry;
> > +	struct nvmem_cell *cell;
> > +	int ret;
> > +
> > +	entry = container_of(cdev, struct nvmem_cell_entry, cdev);
> > +
> > +	if (!entry->nvmem->reg_write)
> > +		return -EPERM;
> > +
> > +	if (offset >= entry->bytes)
> > +		return -EFBIG;
> > +
> > +	if (offset + count > entry->bytes)
> > +		count = entry->bytes - offset;
> > +
> > +	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;
> > +}
> > +
> > +static struct cdev_operations nvmem_cell_chrdev_ops = {
> > +	.read  = nvmem_cell_cdev_read,
> > +	.write = nvmem_cell_cdev_write,
> > +};
> > +
> > +static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> > +{
> > +	struct device *dev = &nvmem->dev;
> > +	struct nvmem_cell_entry *entry;
> > +
> > +	if (list_empty(&nvmem->cells))
> > +		return 0;
> 
> This is unnecessary.

Sure.

> > +
> > +	list_for_each_entry(entry, &nvmem->cells, node) {
> > +		struct cdev *cdev;
> > +		int ret;
> > +
> > +		cdev = &entry->cdev;
> > +		cdev->name = xasprintf("%s.%s", dev_name(dev),
> > +				       kbasename(entry->name));
> > +		cdev->ops = &nvmem_cell_chrdev_ops;
> > +		cdev->dev = dev;
> > +		cdev->size = entry->bytes;
> > +
> > +		ret = devfs_create(cdev);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Can't we just register a cdev when the cell is actually created? Why do
> we iterate over all cells instead?

Reason for me was to align the nvmem_register() function more with the
Linux variant to port other features like layouts more easily and to
import fixes more easily.

> I am looking at the corresponding kernel code and I wonder how
> u-boot-env is supposed to work. In u_boot_env_probe() first
> nvmem_register() is called and nvmem_add_one_cell() for each variable
> afterwards. nvmem_populate_sysfs_cells() is called during
> nvmem_register(), so how are the variables added later are supposed to
> get a sysfs entry?

I think they don't supposed to get an sysfs entry at all since the
uboot_env handling uses the partitions mechanism. To make it work with
the new sysfs interface the u-boot driver need to be changed to an
nvmem-layout driver, which is the new way of abstracting an layout.

> > +	return 0;
> > +}
> > +
> >  static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
> >  {
> >  	list_add_tail(&cell->node, &cell->nvmem->cells);
> > @@ -337,6 +440,12 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  		}
> >  	}
> >  
> > +	rval = nvmem_populate_sysfs_cells(nvmem);
> > +	if (rval) {
> > +		kfree(nvmem);
> 
> It's fine returning an error without cleaning up properly, but freeing
> the memory on an half registered device is leading to memory
> corruptions which must be fixed. We have the same in barebox master
> already:
> 
> >        rval = register_device(&nvmem->dev);
> >        if (rval) {
> >                kfree(nvmem);
> >                return ERR_PTR(rval);
> >        }
> >
> >        if (!config->cdev) {
> >                rval = nvmem_register_cdev(nvmem, config->name);
> >                if (rval) {
> >                        kfree(nvmem);
> 
> Either we unregister the previously registered device before freeing the
> memory or we keep the allocation, but freeing the memory without
> unregistering the device is wrong.

You're right, I'll fix that.

Regards,
  Marco

> 
> >                        return ERR_PTR(rval);
> >                }
> >        }
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>