[PATCH] nvmem: don't use devres in subsystem code

[Bartosz Golaszewski]

Using device-managed APIs at sub-system level can cause issues due to
lack of a strict API contract, especially when relying on managed
resources being freed on device release. Devres should only be used in
drivers, ideally only when it's bound to a driver.

In nvmem, the release of the memory allocated for sysfs attributes,
happens when the last reference to nvmem->dev is dropped, not in
nvmem_unregister(). This means that consumer holding references to nvmem
devices keep the memory around after sysfs files are gone which happens
in device_del().

The rest of nvmem/core.c manages resources explicitly, while using
devres in sysfs registration hides the details of what goes on and is
inconsistent.

Switch to non-managed interfaces exclusively in nvmem core.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
---
 drivers/nvmem/core.c      | 61 ++++++++++++++++++++++++++++++++++-------------
 drivers/nvmem/internals.h |  6 +++++
 2 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 7d4cce8e27424a032fff59a1189847adb1898168..e43adc3b9a2427918f7291bc146790b163a7aa5e 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -478,12 +478,7 @@ static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
 
 static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 {
-	struct attribute_group group = {
-		.name	= "cells",
-	};
 	struct nvmem_cell_entry *entry;
-	const struct bin_attribute **pattrs;
-	struct bin_attribute *attrs;
 	unsigned int ncells = 0, i = 0;
 	int ret;
 
@@ -494,44 +489,72 @@ static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
 
 	/* Allocate an array of attributes with a sentinel */
 	ncells = list_count_nodes(&nvmem->cells);
-	pattrs = devm_kcalloc(&nvmem->dev, ncells + 1,
-			      sizeof(*pattrs), GFP_KERNEL);
+
+	const struct bin_attribute **pattrs __free(kfree) = kzalloc_objs(*pattrs, ncells + 1);
 	if (!pattrs)
 		return -ENOMEM;
 
-	attrs = devm_kcalloc(&nvmem->dev, ncells, sizeof(struct bin_attribute), GFP_KERNEL);
+	struct bin_attribute *attrs __free(kfree) = kzalloc_objs(*attrs, ncells);
 	if (!attrs)
 		return -ENOMEM;
 
 	/* Initialize each attribute to take the name and size of the cell */
 	list_for_each_entry(entry, &nvmem->cells, node) {
 		sysfs_bin_attr_init(&attrs[i]);
-		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
-						    "%s@%x,%x", entry->name,
-						    entry->offset,
-						    entry->bit_offset);
+		attrs[i].attr.name = kasprintf(GFP_KERNEL, "%s@%x,%x", entry->name,
+					       entry->offset, entry->bit_offset);
+		if (!attrs[i].attr.name) {
+			list_for_each_entry_continue_reverse(entry, &nvmem->cells, node)
+				kfree(attrs[--i].attr.name);
+			return -ENOMEM;
+		}
+
 		attrs[i].attr.mode = 0444 & nvmem_bin_attr_get_umode(nvmem);
 		attrs[i].size = entry->bytes;
 		attrs[i].read = &nvmem_cell_attr_read;
 		attrs[i].private = entry;
-		if (!attrs[i].attr.name)
-			return -ENOMEM;
 
 		pattrs[i] = &attrs[i];
 		i++;
 	}
 
-	group.bin_attrs = pattrs;
+	memset(&nvmem->sysfs_group, 0, sizeof(nvmem->sysfs_group));
+	nvmem->sysfs_group.name = "cells";
+	nvmem->sysfs_group.bin_attrs = pattrs;
 
-	ret = device_add_group(&nvmem->dev, &group);
-	if (ret)
+	ret = device_add_group(&nvmem->dev, &nvmem->sysfs_group);
+	if (ret) {
+		i = 0;
+		list_for_each_entry(entry, &nvmem->cells, node)
+			kfree(attrs[i++].attr.name);
 		return ret;
+	}
 
 	nvmem->sysfs_cells_populated = true;
+	nvmem->sysfs_attrs = no_free_ptr(attrs);
+	nvmem->sysfs_pattrs = no_free_ptr(pattrs);
 
 	return ret;
 }
 
+static void nvmem_sysfs_remove_cells(struct nvmem_device *nvmem)
+{
+	int i;
+
+	if (!nvmem->sysfs_cells_populated)
+		return;
+
+	device_remove_group(&nvmem->dev, &nvmem->sysfs_group);
+
+	for (i = 0; nvmem->sysfs_pattrs[i]; i++)
+		kfree(nvmem->sysfs_pattrs[i]->attr.name);
+
+	kfree(nvmem->sysfs_attrs);
+	kfree(nvmem->sysfs_pattrs);
+
+	nvmem->sysfs_cells_populated = false;
+}
+
 #else /* CONFIG_NVMEM_SYSFS */
 
 static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
@@ -542,6 +565,9 @@ static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
 static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem)
 {
 }
+static void nvmem_sysfs_remove_cells(struct nvmem_device *nvmem)
+{
+}
 
 #endif /* CONFIG_NVMEM_SYSFS */
 
@@ -1085,6 +1111,7 @@ void nvmem_unregister(struct nvmem_device *nvmem)
 	synchronize_srcu(&nvmem->srcu);
 
 	nvmem_sysfs_remove_compat(nvmem);
+	nvmem_sysfs_remove_cells(nvmem);
 	nvmem_destroy_layout(nvmem);
 	kfree(ops);
 
diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
index bc7a99f5aefbffced60d275bec888ac867fd9032..b461ec6a0e86db2241a27d4789196bf01532efb0 100644
--- a/drivers/nvmem/internals.h
+++ b/drivers/nvmem/internals.h
@@ -7,6 +7,7 @@
 #include <linux/nvmem-consumer.h>
 #include <linux/nvmem-provider.h>
 #include <linux/srcu.h>
+#include <linux/sysfs.h>
 
 /* Hold pointers to callbacks owned by the nvmem provider module. */
 struct nvmem_operations {
@@ -37,7 +38,12 @@ struct nvmem_device {
 	struct nvmem_layout	*layout;
 	struct nvmem_operations __rcu *ops;
 	void *priv;
+#if IS_ENABLED(CONFIG_NVMEM_SYSFS)
 	bool			sysfs_cells_populated;
+	struct attribute_group	sysfs_group;
+	struct bin_attribute	*sysfs_attrs;
+	const struct bin_attribute **sysfs_pattrs;
+#endif /* CONFIG_NVMEM_SYSFS */
 };
 
 int nvmem_add_cells_from_dt(struct nvmem_device *nvmem, struct device_node *np);

---
base-commit: bcda2aa8c41de315a5cd5d672210541b8aaaad31
change-id: 20261021-nvmem-no-devm-in-core-b171da3d753c

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>