mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Sascha Hauer <s.hauer@pengutronix.de>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 2/5] nvmem: expose nvmem cells as cdev
Date: Mon, 25 Mar 2024 10:49:30 +0100	[thread overview]
Message-ID: <ZgFIqpbUHg1H1A3l@pengutronix.de> (raw)
In-Reply-To: <20240322164559.1768983-2-m.felsch@pengutronix.de>

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.

> +
> +	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.

> +
> +	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?

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?

> +
> +	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.

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



  reply	other threads:[~2024-03-25  9:50 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-22 16:45 [PATCH 1/5] nvmem: sync with linux code base Marco Felsch
2024-03-22 16:45 ` [PATCH 2/5] nvmem: expose nvmem cells as cdev Marco Felsch
2024-03-25  9:49   ` Sascha Hauer [this message]
2024-03-25 10:59     ` Marco Felsch
2024-03-22 16:45 ` [PATCH 3/5] nvmem: constify the write path Marco Felsch
2024-03-25  9:51   ` Sascha Hauer
2024-03-25 10:33     ` Marco Felsch
2024-03-22 16:45 ` [PATCH 4/5] nvmem: allow single and dynamic device ids Marco Felsch
2024-03-22 16:45 ` [PATCH 5/5] eeprom: at24: fix device name handling Marco Felsch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZgFIqpbUHg1H1A3l@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=m.felsch@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox