Message-ID: <2025071609-cardigan-polish-aba6@gregkh>
Date: Wed, 16 Jul 2025 14:48:16 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Oleksij Rempel <o.rempel@...gutronix.de>,
	Sebastian Reichel <sre@...nel.org>,
	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>,
	linux-pm@...r.kernel.org,
	Søren Andersen <san@...v.dk>,
	Guenter Roeck <groeck@...omium.org>,
	Matti Vaittinen <mazziesaccount@...il.com>,
	Ahmad Fatoum <a.fatoum@...gutronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v11 5/7] nvmem: add support for device and sysfs-based
 cell lookups

On Wed, Jun 18, 2025 at 02:02:53PM +0200, Oleksij Rempel wrote:
> Introduce new API functions to allow looking up NVMEM devices and cells
> by name, enhancing flexibility in cases where devicetree-based  lookup
> is not available.
> 
> Key changes:
> - Added `nvmem_device_get_by_name()`: Enables retrieving an NVMEM device by
>   its name for systems where direct device reference is needed.
> - Added `nvmem_cell_get_by_sysfs_name()`: Allows retrieving an NVMEM cell
>   based on its sysfs-style name (e.g., "cell@...set,bits"), making it
>   possible to identify cells dynamically.
> - Introduced `nvmem_find_cell_entry_by_sysfs_name()`: A helper function
>   that constructs sysfs-like names and searches for matching cell entries.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> ---
> changes v5:
> - fix build we NVMEM=n
> ---
>  drivers/nvmem/core.c           | 105 +++++++++++++++++++++++++++++++++
>  include/linux/nvmem-consumer.h |  18 ++++++
>  2 files changed, 123 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 59c295a11d86..d310fd8ca9c0 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1177,6 +1177,23 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
>  EXPORT_SYMBOL_GPL(of_nvmem_device_get);
>  #endif
>  
> +/**
> + * nvmem_device_get_by_name - Look up an NVMEM device by its device name
> + * @name: String matching device name in the provider
> + *
> + * Return: A valid pointer to struct nvmem_device on success,
> + * or ERR_PTR(...) on failure. The caller must later call nvmem_device_put() to
> + * release the reference.
> + */
> +struct nvmem_device *nvmem_device_get_by_name(const char *name)
> +{
> +	if (!name)
> +		return ERR_PTR(-EINVAL);
> +
> +	return __nvmem_device_get((void *)name, device_match_name);
> +}
> +EXPORT_SYMBOL_GPL(nvmem_device_get_by_name);
> +
>  /**
>   * nvmem_device_get() - Get nvmem device from a given id
>   *
> @@ -1490,6 +1507,94 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
>  EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>  #endif
>  
> +/**
> + * nvmem_find_cell_entry_by_sysfs_name - Find an NVMEM cell entry by its sysfs
> + *					 name.
> + * @nvmem:      The nvmem_device pointer where the cell is located.
> + * @sysfs_name: The full sysfs cell name, e.g. "mycell@...00,8".
> + *
> + * This function constructs the sysfs-like name for each cell and compares it
> + * to @sysfs_name. If a match is found, the matching nvmem_cell_entry pointer
> + * is returned. This is analogous to nvmem_find_cell_entry_by_name(), except
> + * it matches on the sysfs naming convention used in the device's attributes.
> + *
> + * Return: Pointer to the matching nvmem_cell_entry on success, or NULL if no
> + * match is found.
> + */
> +static struct nvmem_cell_entry *
> +nvmem_find_cell_entry_by_sysfs_name(struct nvmem_device *nvmem,
> +				    const char *sysfs_name)
> +{
> +	struct nvmem_cell_entry *entry;
> +	char tmp[NVMEM_CELL_NAME_MAX];

That's a lot of bytes on the stack, are you sure?

> +
> +	mutex_lock(&nvmem_mutex);

Use a guard?

> +	list_for_each_entry(entry, &nvmem->cells, node) {
> +		int len = snprintf(tmp, NVMEM_CELL_NAME_MAX, "%s@%x,%u",

You have a naming scheme here that you now need to keep in sync with a
naming scheme else where.  Why?  How is that going to happen?

sysfs names are NOT static, or deterministic, and will be totally random
depending on the boot order and the phase of the moon.  Attempting to
look, within the kernel, at a sysfs path name is almost always the sign
of something gone wrong.

Please don't do this, it will only cause headaches and issues over time,
trust me.

> +				   entry->name, entry->offset,
> +				   entry->bit_offset);
> +
> +		if (len >= NVMEM_CELL_NAME_MAX) {
> +			pr_warn("nvmem: cell name too long (max %zu bytes): %s\n",
> +				NVMEM_CELL_NAME_MAX, sysfs_name);

What can a user do about this?  

> +			continue;

Shouldn't you have errored out?

> +		}
> +
> +		if (len < 0) {
> +			pr_warn("nvmem: error formatting cell name\n");
> +			continue;

No error?

> +		}
> +
> +		if (!strcmp(tmp, sysfs_name)) {
> +			mutex_unlock(&nvmem_mutex);
> +			return entry;
> +		}
> +	}
> +
> +	mutex_unlock(&nvmem_mutex);
> +	return NULL;
> +}
> +
> +/**
> + * nvmem_cell_get_by_sysfs_name - Retrieve an NVMEM cell using a sysfs-style
> + *				  name.
> + * @nvmem: Pointer to the `struct nvmem_device` containing the cell.
> + * @sysfs_name: The sysfs-style cell name, formatted as
> + *		"<cell_name>@<offset>,<bits>".

Again, who is keeping this naming scheme in sync?  And what happens when
the firmware changes it?

Please don't.

> + *
> + * This function enables dynamic lookup of NVMEM cells via sysfs-style
> + * identifiers. It is useful when devicetree-based lookup is unavailable or when
> + * cells are managed dynamically.
> + *
> + * Example Usage:
> + *   nvmem_cell_get_by_sysfs_name(nvmem, "mycell@...00,8");
> + *
> + * Return: Pointer to `struct nvmem_cell` on success. On error, an ERR_PTR() is
> + * returned with the appropriate error code.
> + */
> +struct nvmem_cell *nvmem_cell_get_by_sysfs_name(struct nvmem_device *nvmem,
> +						const char *sysfs_name)
> +{
> +	struct nvmem_cell_entry *entry;
> +	struct nvmem_cell *cell;
> +
> +	entry = nvmem_find_cell_entry_by_sysfs_name(nvmem, sysfs_name);
> +	if (!entry)
> +		return ERR_PTR(-ENOENT);
> +
> +	if (!try_module_get(nvmem->owner))
> +		return ERR_PTR(-EINVAL);

Why are you messing with a module owner field, when no other nvmem call
uses that?  Who will decrement it?  Are you sure you didn't just race
with it being unloaded?  module owner fields are almost always wrong
these days.

thanks,

greg k-h

Powered by blists - more mailing lists