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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Jun 13, 2024 at 03:15:29PM +0200, 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>
> ---
> v2:
> - fix error handling path during nvmem_register
> - drop useless list_empty()
> 
>  drivers/nvmem/core.c | 101 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ee7d4c6301c3..8e91d9c0fc8a 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,101 @@ 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);
> +
> +	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;
> +
> +	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;

You might fail here without unregistering the already registered cells.
Furthermore you free the nvmem device which likely leads to memory
corruptions further down. Your choices are:

- Do not fail here, but only print a warning when devfs_create() fails,
  or:
- properly unregister what you registered beforehand.

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 |