[PATCH v2 0/7] nvmem: survive unbind with active consumers

[PATCH v2 0/7] nvmem: survive unbind with active consumers

[Bartosz Golaszewski]

Nvmem is one of the subsystems vulnerable to object life-time issues.
The memory nvmem core dereferences is owned by nvmem providers which can
be unbound at any time and even though nvmem devices themselves are
reference-counted, there's no synchronization with the provider modules.

This typically is not a problem because thanks to fw_devlink, consumers
get synchronously unbound before providers but it's enough to pass
fw_devlink=off over the command line, unbind the nvmem controller with
consumers still holding references to it and try to read/write in order
to see fireworks in the kernel log.

User-space can trigger it too if a device (for instance: i2c eeprom on a
cp2112 USB expander) is unplugged halfway through a long read.

Thankfully the design of nvmem is rather sane so it just needs a bit of
encouragement and synchronization.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Changes in v2:
- add missing SRCU struct cleanup
- improve the teardown path on error in nvmem_register()
- Link to v1: https://lore.kernel.org/r/20260116-nvmem-unbind-v1-0-7bb401ab19a8@oss.qualcomm.com

---
Bartosz Golaszewski (7):
      nvmem: remove unused field from struct nvmem_device
      nvmem: return -EOPNOTSUPP to in-kernel users on missing callbacks
      nvmem: check the return value of gpiod_set_value_cansleep()
      nvmem: simplify locking with guard()
      nvmem: remove unneeded __nvmem_device_put()
      nvmem: split struct nvmem_device into refcounted and provider-owned data
      nvmem: synchronize nvmem device unregistering with SRCU

 drivers/nvmem/core.c      | 295 +++++++++++++++++++++++++---------------------
 drivers/nvmem/internals.h |  18 ++-
 2 files changed, 173 insertions(+), 140 deletions(-)
---
base-commit: d75756d8d78aa50d454b92665896b3cd0d1077bb
change-id: 20260114-nvmem-unbind-673b52fc84a0

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

[PATCH v2 1/7] nvmem: remove unused field from struct nvmem_device

[Bartosz Golaszewski]

The node list_head in struct nvmem_device is unused. Drop it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/internals.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
index 18fed57270e5e3391cf24d5e49836121a53a8cd6..7cbc55f40259fc4315c41979ad8bf75c36bcb056 100644
--- a/drivers/nvmem/internals.h
+++ b/drivers/nvmem/internals.h
@@ -10,7 +10,6 @@
 struct nvmem_device {
 	struct module		*owner;
 	struct device		dev;
-	struct list_head	node;
 	int			stride;
 	int			word_size;
 	int			id;

-- 
2.47.3

[PATCH v2 2/7] nvmem: return -EOPNOTSUPP to in-kernel users on missing callbacks

[Bartosz Golaszewski]

__nvmem_reg_read/write() currently return -EINVAL if the relevant
callback is not present. User-space helpers again check the presence
of the callbacks to see if they should return -EPERM.

Ahead of adding SRCU synchronization: change the error code returned to
in-kernel users to -EOPNOTSUPP which is more indicative of the actual
reason for the failure as well as allows us to translate it easily to
-EPERM in sysfs callbacks without having to dereference the callback
pointers. This will allow us to limit the number of SRCU critical
sections in the follow-up commits.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/core.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 311cb2e5a5c02d2c6979d7c9bbb7f94abdfbdad1..14f583e466c8d95690539bd886fd0c2fdd440998 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -55,10 +55,10 @@ static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 			    void *val, size_t bytes)
 {
-	if (nvmem->reg_read)
-		return nvmem->reg_read(nvmem->priv, offset, val, bytes);
+	if (!nvmem->reg_read)
+		return -EOPNOTSUPP;
 
-	return -EINVAL;
+	return nvmem->reg_read(nvmem->priv, offset, val, bytes);
 }
 
 static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
@@ -66,14 +66,14 @@ static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
 {
 	int ret;
 
-	if (nvmem->reg_write) {
-		gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
-		ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
-		gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
-		return ret;
-	}
+	if (!nvmem->reg_write)
+		return -EOPNOTSUPP;
+
+	gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
+	ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
+	gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
 
-	return -EINVAL;
+	return ret;
 }
 
 static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
@@ -231,13 +231,12 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 
 	count = round_down(count, nvmem->word_size);
 
-	if (!nvmem->reg_read)
-		return -EPERM;
-
 	rc = nvmem_reg_read(nvmem, pos, buf, count);
-
-	if (rc)
+	if (rc) {
+		if (rc == -EOPNOTSUPP)
+			rc = -EPERM;
 		return rc;
+	}
 
 	return count;
 }
@@ -264,13 +263,15 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 
 	count = round_down(count, nvmem->word_size);
 
-	if (!nvmem->reg_write || nvmem->read_only)
+	if (nvmem->read_only)
 		return -EPERM;
 
 	rc = nvmem_reg_write(nvmem, pos, buf, count);
-
-	if (rc)
+	if (rc) {
+		if (rc == -EOPNOTSUPP)
+			rc = -EPERM;
 		return rc;
+	}
 
 	return count;
 }

-- 
2.47.3

[PATCH v2 3/7] nvmem: check the return value of gpiod_set_value_cansleep()

[Bartosz Golaszewski]

GPIO setters now return integer values and can indicate failures in
lower abstraction layers. Check the return values of
gpiod_set_value_cansleep() calls in nvmem core.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/core.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 14f583e466c8d95690539bd886fd0c2fdd440998..924e2e247ac1ad426d73310f1024000ca64e0471 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -64,16 +64,22 @@ static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
 			     void *val, size_t bytes)
 {
-	int ret;
+	int ret, written;
 
 	if (!nvmem->reg_write)
 		return -EOPNOTSUPP;
 
-	gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
-	ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
-	gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
+	ret = gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
+	if (ret)
+		return ret;
 
-	return ret;
+	written = nvmem->reg_write(nvmem->priv, offset, val, bytes);
+
+	ret = gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
+	if (ret)
+		return ret;
+
+	return written;
 }
 
 static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,

-- 
2.47.3

[PATCH v2 4/7] nvmem: simplify locking with guard()

[Bartosz Golaszewski]

Use lock guards from cleanup.h to simplify locking. While at it: add the
missing mutex.h include.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/core.c | 76 ++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 924e2e247ac1ad426d73310f1024000ca64e0471..42827ba2146ce9ba07716ac5973327da6e9426da 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/fs.h>
@@ -13,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/kref.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/nvmem-provider.h>
 #include <linux/gpio/consumer.h>
@@ -468,27 +470,23 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 	const struct bin_attribute **pattrs;
 	struct bin_attribute *attrs;
 	unsigned int ncells = 0, i = 0;
-	int ret = 0;
+	int ret;
 
-	mutex_lock(&nvmem_mutex);
+	guard(mutex)(&nvmem_mutex);
 
 	if (list_empty(&nvmem->cells) || nvmem->sysfs_cells_populated)
-		goto unlock_mutex;
+		return 0;
 
 	/* Allocate an array of attributes with a sentinel */
 	ncells = list_count_nodes(&nvmem->cells);
 	pattrs = devm_kcalloc(&nvmem->dev, ncells + 1,
 			      sizeof(struct bin_attribute *), GFP_KERNEL);
-	if (!pattrs) {
-		ret = -ENOMEM;
-		goto unlock_mutex;
-	}
+	if (!pattrs)
+		return -ENOMEM;
 
 	attrs = devm_kcalloc(&nvmem->dev, ncells, sizeof(struct bin_attribute), GFP_KERNEL);
-	if (!attrs) {
-		ret = -ENOMEM;
-		goto unlock_mutex;
-	}
+	if (!attrs)
+		return -ENOMEM;
 
 	/* Initialize each attribute to take the name and size of the cell */
 	list_for_each_entry(entry, &nvmem->cells, node) {
@@ -501,10 +499,8 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 		attrs[i].size = entry->bytes;
 		attrs[i].read = &nvmem_cell_attr_read;
 		attrs[i].private = entry;
-		if (!attrs[i].attr.name) {
-			ret = -ENOMEM;
-			goto unlock_mutex;
-		}
+		if (!attrs[i].attr.name)
+			return -ENOMEM;
 
 		pattrs[i] = &attrs[i];
 		i++;
@@ -514,13 +510,10 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 
 	ret = device_add_group(&nvmem->dev, &group);
 	if (ret)
-		goto unlock_mutex;
+		return ret;
 
 	nvmem->sysfs_cells_populated = true;
 
-unlock_mutex:
-	mutex_unlock(&nvmem_mutex);
-
 	return ret;
 }
 
@@ -558,9 +551,8 @@ static const struct bus_type nvmem_bus_type = {
 static void nvmem_cell_entry_drop(struct nvmem_cell_entry *cell)
 {
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
-	mutex_lock(&nvmem_mutex);
-	list_del(&cell->node);
-	mutex_unlock(&nvmem_mutex);
+	scoped_guard(mutex, &nvmem_mutex)
+		list_del(&cell->node);
 	of_node_put(cell->np);
 	kfree_const(cell->name);
 	kfree(cell);
@@ -576,9 +568,8 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
 
 static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
 {
-	mutex_lock(&nvmem_mutex);
-	list_add_tail(&cell->node, &cell->nvmem->cells);
-	mutex_unlock(&nvmem_mutex);
+	scoped_guard(mutex, &nvmem_mutex)
+		list_add_tail(&cell->node, &cell->nvmem->cells);
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_ADD, cell);
 }
 
@@ -728,14 +719,14 @@ nvmem_find_cell_entry_by_name(struct nvmem_device *nvmem, const char *cell_id)
 {
 	struct nvmem_cell_entry *iter, *cell = NULL;
 
-	mutex_lock(&nvmem_mutex);
+	guard(mutex)(&nvmem_mutex);
+
 	list_for_each_entry(iter, &nvmem->cells, node) {
 		if (strcmp(cell_id, iter->name) == 0) {
 			cell = iter;
 			break;
 		}
 	}
-	mutex_unlock(&nvmem_mutex);
 
 	return cell;
 }
@@ -1121,11 +1112,11 @@ static struct nvmem_device *__nvmem_device_get(void *data,
 	struct nvmem_device *nvmem = NULL;
 	struct device *dev;
 
-	mutex_lock(&nvmem_mutex);
-	dev = bus_find_device(&nvmem_bus_type, NULL, data, match);
-	if (dev)
-		nvmem = to_nvmem_device(dev);
-	mutex_unlock(&nvmem_mutex);
+	scoped_guard(mutex, &nvmem_mutex) {
+		dev = bus_find_device(&nvmem_bus_type, NULL, data, match);
+		if (dev)
+			nvmem = to_nvmem_device(dev);
+	}
 	if (!nvmem)
 		return ERR_PTR(-EPROBE_DEFER);
 
@@ -1335,7 +1326,7 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 
 	dev_id = dev_name(dev);
 
-	mutex_lock(&nvmem_lookup_mutex);
+	guard(mutex)(&nvmem_mutex);
 
 	list_for_each_entry(lookup, &nvmem_lookup_list, node) {
 		if ((strcmp(lookup->dev_id, dev_id) == 0) &&
@@ -1343,11 +1334,9 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 			/* This is the right entry. */
 			nvmem = __nvmem_device_get((void *)lookup->nvmem_name,
 						   device_match_name);
-			if (IS_ERR(nvmem)) {
+			if (IS_ERR(nvmem))
 				/* Provider may not be registered yet. */
-				cell = ERR_CAST(nvmem);
-				break;
-			}
+				return ERR_CAST(nvmem);
 
 			cell_entry = nvmem_find_cell_entry_by_name(nvmem,
 								   lookup->cell_name);
@@ -1363,7 +1352,6 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 		}
 	}
 
-	mutex_unlock(&nvmem_lookup_mutex);
 	return cell;
 }
 
@@ -1379,14 +1367,14 @@ nvmem_find_cell_entry_by_node(struct nvmem_device *nvmem, struct device_node *np
 {
 	struct nvmem_cell_entry *iter, *cell = NULL;
 
-	mutex_lock(&nvmem_mutex);
+	guard(mutex)(&nvmem_mutex);
+
 	list_for_each_entry(iter, &nvmem->cells, node) {
 		if (np == iter->np) {
 			cell = iter;
 			break;
 		}
 	}
-	mutex_unlock(&nvmem_mutex);
 
 	return cell;
 }
@@ -2123,10 +2111,10 @@ void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries)
 {
 	int i;
 
-	mutex_lock(&nvmem_lookup_mutex);
+	guard(mutex)(&nvmem_mutex);
+
 	for (i = 0; i < nentries; i++)
 		list_add_tail(&entries[i].node, &nvmem_lookup_list);
-	mutex_unlock(&nvmem_lookup_mutex);
 }
 EXPORT_SYMBOL_GPL(nvmem_add_cell_lookups);
 
@@ -2141,10 +2129,10 @@ void nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries)
 {
 	int i;
 
-	mutex_lock(&nvmem_lookup_mutex);
+	guard(mutex)(&nvmem_mutex);
+
 	for (i = 0; i < nentries; i++)
 		list_del(&entries[i].node);
-	mutex_unlock(&nvmem_lookup_mutex);
 }
 EXPORT_SYMBOL_GPL(nvmem_del_cell_lookups);
 

-- 
2.47.3

[PATCH v2 5/7] nvmem: remove unneeded __nvmem_device_put()

[Bartosz Golaszewski]

__nvmem_device_put() is wrapped by nvmem_device_put() but there's no
extra functionality offered by the latter so just fold one into the
other.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/core.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 42827ba2146ce9ba07716ac5973327da6e9426da..30aee8089f1dc65b1fa07dac8fb35e1dbbcd0ce7 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1134,13 +1134,6 @@ static struct nvmem_device *__nvmem_device_get(void *data,
 	return nvmem;
 }
 
-static void __nvmem_device_put(struct nvmem_device *nvmem)
-{
-	put_device(&nvmem->dev);
-	module_put(nvmem->owner);
-	kref_put(&nvmem->refcnt, nvmem_device_release);
-}
-
 #if IS_ENABLED(CONFIG_OF)
 /**
  * of_nvmem_device_get() - Get nvmem device from a given id
@@ -1253,7 +1246,9 @@ EXPORT_SYMBOL_GPL(devm_nvmem_device_put);
  */
 void nvmem_device_put(struct nvmem_device *nvmem)
 {
-	__nvmem_device_put(nvmem);
+	put_device(&nvmem->dev);
+	module_put(nvmem->owner);
+	kref_put(&nvmem->refcnt, nvmem_device_release);
 }
 EXPORT_SYMBOL_GPL(nvmem_device_put);
 
@@ -1341,12 +1336,12 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 			cell_entry = nvmem_find_cell_entry_by_name(nvmem,
 								   lookup->cell_name);
 			if (!cell_entry) {
-				__nvmem_device_put(nvmem);
+				nvmem_device_put(nvmem);
 				cell = ERR_PTR(-ENOENT);
 			} else {
 				cell = nvmem_create_cell(cell_entry, con_id, 0);
 				if (IS_ERR(cell))
-					__nvmem_device_put(nvmem);
+					nvmem_device_put(nvmem);
 			}
 			break;
 		}
@@ -1456,14 +1451,14 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 	ret = nvmem_layout_module_get_optional(nvmem);
 	if (ret) {
 		of_node_put(cell_np);
-		__nvmem_device_put(nvmem);
+		nvmem_device_put(nvmem);
 		return ERR_PTR(ret);
 	}
 
 	cell_entry = nvmem_find_cell_entry_by_node(nvmem, cell_np);
 	of_node_put(cell_np);
 	if (!cell_entry) {
-		__nvmem_device_put(nvmem);
+		nvmem_device_put(nvmem);
 		nvmem_layout_module_put(nvmem);
 		if (nvmem->layout)
 			return ERR_PTR(-EPROBE_DEFER);
@@ -1473,7 +1468,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
 
 	cell = nvmem_create_cell(cell_entry, id, cell_index);
 	if (IS_ERR(cell)) {
-		__nvmem_device_put(nvmem);
+		nvmem_device_put(nvmem);
 		nvmem_layout_module_put(nvmem);
 	}
 
@@ -1588,7 +1583,7 @@ void nvmem_cell_put(struct nvmem_cell *cell)
 		kfree_const(cell->id);
 
 	kfree(cell);
-	__nvmem_device_put(nvmem);
+	nvmem_device_put(nvmem);
 	nvmem_layout_module_put(nvmem);
 }
 EXPORT_SYMBOL_GPL(nvmem_cell_put);

-- 
2.47.3

[PATCH v2 6/7] nvmem: split struct nvmem_device into refcounted and provider-owned data

[Bartosz Golaszewski]

Data owned by the nvmem provider must not be part of the
reference-counted struct nvmem_device. Ahead of protecting that data
with SRCU, move it into a separate structure the address of which is
stored in nvmem_device.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/core.c      | 39 ++++++++++++++++++++++++++++-----------
 drivers/nvmem/internals.h | 14 +++++++++++---
 2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 30aee8089f1dc65b1fa07dac8fb35e1dbbcd0ce7..9ff97330682975ef724b542fee6089fa7cd5414a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -57,25 +57,28 @@ static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 			    void *val, size_t bytes)
 {
-	if (!nvmem->reg_read)
+	struct nvmem_impl *impl = nvmem->impl;
+
+	if (!impl->reg_read)
 		return -EOPNOTSUPP;
 
-	return nvmem->reg_read(nvmem->priv, offset, val, bytes);
+	return impl->reg_read(impl->priv, offset, val, bytes);
 }
 
 static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
 			     void *val, size_t bytes)
 {
+	struct nvmem_impl *impl = nvmem->impl;
 	int ret, written;
 
-	if (!nvmem->reg_write)
+	if (!impl->reg_write)
 		return -EOPNOTSUPP;
 
 	ret = gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
 	if (ret)
 		return ret;
 
-	written = nvmem->reg_write(nvmem->priv, offset, val, bytes);
+	written = impl->reg_write(impl->priv, offset, val, bytes);
 
 	ret = gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
 	if (ret)
@@ -286,6 +289,8 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 
 static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
 {
+	struct nvmem_impl *impl = nvmem->impl;
+
 	umode_t mode = 0400;
 
 	if (!nvmem->root_only)
@@ -294,10 +299,10 @@ static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
 	if (!nvmem->read_only)
 		mode |= 0200;
 
-	if (!nvmem->reg_write)
+	if (!impl->reg_write)
 		mode &= ~0200;
 
-	if (!nvmem->reg_read)
+	if (!impl->reg_read)
 		mode &= ~0444;
 
 	return mode;
@@ -328,6 +333,7 @@ static umode_t nvmem_attr_is_visible(struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct nvmem_device *nvmem = to_nvmem_device(dev);
+	struct nvmem_impl *impl = nvmem->impl;
 
 	/*
 	 * If the device has no .reg_write operation, do not allow
@@ -336,7 +342,7 @@ static umode_t nvmem_attr_is_visible(struct kobject *kobj,
 	 * can be forced into read-write mode using the 'force_ro'
 	 * attribute.
 	 */
-	if (attr == &dev_attr_force_ro.attr && !nvmem->reg_write)
+	if (attr == &dev_attr_force_ro.attr && !impl->reg_write)
 		return 0;	/* Attribute not visible */
 
 	return attr->mode;
@@ -537,6 +543,7 @@ static void nvmem_release(struct device *dev)
 
 	ida_free(&nvmem_ida, nvmem->id);
 	gpiod_put(nvmem->wp_gpio);
+	kfree(nvmem->impl);
 	kfree(nvmem);
 }
 
@@ -898,6 +905,7 @@ EXPORT_SYMBOL_GPL(nvmem_layout_unregister);
 struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 {
 	struct nvmem_device *nvmem;
+	struct nvmem_impl *impl;
 	int rval;
 
 	if (!config->dev)
@@ -910,8 +918,15 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (!nvmem)
 		return ERR_PTR(-ENOMEM);
 
+	impl = kzalloc_obj(*impl, GFP_KERNEL);
+	if (!impl) {
+		kfree(nvmem);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	rval = ida_alloc(&nvmem_ida, GFP_KERNEL);
 	if (rval < 0) {
+		kfree(impl);
 		kfree(nvmem);
 		return ERR_PTR(rval);
 	}
@@ -937,6 +952,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	INIT_LIST_HEAD(&nvmem->cells);
 	nvmem->fixup_dt_cell_info = config->fixup_dt_cell_info;
 
+	impl->priv = config->priv;
+	impl->reg_read = config->reg_read;
+	impl->reg_write = config->reg_write;
+
+	nvmem->impl = impl;
 	nvmem->owner = config->owner;
 	if (!nvmem->owner && config->dev->driver)
 		nvmem->owner = config->dev->driver->owner;
@@ -944,10 +964,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->word_size = config->word_size ?: 1;
 	nvmem->size = config->size;
 	nvmem->root_only = config->root_only;
-	nvmem->priv = config->priv;
 	nvmem->type = config->type;
-	nvmem->reg_read = config->reg_read;
-	nvmem->reg_write = config->reg_write;
 	nvmem->keepout = config->keepout;
 	nvmem->nkeepout = config->nkeepout;
 	if (config->of_node)
@@ -973,7 +990,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		goto err_put_device;
 
 	nvmem->read_only = device_property_present(config->dev, "read-only") ||
-			   config->read_only || !nvmem->reg_write;
+			   config->read_only || !impl->reg_write;
 
 #ifdef CONFIG_NVMEM_SYSFS
 	nvmem->dev.groups = nvmem_dev_groups;
diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
index 7cbc55f40259fc4315c41979ad8bf75c36bcb056..05197074799ff3e2a6720f6552878a9e1354a5c3 100644
--- a/drivers/nvmem/internals.h
+++ b/drivers/nvmem/internals.h
@@ -7,9 +7,20 @@
 #include <linux/nvmem-consumer.h>
 #include <linux/nvmem-provider.h>
 
+/*
+ * Holds data owned by the provider of the nvmem implementation. This goes
+ * away immediately the moment nvmem_unregister() is called.
+ */
+struct nvmem_impl {
+	nvmem_reg_read_t	reg_read;
+	nvmem_reg_write_t	reg_write;
+	void			*priv;
+};
+
 struct nvmem_device {
 	struct module		*owner;
 	struct device		dev;
+	struct nvmem_impl	*impl;
 	int			stride;
 	int			word_size;
 	int			id;
@@ -26,11 +37,8 @@ struct nvmem_device {
 				   struct nvmem_cell_info *cell);
 	const struct nvmem_keepout *keepout;
 	unsigned int		nkeepout;
-	nvmem_reg_read_t	reg_read;
-	nvmem_reg_write_t	reg_write;
 	struct gpio_desc	*wp_gpio;
 	struct nvmem_layout	*layout;
-	void *priv;
 	bool			sysfs_cells_populated;
 };
 

-- 
2.47.3

[PATCH v2 7/7] nvmem: synchronize nvmem device unregistering with SRCU

[Bartosz Golaszewski]

With the provider-owned data split out into a separate structure, we can
now protect it with SRCU.

Protect all dereferences of nvmem->impl with an SRCU read lock.
Synchronize SRCU in nvmem_unregister() after setting the implementation
pointer to NULL. This has the effect of numbing down the device after
nvmem_unregister() returns - it will no longer accept any consumer calls
and return -ENODEV. The actual device will live on for as long as there
are references to it but we will no longer reach into the consumer's
memory which may be gone by this time.

The change has the added benefit of dropping the - now redundant -
reference counting with kref. We are left with a single release()
function depending on the kobject reference counting provided by struct
device.

Nvmem cell entries are destroyed in .release() now as they may be still
dereferenced via the nvmem_cell handles after nvmem_release(). The
actual calls will still go through SRCU and fail with -ENODEV if the
provider is gone.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/nvmem/core.c      | 140 ++++++++++++++++++++++++++--------------------
 drivers/nvmem/internals.h |   5 +-
 2 files changed, 82 insertions(+), 63 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9ff97330682975ef724b542fee6089fa7cd5414a..f80a1ec501d574cb0c3114c6341fd0f05d00a543 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -12,7 +12,6 @@
 #include <linux/fs.h>
 #include <linux/idr.h>
 #include <linux/init.h>
-#include <linux/kref.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/nvmem-consumer.h>
@@ -57,7 +56,12 @@ static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 			    void *val, size_t bytes)
 {
-	struct nvmem_impl *impl = nvmem->impl;
+	struct nvmem_impl *impl;
+
+	guard(srcu)(&nvmem->srcu);
+	impl = rcu_dereference(nvmem->impl);
+	if (!impl)
+		return -ENODEV;
 
 	if (!impl->reg_read)
 		return -EOPNOTSUPP;
@@ -68,9 +72,14 @@ static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
 			     void *val, size_t bytes)
 {
-	struct nvmem_impl *impl = nvmem->impl;
+	struct nvmem_impl *impl;
 	int ret, written;
 
+	guard(srcu)(&nvmem->srcu);
+	impl = rcu_dereference(nvmem->impl);
+	if (!impl)
+		return -ENODEV;
+
 	if (!impl->reg_write)
 		return -EOPNOTSUPP;
 
@@ -289,10 +298,14 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 
 static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
 {
-	struct nvmem_impl *impl = nvmem->impl;
-
+	struct nvmem_impl *impl;
 	umode_t mode = 0400;
 
+	guard(srcu)(&nvmem->srcu);
+	impl = rcu_dereference(nvmem->impl);
+	if (!impl)
+		return 0;
+
 	if (!nvmem->root_only)
 		mode |= 0044;
 
@@ -333,7 +346,12 @@ static umode_t nvmem_attr_is_visible(struct kobject *kobj,
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct nvmem_device *nvmem = to_nvmem_device(dev);
-	struct nvmem_impl *impl = nvmem->impl;
+	struct nvmem_impl *impl;
+
+	guard(srcu)(&nvmem->srcu);
+	impl = rcu_dereference(nvmem->impl);
+	if (!impl)
+		return 0;
 
 	/*
 	 * If the device has no .reg_write operation, do not allow
@@ -460,10 +478,9 @@ static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
 	return 0;
 }
 
-static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem,
-			      const struct nvmem_config *config)
+static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
 {
-	if (config->compat)
+	if (nvmem->flags & FLAG_COMPAT)
 		device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
 }
 
@@ -530,31 +547,12 @@ static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
 {
 	return -ENOSYS;
 }
-static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem,
-				      const struct nvmem_config *config)
+static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
 {
 }
 
 #endif /* CONFIG_NVMEM_SYSFS */
 
-static void nvmem_release(struct device *dev)
-{
-	struct nvmem_device *nvmem = to_nvmem_device(dev);
-
-	ida_free(&nvmem_ida, nvmem->id);
-	gpiod_put(nvmem->wp_gpio);
-	kfree(nvmem->impl);
-	kfree(nvmem);
-}
-
-static const struct device_type nvmem_provider_type = {
-	.release	= nvmem_release,
-};
-
-static const struct bus_type nvmem_bus_type = {
-	.name		= "nvmem",
-};
-
 static void nvmem_cell_entry_drop(struct nvmem_cell_entry *cell)
 {
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
@@ -573,6 +571,26 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
 		nvmem_cell_entry_drop(cell);
 }
 
+static void nvmem_release(struct device *dev)
+{
+	struct nvmem_device *nvmem = to_nvmem_device(dev);
+
+	gpiod_put(nvmem->wp_gpio);
+	nvmem_sysfs_remove_compat(nvmem);
+	nvmem_device_remove_all_cells(nvmem);
+	ida_free(&nvmem_ida, nvmem->id);
+	cleanup_srcu_struct(&nvmem->srcu);
+	kfree(nvmem);
+}
+
+static const struct device_type nvmem_provider_type = {
+	.release	= nvmem_release,
+};
+
+static const struct bus_type nvmem_bus_type = {
+	.name		= "nvmem",
+};
+
 static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
 {
 	scoped_guard(mutex, &nvmem_mutex)
@@ -948,7 +966,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		goto err_put_device;
 	}
 
-	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);
 	nvmem->fixup_dt_cell_info = config->fixup_dt_cell_info;
 
@@ -956,7 +973,12 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	impl->reg_read = config->reg_read;
 	impl->reg_write = config->reg_write;
 
-	nvmem->impl = impl;
+	rval = init_srcu_struct(&nvmem->srcu);
+	if (rval)
+		goto err_put_device;
+
+	rcu_assign_pointer(nvmem->impl, impl);
+
 	nvmem->owner = config->owner;
 	if (!nvmem->owner && config->dev->driver)
 		nvmem->owner = config->dev->driver->owner;
@@ -1011,24 +1033,24 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (config->cells) {
 		rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
 		if (rval)
-			goto err_remove_cells;
+			goto err_put_device;
 	}
 
 	if (config->add_legacy_fixed_of_cells) {
 		rval = nvmem_add_cells_from_legacy_of(nvmem);
 		if (rval)
-			goto err_remove_cells;
+			goto err_put_device;
 	}
 
 	rval = nvmem_add_cells_from_fixed_layout(nvmem);
 	if (rval)
-		goto err_remove_cells;
+		goto err_put_device;
 
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
 
 	rval = device_add(&nvmem->dev);
 	if (rval)
-		goto err_remove_cells;
+		goto err_put_device;
 
 	rval = nvmem_populate_layout(nvmem);
 	if (rval)
@@ -1050,33 +1072,14 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 #endif
 err_remove_dev:
 	device_del(&nvmem->dev);
-err_remove_cells:
-	nvmem_device_remove_all_cells(nvmem);
-	if (config->compat)
-		nvmem_sysfs_remove_compat(nvmem, config);
 err_put_device:
 	put_device(&nvmem->dev);
+	kfree(impl);
 
 	return ERR_PTR(rval);
 }
 EXPORT_SYMBOL_GPL(nvmem_register);
 
-static void nvmem_device_release(struct kref *kref)
-{
-	struct nvmem_device *nvmem;
-
-	nvmem = container_of(kref, struct nvmem_device, refcnt);
-
-	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_REMOVE, nvmem);
-
-	if (nvmem->flags & FLAG_COMPAT)
-		device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
-
-	nvmem_device_remove_all_cells(nvmem);
-	nvmem_destroy_layout(nvmem);
-	device_unregister(&nvmem->dev);
-}
-
 /**
  * nvmem_unregister() - Unregister previously registered nvmem device
  *
@@ -1084,8 +1087,17 @@ static void nvmem_device_release(struct kref *kref)
  */
 void nvmem_unregister(struct nvmem_device *nvmem)
 {
-	if (nvmem)
-		kref_put(&nvmem->refcnt, nvmem_device_release);
+	struct nvmem_impl *impl;
+
+	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_REMOVE, nvmem);
+
+	impl = rcu_replace_pointer(nvmem->impl, NULL, true);
+	synchronize_srcu(&nvmem->srcu);
+
+	nvmem_destroy_layout(nvmem);
+	kfree(impl);
+
+	device_unregister(&nvmem->dev);
 }
 EXPORT_SYMBOL_GPL(nvmem_unregister);
 
@@ -1146,8 +1158,6 @@ static struct nvmem_device *__nvmem_device_get(void *data,
 		return ERR_PTR(-EINVAL);
 	}
 
-	kref_get(&nvmem->refcnt);
-
 	return nvmem;
 }
 
@@ -1263,9 +1273,8 @@ EXPORT_SYMBOL_GPL(devm_nvmem_device_put);
  */
 void nvmem_device_put(struct nvmem_device *nvmem)
 {
-	put_device(&nvmem->dev);
 	module_put(nvmem->owner);
-	kref_put(&nvmem->refcnt, nvmem_device_release);
+	put_device(&nvmem->dev);
 }
 EXPORT_SYMBOL_GPL(nvmem_device_put);
 
@@ -1652,6 +1661,15 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 {
 	int rc;
 
+	/*
+	 * Take the SRCU read lock earlier. It will be taken again in
+	 * nvmem_reg_read() but that's alright, they can be nested. If
+	 * nvmem_reg_read() returns -ENODEV, we'll return right way. If it
+	 * succeeds, we need to stay within the SRCU read-critical section
+	 * until we're done calling cell->read_post_process().
+	 */
+	guard(srcu)(&nvmem->srcu);
+
 	rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->raw_len);
 
 	if (rc)
diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
index 05197074799ff3e2a6720f6552878a9e1354a5c3..5afb1297a93a38e399085391130c4df99f64af16 100644
--- a/drivers/nvmem/internals.h
+++ b/drivers/nvmem/internals.h
@@ -6,6 +6,7 @@
 #include <linux/device.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/nvmem-provider.h>
+#include <linux/srcu.h>
 
 /*
  * Holds data owned by the provider of the nvmem implementation. This goes
@@ -20,11 +21,11 @@ struct nvmem_impl {
 struct nvmem_device {
 	struct module		*owner;
 	struct device		dev;
-	struct nvmem_impl	*impl;
+	struct nvmem_impl __rcu	*impl;
+	struct srcu_struct	srcu;
 	int			stride;
 	int			word_size;
 	int			id;
-	struct kref		refcnt;
 	size_t			size;
 	bool			read_only;
 	bool			root_only;

-- 
2.47.3

Re: [PATCH v2 6/7] nvmem: split struct nvmem_device into refcounted and provider-owned data

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:07AM +0100, Bartosz Golaszewski wrote:
> Data owned by the nvmem provider must not be part of the
> reference-counted struct nvmem_device.

It can be as long as it's not referenced after deregistration so this
claim is too strong.

It can be handled the way you are doing it here, but you could also just
use a boolean flag to indicate that no further callbacks should be made.

> Ahead of protecting that data
> with SRCU, move it into a separate structure the address of which is
> stored in nvmem_device.
 
> +/*
> + * Holds data owned by the provider of the nvmem implementation. This goes
> + * away immediately the moment nvmem_unregister() is called.
> + */
> +struct nvmem_impl {
> +	nvmem_reg_read_t	reg_read;
> +	nvmem_reg_write_t	reg_write;
> +	void			*priv;

And priv is just a pointer that is never used by nvmem core directly.

> +};

How about just wrapping the callbacks and calling the struct nvmem_ops
to make it more clear how this is used?

That is, that nvmem core guarantees that there will be no further
callbacks after deregistration.

>  struct nvmem_device {
>  	struct module		*owner;
>  	struct device		dev;
> +	struct nvmem_impl	*impl;
>  	int			stride;
>  	int			word_size;
>  	int			id;
> @@ -26,11 +37,8 @@ struct nvmem_device {
>  				   struct nvmem_cell_info *cell);
>  	const struct nvmem_keepout *keepout;
>  	unsigned int		nkeepout;
> -	nvmem_reg_read_t	reg_read;
> -	nvmem_reg_write_t	reg_write;
>  	struct gpio_desc	*wp_gpio;
>  	struct nvmem_layout	*layout;
> -	void *priv;
>  	bool			sysfs_cells_populated;
>  };

Johan

Re: [PATCH v2 7/7] nvmem: synchronize nvmem device unregistering with SRCU

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:08AM +0100, Bartosz Golaszewski wrote:
> With the provider-owned data split out into a separate structure, we can
> now protect it with SRCU.
> 
> Protect all dereferences of nvmem->impl with an SRCU read lock.
> Synchronize SRCU in nvmem_unregister() after setting the implementation
> pointer to NULL. This has the effect of numbing down the device after
> nvmem_unregister() returns - it will no longer accept any consumer calls
> and return -ENODEV. The actual device will live on for as long as there
> are references to it but we will no longer reach into the consumer's
> memory which may be gone by this time.
> 
> The change has the added benefit of dropping the - now redundant -
> reference counting with kref. We are left with a single release()
> function depending on the kobject reference counting provided by struct
> device.

You're actually fixing two separate issues here. And one of those issues
stem from the broken kref implementation added by:

	c1de7f43bd84 ("nvmem: use kref")

Sure, the code was already broken (by returning -EBUSY when there were
references) but the above commit deferred deregistration until the last
reference is gone which is just wrong.

So the first issue is the deferred deregistration (kref) which allows
further lookups after the provider is gone and which keeps the sysfs
interface around which can lead to use-after-free.

The second issue is that other driver can try to access the nvmem after
it's gone and that bit you can fix with SRCU. But note that you don't
need that for sysfs as kernfs already drains any active user at
deregistration.

So I think this should be split in two after dropping some of the
unnecessary sysfs bits.

> @@ -289,10 +298,14 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>  
>  static umode_t nvmem_bin_attr_get_umode(struct nvmem_device *nvmem)
>  {
> -	struct nvmem_impl *impl = nvmem->impl;
> -
> +	struct nvmem_impl *impl;
>  	umode_t mode = 0400;
>  
> +	guard(srcu)(&nvmem->srcu);
> +	impl = rcu_dereference(nvmem->impl);
> +	if (!impl)
> +		return 0;
> +
>  	if (!nvmem->root_only)
>  		mode |= 0044;
>  
> @@ -333,7 +346,12 @@ static umode_t nvmem_attr_is_visible(struct kobject *kobj,
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct nvmem_device *nvmem = to_nvmem_device(dev);
> -	struct nvmem_impl *impl = nvmem->impl;
> +	struct nvmem_impl *impl;
> +
> +	guard(srcu)(&nvmem->srcu);
> +	impl = rcu_dereference(nvmem->impl);
> +	if (!impl)
> +		return 0;
>  
>  	/*
>  	 * If the device has no .reg_write operation, do not allow

These functions are only called during registration (and don't even
dereference the callback pointers) so adding locking here is a bit
misleading.

Perhaps you can just add another flag in a preparatory patch to stop
accessing the ops directly.

Note that read_only is already set if there is no write callback, so
you'd only need a flag for write_only.

> @@ -460,10 +478,9 @@ static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
>  	return 0;
>  }
>  
> -static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem,
> -			      const struct nvmem_config *config)
> +static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
>  {
> -	if (config->compat)
> +	if (nvmem->flags & FLAG_COMPAT)
>  		device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
>  }

The compat attribute should also be removed at deregistration so this
may not be needed.
 
> +static void nvmem_release(struct device *dev)
> +{
> +	struct nvmem_device *nvmem = to_nvmem_device(dev);
> +
> +	gpiod_put(nvmem->wp_gpio);

> +	nvmem_sysfs_remove_compat(nvmem);

This one should be moved to nvmem_unregister().

> +	nvmem_device_remove_all_cells(nvmem);
> +	ida_free(&nvmem_ida, nvmem->id);
> +	cleanup_srcu_struct(&nvmem->srcu);
> +	kfree(nvmem);
> +}

Johan

Re: [PATCH v2 6/7] nvmem: split struct nvmem_device into refcounted and provider-owned data

[Bartosz Golaszewski]

On Tue, Mar 17, 2026 at 11:44 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Feb 23, 2026 at 11:57:07AM +0100, Bartosz Golaszewski wrote:
> > Data owned by the nvmem provider must not be part of the
> > reference-counted struct nvmem_device.
>
> It can be as long as it's not referenced after deregistration so this
> claim is too strong.
>
> It can be handled the way you are doing it here, but you could also just
> use a boolean flag to indicate that no further callbacks should be made.
>
> > Ahead of protecting that data
> > with SRCU, move it into a separate structure the address of which is
> > stored in nvmem_device.
>
> > +/*
> > + * Holds data owned by the provider of the nvmem implementation. This goes
> > + * away immediately the moment nvmem_unregister() is called.
> > + */
> > +struct nvmem_impl {
> > +     nvmem_reg_read_t        reg_read;
> > +     nvmem_reg_write_t       reg_write;
> > +     void                    *priv;
>
> And priv is just a pointer that is never used by nvmem core directly.
>
> > +};
>
> How about just wrapping the callbacks and calling the struct nvmem_ops
> to make it more clear how this is used?
>
> That is, that nvmem core guarantees that there will be no further
> callbacks after deregistration.
>

Yeah, having an ops struct makes sense. Do you have any objections to
patches 1-5? Maybe Srini could pick them up for v7.1 because I'm not
sure when I'll be able to rework the last two.

Bartosz

Re: [PATCH v2 1/7] nvmem: remove unused field from struct nvmem_device

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:02AM +0100, Bartosz Golaszewski wrote:
> The node list_head in struct nvmem_device is unused. Drop it.

I'd expect a comment here saying something about why it's no longer
used (and since when).

Looking at git blame it seems this was mistakenly added by commit
ec9c08a1cb8d ("nvmem: Create a header for internal sharing") which was
just supposed to move the struct to a new header. 

Johan

Re: [PATCH v2 2/7] nvmem: return -EOPNOTSUPP to in-kernel users on missing callbacks

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:03AM +0100, Bartosz Golaszewski wrote:
> __nvmem_reg_read/write() currently return -EINVAL if the relevant
> callback is not present. User-space helpers again check the presence
> of the callbacks to see if they should return -EPERM.
> 
> Ahead of adding SRCU synchronization: change the error code returned to
> in-kernel users to -EOPNOTSUPP which is more indicative of the actual
> reason for the failure as well as allows us to translate it easily to
> -EPERM in sysfs callbacks without having to dereference the callback
> pointers. This will allow us to limit the number of SRCU critical
> sections in the follow-up commits.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/nvmem/core.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 311cb2e5a5c02d2c6979d7c9bbb7f94abdfbdad1..14f583e466c8d95690539bd886fd0c2fdd440998 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -55,10 +55,10 @@ static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>  static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
>  			    void *val, size_t bytes)
>  {
> -	if (nvmem->reg_read)
> -		return nvmem->reg_read(nvmem->priv, offset, val, bytes);
> +	if (!nvmem->reg_read)
> +		return -EOPNOTSUPP;
>  
> -	return -EINVAL;
> +	return nvmem->reg_read(nvmem->priv, offset, val, bytes);
>  }
>  
>  static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
> @@ -66,14 +66,14 @@ static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
>  {
>  	int ret;
>  
> -	if (nvmem->reg_write) {
> -		gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
> -		ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
> -		gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
> -		return ret;
> -	}
> +	if (!nvmem->reg_write)
> +		return -EOPNOTSUPP;
> +
> +	gpiod_set_value_cansleep(nvmem->wp_gpio, 0);
> +	ret = nvmem->reg_write(nvmem->priv, offset, val, bytes);
> +	gpiod_set_value_cansleep(nvmem->wp_gpio, 1);
>  
> -	return -EINVAL;
> +	return ret;
>  }

The above looks like good clean ups in their own right.

But shouldn't the check be moved to to nvmem_reg_read/write() to avoid
calling into nvmem_access_with_keepouts()?

>  static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
> @@ -231,13 +231,12 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
>  
>  	count = round_down(count, nvmem->word_size);
>  
> -	if (!nvmem->reg_read)
> -		return -EPERM;
> -
>  	rc = nvmem_reg_read(nvmem, pos, buf, count);
> -
> -	if (rc)
> +	if (rc) {
> +		if (rc == -EOPNOTSUPP)
> +			rc = -EPERM;
>  		return rc;
> +	}
>  
>  	return count;
>  }
> @@ -264,13 +263,15 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>  
>  	count = round_down(count, nvmem->word_size);
>  
> -	if (!nvmem->reg_write || nvmem->read_only)
> +	if (nvmem->read_only)
>  		return -EPERM;
>  
>  	rc = nvmem_reg_write(nvmem, pos, buf, count);
> -
> -	if (rc)
> +	if (rc) {
> +		if (rc == -EOPNOTSUPP)
> +			rc = -EPERM;
>  		return rc;
> +	}
>  
>  	return count;
>  }

And these attributes are not even visible when the device does not
support reading or writing so checking later is not an issue.

Did you make sure that there are no in-kernel callers that will get
confused by the -EINVAL => -EOPNOTSUPP change?

Johan

Re: [PATCH v2 3/7] nvmem: check the return value of gpiod_set_value_cansleep()

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:04AM +0100, Bartosz Golaszewski wrote:
> GPIO setters now return integer values and can indicate failures in
> lower abstraction layers. Check the return values of
> gpiod_set_value_cansleep() calls in nvmem core.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Reviewed-by: Johan Hovold <johan@kernel.org>

Re: [PATCH v2 4/7] nvmem: simplify locking with guard()

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:05AM +0100, Bartosz Golaszewski wrote:
> Use lock guards from cleanup.h to simplify locking. While at it: add the
> missing mutex.h include.
 
> @@ -1335,7 +1326,7 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
>  
>  	dev_id = dev_name(dev);
>  
> -	mutex_lock(&nvmem_lookup_mutex);
> +	guard(mutex)(&nvmem_mutex);
>  
>  	list_for_each_entry(lookup, &nvmem_lookup_list, node) {
>  		if ((strcmp(lookup->dev_id, dev_id) == 0) &&
> @@ -1343,11 +1334,9 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
>  			/* This is the right entry. */
>  			nvmem = __nvmem_device_get((void *)lookup->nvmem_name,
>  						   device_match_name);
> -			if (IS_ERR(nvmem)) {
> +			if (IS_ERR(nvmem))
>  				/* Provider may not be registered yet. */
> -				cell = ERR_CAST(nvmem);
> -				break;
> -			}
> +				return ERR_CAST(nvmem);

Please keep the brackets here for readability.

>  
>  			cell_entry = nvmem_find_cell_entry_by_name(nvmem,
>  								   lookup->cell_name);
> @@ -1363,7 +1352,6 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
>  		}
>  	}
>  
> -	mutex_unlock(&nvmem_lookup_mutex);
>  	return cell;
>  }
>  
> @@ -1379,14 +1367,14 @@ nvmem_find_cell_entry_by_node(struct nvmem_device *nvmem, struct device_node *np
>  {
>  	struct nvmem_cell_entry *iter, *cell = NULL;
>  
> -	mutex_lock(&nvmem_mutex);
> +	guard(mutex)(&nvmem_mutex);
> +
>  	list_for_each_entry(iter, &nvmem->cells, node) {
>  		if (np == iter->np) {
>  			cell = iter;
>  			break;

Shouldn't you return cell here now?

>  		}
>  	}
> -	mutex_unlock(&nvmem_mutex);
>  
>  	return cell;
>  }

And explicit NULL here (dropping the initialisation above)?

Johan

Re: [PATCH v2 5/7] nvmem: remove unneeded __nvmem_device_put()

[Johan Hovold]

On Mon, Feb 23, 2026 at 11:57:06AM +0100, Bartosz Golaszewski wrote:
> __nvmem_device_put() is wrapped by nvmem_device_put() but there's no
> extra functionality offered by the latter so just fold one into the
> other.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/nvmem/core.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 42827ba2146ce9ba07716ac5973327da6e9426da..30aee8089f1dc65b1fa07dac8fb35e1dbbcd0ce7 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1134,13 +1134,6 @@ static struct nvmem_device *__nvmem_device_get(void *data,
>  	return nvmem;
>  }
>  
> -static void __nvmem_device_put(struct nvmem_device *nvmem)
> -{
> -	put_device(&nvmem->dev);
> -	module_put(nvmem->owner);
> -	kref_put(&nvmem->refcnt, nvmem_device_release);
> -}

Not sure this is a good idea since __nvmem_device_put() here matches
__nvmem_device_get() just above (and we already have
nvmem_device_get()).

Keeping them together and as the inverse of each other should prevent
things from getting out of sync.

I'd just leave this as is for now.

Johan

Re: [PATCH v2 6/7] nvmem: split struct nvmem_device into refcounted and provider-owned data

[Johan Hovold]

On Tue, Mar 17, 2026 at 01:52:13PM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 17, 2026 at 11:44 AM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Feb 23, 2026 at 11:57:07AM +0100, Bartosz Golaszewski wrote:

> > > +/*
> > > + * Holds data owned by the provider of the nvmem implementation. This goes
> > > + * away immediately the moment nvmem_unregister() is called.
> > > + */
> > > +struct nvmem_impl {
> > > +     nvmem_reg_read_t        reg_read;
> > > +     nvmem_reg_write_t       reg_write;
> > > +     void                    *priv;
> >
> > And priv is just a pointer that is never used by nvmem core directly.
> >
> > > +};
> >
> > How about just wrapping the callbacks and calling the struct nvmem_ops
> > to make it more clear how this is used?
> >
> > That is, that nvmem core guarantees that there will be no further
> > callbacks after deregistration.
> 
> Yeah, having an ops struct makes sense. Do you have any objections to
> patches 1-5? Maybe Srini could pick them up for v7.1 because I'm not
> sure when I'll be able to rework the last two.

This one fell between the cracks, sorry. I've taken a closer look at 1-5
now as well.

Johan