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

[PATCH 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>
---
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      | 270 +++++++++++++++++++++++++---------------------
 drivers/nvmem/internals.h |  18 +++-
 2 files changed, 163 insertions(+), 125 deletions(-)
---
base-commit: 79455889ce4668b4239b3ffe97a86b24e99f2d7e
change-id: 20260114-nvmem-unbind-673b52fc84a0

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

[PATCH 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 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 387c88c55259541446901f0e539bbb0dd8c4c3de..c4a5f8ef65164ef6994343e6d26d3d302c9b00c3 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 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 c4a5f8ef65164ef6994343e6d26d3d302c9b00c3..454e0645d1545602cd193f66b54bd5c5811ca5ed 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 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 454e0645d1545602cd193f66b54bd5c5811ca5ed..f045c53863aa7d55d51e44712c01eba97be2ac66 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;
 }
@@ -1124,11 +1115,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);
 
@@ -1338,7 +1329,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) &&
@@ -1346,11 +1337,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);
@@ -1366,7 +1355,6 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
 		}
 	}
 
-	mutex_unlock(&nvmem_lookup_mutex);
 	return cell;
 }
 
@@ -1382,14 +1370,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;
 }
@@ -2126,10 +2114,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);
 
@@ -2144,10 +2132,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 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 f045c53863aa7d55d51e44712c01eba97be2ac66..e3e15fa462567872718b9262a039202c6961deea 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1137,13 +1137,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
@@ -1256,7 +1249,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);
 
@@ -1344,12 +1339,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;
 		}
@@ -1459,14 +1454,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);
@@ -1476,7 +1471,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);
 	}
 
@@ -1591,7 +1586,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 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 e3e15fa462567872718b9262a039202c6961deea..f6892af8ace52d033846f4052722289c2aa826df 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);
 }
 
@@ -901,6 +908,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)
@@ -913,8 +921,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);
 	}
@@ -940,6 +955,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;
@@ -947,10 +967,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)
@@ -976,7 +993,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 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      | 115 +++++++++++++++++++++++++++-------------------
 drivers/nvmem/internals.h |   5 +-
 2 files changed, 72 insertions(+), 48 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index f6892af8ace52d033846f4052722289c2aa826df..1acc65026eb5094658e5523104c649a280bfc471 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
@@ -537,24 +555,6 @@ 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 +573,23 @@ 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);
+
+	nvmem_device_remove_all_cells(nvmem);
+	ida_free(&nvmem_ida, nvmem->id);
+	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)
@@ -951,7 +968,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;
 
@@ -959,7 +975,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;
@@ -1064,32 +1085,28 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 }
 EXPORT_SYMBOL_GPL(nvmem_register);
 
-static void nvmem_device_release(struct kref *kref)
+/**
+ * nvmem_unregister() - Unregister previously registered nvmem device
+ *
+ * @nvmem: Pointer to previously registered nvmem device.
+ */
+void nvmem_unregister(struct nvmem_device *nvmem)
 {
-	struct nvmem_device *nvmem;
-
-	nvmem = container_of(kref, struct nvmem_device, refcnt);
+	struct nvmem_impl *impl;
 
 	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);
+	impl = rcu_replace_pointer(nvmem->impl, NULL, true);
+	synchronize_srcu(&nvmem->srcu);
+
 	nvmem_destroy_layout(nvmem);
+	kfree(impl);
+	gpiod_put(nvmem->wp_gpio);
 	device_unregister(&nvmem->dev);
 }
-
-/**
- * nvmem_unregister() - Unregister previously registered nvmem device
- *
- * @nvmem: Pointer to previously registered nvmem device.
- */
-void nvmem_unregister(struct nvmem_device *nvmem)
-{
-	if (nvmem)
-		kref_put(&nvmem->refcnt, nvmem_device_release);
-}
 EXPORT_SYMBOL_GPL(nvmem_unregister);
 
 static void devm_nvmem_unregister(void *nvmem)
@@ -1149,8 +1166,6 @@ static struct nvmem_device *__nvmem_device_get(void *data,
 		return ERR_PTR(-EINVAL);
 	}
 
-	kref_get(&nvmem->refcnt);
-
 	return nvmem;
 }
 
@@ -1266,9 +1281,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);
 
@@ -1655,6 +1669,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 7/7] nvmem: synchronize nvmem device unregistering with SRCU

[Bartosz Golaszewski]

On Fri, Jan 16, 2026 at 12:01 PM Bartosz Golaszewski
<bartosz.golaszewski@oss.qualcomm.com> wrote:
>
> +static void nvmem_release(struct device *dev)
> +{
> +       struct nvmem_device *nvmem = to_nvmem_device(dev);
> +
> +       nvmem_device_remove_all_cells(nvmem);
> +       ida_free(&nvmem_ida, nvmem->id);

Gah, this is missing cleanup_srcu_struct() here. But I'll only send a
v2 if Srini agrees this should go forward and it's targeting v7.1
anyway.

Bartosz

> +       kfree(nvmem);
> +}
> +

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

[Johan Hovold]

On Fri, Jan 16, 2026 at 12:01:07PM +0100, Bartosz Golaszewski wrote:
> 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.

Well, don't do that then. Only root can unbind drivers, and we have
things like module references to prevent drivers from being unloaded
while in use.

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

Hotplugging may be a real issue, though. But this can solved at the user
interface level. Did you explore that?

For reference, this is related to the i2c discussion here:

	https://lore.kernel.org/lkml/aW4OWnyYp6Vas53L@hovoldconsulting.com/

Johan

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

[Bartosz Golaszewski]

On Mon, Jan 19, 2026 at 12:22 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Jan 16, 2026 at 12:01:07PM +0100, Bartosz Golaszewski wrote:
> > 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.
>
> Well, don't do that then. Only root can unbind drivers, and we have
> things like module references to prevent drivers from being unloaded
> while in use.
>

That's a very weird argument. It roughly translates to: "There's an
issue that can crash the system but only root can trigger it so let's
never fix it". If that's how we work then why don't we allow root to
kill init with a signal?

Is this a corner-case? Sure. Should we not address corner cases? Then
why do we fix error paths we never hit or that result in an unusable
system anyway?

Also: sysfs attributes don't take a module reference. Unloading an
nvmem module during a long read from user-space is equivalent to a USB
unplug.

> > 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.
>
> Hotplugging may be a real issue, though. But this can solved at the user
> interface level. Did you explore that?
>

Did I explore what exactly? If you're saying this *can* be solved,
then please also say how. Nvmem has no idea what abstraction layer it
sits above. Nvmem core doesn't even know what abstraction layer the
nvmem providers use.

> For reference, this is related to the i2c discussion here:
>
>         https://lore.kernel.org/lkml/aW4OWnyYp6Vas53L@hovoldconsulting.com/
>

Your argument against the i2c changes is that they affect lots of
drivers and decrease readability. I can see it as a point worth
considering. In the end it's up to Wolfram to decide and I think he
was pretty clear he wants it to proceed.

This series addresses the unbinding issue entirely within nvmem core,
no driver changes are required. It uses SRCU which is very fast in
read sections. What exactly is your argument against it? What is the
reason for your comment? Unless you deal with nvmem internals, you
never have to concern yourself with it.

Bartosz

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

[Johan Hovold]

On Mon, Jan 19, 2026 at 01:43:01PM +0100, Bartosz Golaszewski wrote:
> On Mon, Jan 19, 2026 at 12:22 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Fri, Jan 16, 2026 at 12:01:07PM +0100, Bartosz Golaszewski wrote:
> > > 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.
> >
> > Well, don't do that then. Only root can unbind drivers, and we have
> > things like module references to prevent drivers from being unloaded
> > while in use.
> 
> That's a very weird argument. It roughly translates to: "There's an
> issue that can crash the system but only root can trigger it so let's
> never fix it". If that's how we work then why don't we allow root to
> kill init with a signal?

It's not weird at all. The cost associated with preventing *root* from
using a foot gun may simply be too high for it to be worth it. For a
regular user, the equation is different.

> Is this a corner-case? Sure. Should we not address corner cases? Then
> why do we fix error paths we never hit or that result in an unusable
> system anyway?

That's just a false equivalence.

> Also: sysfs attributes don't take a module reference. Unloading an
> nvmem module during a long read from user-space is equivalent to a USB
> unplug.

Sysfs will drain any ongoing operations before deregistering, so not
sure what you're referring to here.

> > > 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.
> >
> > Hotplugging may be a real issue, though. But this can solved at the user
> > interface level. Did you explore that?
> >
> 
> Did I explore what exactly? If you're saying this *can* be solved,
> then please also say how. Nvmem has no idea what abstraction layer it
> sits above. Nvmem core doesn't even know what abstraction layer the
> nvmem providers use.

The userspace interface for nvmem access. If the only interface is the
sysfs one then there should be nothing to worry about there.

> > For reference, this is related to the i2c discussion here:
> >
> >         https://lore.kernel.org/lkml/aW4OWnyYp6Vas53L@hovoldconsulting.com/
> >
> 
> Your argument against the i2c changes is that they affect lots of
> drivers and decrease readability. I can see it as a point worth
> considering. In the end it's up to Wolfram to decide and I think he
> was pretty clear he wants it to proceed.
> 
> This series addresses the unbinding issue entirely within nvmem core,
> no driver changes are required. It uses SRCU which is very fast in
> read sections. What exactly is your argument against it? What is the
> reason for your comment? Unless you deal with nvmem internals, you
> never have to concern yourself with it.

My concern is that you're stuck on the idea of wrapping every access in
the kernel with unnecessary constructs because you seem to think driver
unbinding is something we need to worry about it. It's not. At least
not at any cost (readability, maintainability, cognitive load, churn,
performance, ...).

Johan

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

[Bartosz Golaszewski]

On Tue, Jan 20, 2026 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:
>

[snip to avoid more bickering]

>
> > Also: sysfs attributes don't take a module reference. Unloading an
> > nvmem module during a long read from user-space is equivalent to a USB
> > unplug.
>
> Sysfs will drain any ongoing operations before deregistering, so not
> sure what you're referring to here.
>

True, sysfs will wait for the current operation to complete when
removing attributes but currently you can still very easily trigger a
use-after-free splat from user-space with nvmem the way I described
due to ordering of the teardown. But fair enough - this is an nvmem
problem, not sysfs.

> > > For reference, this is related to the i2c discussion here:
> > >
> > >         https://lore.kernel.org/lkml/aW4OWnyYp6Vas53L@hovoldconsulting.com/
> > >
> >
> > Your argument against the i2c changes is that they affect lots of
> > drivers and decrease readability. I can see it as a point worth
> > considering. In the end it's up to Wolfram to decide and I think he
> > was pretty clear he wants it to proceed.
> >
> > This series addresses the unbinding issue entirely within nvmem core,
> > no driver changes are required. It uses SRCU which is very fast in
> > read sections. What exactly is your argument against it? What is the
> > reason for your comment? Unless you deal with nvmem internals, you
> > never have to concern yourself with it.
>
> My concern is that you're stuck on the idea of wrapping every access in

"Every access" is an exaggeration.

> the kernel with unnecessary constructs because you seem to think driver
> unbinding is something we need to worry about it. It's not. At least
> not at any cost (readability, maintainability, cognitive load, churn,
> performance, ...).
>

That is simply incorrect. I didn't just suddenly make this up. I
encountered this issue - and subsequently started working on it - in
my work with a client's device based on CP2112 I2C/GPIO USB expander
where unplugging it would either crash the system in GPIO unbind path
or freeze the kernel thread forever waiting for a completion in I2C
unbind path. That's why - contrary to what you're claiming here - we
(the kernel community) *do* care about driver unbinding. I truly can't
comprehend why kernel just easily crashing on what is pretty normal
operation does not seem wrong to you.

Ever since Laurent's first talk about object life-time issues (was
that 2021?), I've discussed this with many people - including key
kernel maintainers like Greg KH. People disagree on ways of fixing
this family of problems (it's not a single bug) but you are quite
literally the only person who says that we should do nothing.

Meanwhile, unbinding may not be a problem until it is. I've had Brian
Masney from Red Hat approach me during LPC in Tokyo because he needs
to be able to unbind a clock driver at runtime. At another conference,
Bootlin had a whole BoF/miniconf about dynamically instantiated and
removed platform devices (this was for cape-like extensions but
unbinding was one of the issues). If you're claiming it's unimportant,
then you're simply wrong.

Feel free to review the patches or point out anything incorrect in the
description of the problem, but please stop commenting under every
related series I post (seriously, did you filter lore by my name to
find this one?), saying "we don't want to do this". The consensus is
that this can and should be fixed. You yourself posted a link to
Wolfram's email saying that much about I2C.

I'll wait for Srini to decide whether he wants this or not.

Thanks,
Bartosz

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

[Johan Hovold]

On Tue, Jan 20, 2026 at 08:57:41PM +0100, Bartosz Golaszewski wrote:
> On Tue, Jan 20, 2026 at 11:18 AM Johan Hovold <johan@kernel.org> wrote:

> > > > For reference, this is related to the i2c discussion here:
> > > >
> > > >         https://lore.kernel.org/lkml/aW4OWnyYp6Vas53L@hovoldconsulting.com/
> > > >
> > >
> > > Your argument against the i2c changes is that they affect lots of
> > > drivers and decrease readability. I can see it as a point worth
> > > considering. In the end it's up to Wolfram to decide and I think he
> > > was pretty clear he wants it to proceed.
> > >
> > > This series addresses the unbinding issue entirely within nvmem core,
> > > no driver changes are required. It uses SRCU which is very fast in
> > > read sections. What exactly is your argument against it? What is the
> > > reason for your comment? Unless you deal with nvmem internals, you
> > > never have to concern yourself with it.
> >
> > My concern is that you're stuck on the idea of wrapping every access in
> 
> "Every access" is an exaggeration.
> 
> > the kernel with unnecessary constructs because you seem to think driver
> > unbinding is something we need to worry about it. It's not. At least
> > not at any cost (readability, maintainability, cognitive load, churn,
> > performance, ...).
> >
> 
> That is simply incorrect. I didn't just suddenly make this up. I
> encountered this issue - and subsequently started working on it - in
> my work with a client's device based on CP2112 I2C/GPIO USB expander
> where unplugging it would either crash the system in GPIO unbind path
> or freeze the kernel thread forever waiting for a completion in I2C
> unbind path. That's why - contrary to what you're claiming here - we
> (the kernel community) *do* care about driver unbinding. I truly can't
> comprehend why kernel just easily crashing on what is pretty normal
> operation does not seem wrong to you.

Again, driver unbinding (module unloading) is a development tool, if
making it 100% fool proof would require making drivers unmaintainable
while adding enough runtime overhead it *may* simply not be worth it.

And there are other ways to address this without any such costs since in
99.9999% of cases where unbinding makes no sense at all. If you don't
trust yourself from running around unbinding random drivers, you can
even disable unloading completely. And fw_devlink unbinds consumers
before providers. Etc, etc.

Hot-plug is a different issue, and not something new that you've just
discovered. USB serial deals with this since forever and during Project
Ara we quickly learned that most subsystems do not support hot plugging
(for obvious reasons).

But the most relevant issue here is the user-space interface since a
user can keep a file descriptor open indefinitely. And that can
potentially be solved in a generic fashion without messing up every
driver for something that is not a problem in practice.
 
> Ever since Laurent's first talk about object life-time issues (was
> that 2021?), I've discussed this with many people - including key
> kernel maintainers like Greg KH. People disagree on ways of fixing
> this family of problems (it's not a single bug) but you are quite
> literally the only person who says that we should do nothing.

Re-read what I just wrote above.

> Meanwhile, unbinding may not be a problem until it is. I've had Brian
> Masney from Red Hat approach me during LPC in Tokyo because he needs
> to be able to unbind a clock driver at runtime. At another conference,
> Bootlin had a whole BoF/miniconf about dynamically instantiated and
> removed platform devices (this was for cape-like extensions but
> unbinding was one of the issues). If you're claiming it's unimportant,
> then you're simply wrong.

That's about tearing down a subtree, where children would be removed
before parents. If we have random links to hotpluggable devices then
that would be an issue, but so far that looks like yet another contrived
example.

> Feel free to review the patches or point out anything incorrect in the
> description of the problem, but please stop commenting under every
> related series I post (seriously, did you filter lore by my name to
> find this one?), saying "we don't want to do this". The consensus is
> that this can and should be fixed. You yourself posted a link to
> Wolfram's email saying that much about I2C.

Don't flatter yourself.

Johan