[PATCH 0/7] Miscellaneous devres improvements

[PATCH 0/7] Miscellaneous devres improvements

[Danilo Krummrich]

This patch series provides some miscellaneous devres improvements.

The most notable change is a more clear separation between the base struct
devres_node and its "subclasses" (such as struct devres and struct devres_group)
as well as the mixup between struct devres and struct devres_group.

This serves three purposes.

  (1) Better maintainability due to clear type separation.

  (2) No waste of memory due to unnecessary minimal alignment for devres
      actions, by introducing its own struct devres_action "subclass".

  (3) Bootstrapping for Rust devres code to embed a struct devres_node itself
      to avoid both the unnecessary minimum alignment of struct devres and the
      indirection through devres action avoiding unnecessary additional memory
      allocations. [1]

However, (3) is just for completeness, the series also stands on its own.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?h=devres/devres_node&id=6da8ca499fa65992d71c5cb7a51df7260666e695

Danilo Krummrich (7):
  devres: fix missing node debug info in devm_krealloc()
  devres: add devres_node_add()
  devres: add devres_node_init()
  devres: don't require ARCH_DMA_MINALIGN for devres actions
  devres: add free_node callback to struct devres_node
  devres: use guard(spinlock_irqsave) where applicable
  devres: remove unnecessary unlocks in devres_release_group()

 drivers/base/devres.c | 248 ++++++++++++++++++++++++++++--------------
 1 file changed, 165 insertions(+), 83 deletions(-)


base-commit: 289b14592cefe95f7d0ef334873c12b96ce3824f
-- 
2.52.0

[PATCH 1/7] devres: fix missing node debug info in devm_krealloc()

[Danilo Krummrich]

Fix missing call to set_node_dbginfo() for new devres nodes created by
devm_krealloc().

Fixes: f82485722e5d ("devres: provide devm_krealloc()")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index f54db6d138ab..1a972f47916e 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -940,6 +940,8 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
 	if (!new_dr)
 		return NULL;
 
+	set_node_dbginfo(&new_dr->node, "devm_krealloc_release", new_size);
+
 	/*
 	 * The spinlock protects the linked list against concurrent
 	 * modifications but not the resource itself.
-- 
2.52.0

[PATCH 2/7] devres: add devres_node_add()

[Danilo Krummrich]

Both devres_add() and devres_open_group() acquire the devres_lock and
call add_dr(). Add a helper, devres_node_add(), for this pattern.

Use guard(spinlock_irqsave) to avoid the explicit unlock call and local
flag variables.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 1a972f47916e..0061c6564469 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -231,6 +231,13 @@ void devres_free(void *res)
 }
 EXPORT_SYMBOL_GPL(devres_free);
 
+static void devres_node_add(struct device *dev, struct devres_node *node)
+{
+	guard(spinlock_irqsave)(&dev->devres_lock);
+
+	add_dr(dev, node);
+}
+
 /**
  * devres_add - Register device resource
  * @dev: Device to add resource to
@@ -243,11 +250,8 @@ EXPORT_SYMBOL_GPL(devres_free);
 void devres_add(struct device *dev, void *res)
 {
 	struct devres *dr = container_of(res, struct devres, data);
-	unsigned long flags;
 
-	spin_lock_irqsave(&dev->devres_lock, flags);
-	add_dr(dev, &dr->node);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
+	devres_node_add(dev, &dr->node);
 }
 EXPORT_SYMBOL_GPL(devres_add);
 
@@ -552,7 +556,6 @@ int devres_release_all(struct device *dev)
 void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
 {
 	struct devres_group *grp;
-	unsigned long flags;
 
 	grp = kmalloc(sizeof(*grp), gfp);
 	if (unlikely(!grp))
@@ -569,9 +572,7 @@ void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
 		grp->id = id;
 	grp->color = 0;
 
-	spin_lock_irqsave(&dev->devres_lock, flags);
-	add_dr(dev, &grp->node[0]);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
+	devres_node_add(dev, &grp->node[0]);
 	return grp->id;
 }
 EXPORT_SYMBOL_GPL(devres_open_group);
-- 
2.52.0

[PATCH 3/7] devres: add devres_node_init()

[Danilo Krummrich]

Both alloc_dr() and devres_open_group() initialize devres_node.entry and
set devres_node.release. Add a helper, devres_node_init(), for this
pattern.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 0061c6564469..7cc46aeae792 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -42,6 +42,12 @@ struct devres_group {
 	/* -- 8 pointers */
 };
 
+static void devres_node_init(struct devres_node *node, dr_release_t release)
+{
+	INIT_LIST_HEAD(&node->entry);
+	node->release = release;
+}
+
 static void set_node_dbginfo(struct devres_node *node, const char *name,
 			     size_t size)
 {
@@ -124,8 +130,7 @@ static __always_inline struct devres *alloc_dr(dr_release_t release,
 	if (!(gfp & __GFP_ZERO))
 		memset(dr, 0, offsetof(struct devres, data));
 
-	INIT_LIST_HEAD(&dr->node.entry);
-	dr->node.release = release;
+	devres_node_init(&dr->node, release);
 	return dr;
 }
 
@@ -561,10 +566,8 @@ void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
 	if (unlikely(!grp))
 		return NULL;
 
-	grp->node[0].release = &group_open_release;
-	grp->node[1].release = &group_close_release;
-	INIT_LIST_HEAD(&grp->node[0].entry);
-	INIT_LIST_HEAD(&grp->node[1].entry);
+	devres_node_init(&grp->node[0], &group_open_release);
+	devres_node_init(&grp->node[1], &group_close_release);
 	set_node_dbginfo(&grp->node[0], "grp<", 0);
 	set_node_dbginfo(&grp->node[1], "grp>", 0);
 	grp->id = grp;
-- 
2.52.0

[PATCH 4/7] devres: don't require ARCH_DMA_MINALIGN for devres actions

[Danilo Krummrich]

Currently, devres actions are allocated with devres_alloc(), which
allocates a struct devres with a flexible array member for the actual
data of the resource. The flexible array member is aligned to
ARCH_DMA_MINALIGN, which is wasteful for devres actions that only need
to store a struct action_devres.

Introduce struct devres_action to handle devres actions separately from
struct devres, analogous to what we do for struct devres_group.

Speaking of which, without this patch struct devres_group is treated as
struct devres in release_nodes(). While this is not an actual bug, as
release callbacks for devres nodes in struct devres_group are empty
functions anyways, it is a bit messy and can be confusing.

(Note that besides devres actions, the Rust devres code will also make
use of this. The Rust compiler can figure out the correct alignment of T
in Devres<T> itself, i.e. no need to force a minimum alignment.)

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 147 +++++++++++++++++++++++++++++-------------
 1 file changed, 102 insertions(+), 45 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 7cc46aeae792..df015ef155e0 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -16,15 +16,19 @@
 #include "base.h"
 #include "trace.h"
 
+struct devres_node;
+typedef void (*dr_node_release_t)(struct device *dev, struct devres_node *node);
+
 struct devres_node {
 	struct list_head		entry;
-	dr_release_t			release;
+	dr_node_release_t		release;
 	const char			*name;
 	size_t				size;
 };
 
 struct devres {
 	struct devres_node		node;
+	dr_release_t			release;
 	/*
 	 * Some archs want to perform DMA into kmalloc caches
 	 * and need a guaranteed alignment larger than
@@ -42,7 +46,7 @@ struct devres_group {
 	/* -- 8 pointers */
 };
 
-static void devres_node_init(struct devres_node *node, dr_release_t release)
+static void devres_node_init(struct devres_node *node, dr_node_release_t release)
 {
 	INIT_LIST_HEAD(&node->entry);
 	node->release = release;
@@ -81,12 +85,12 @@ static void devres_log(struct device *dev, struct devres_node *node,
  * Release functions for devres group.  These callbacks are used only
  * for identification.
  */
-static void group_open_release(struct device *dev, void *res)
+static void group_open_release(struct device *dev, struct devres_node *node)
 {
 	/* noop */
 }
 
-static void group_close_release(struct device *dev, void *res)
+static void group_close_release(struct device *dev, struct devres_node *node)
 {
 	/* noop */
 }
@@ -113,6 +117,13 @@ static bool check_dr_size(size_t size, size_t *tot_size)
 	return true;
 }
 
+static void dr_node_release(struct device *dev, struct devres_node *node)
+{
+	struct devres *dr = container_of(node, struct devres, node);
+
+	dr->release(dev, dr->data);
+}
+
 static __always_inline struct devres *alloc_dr(dr_release_t release,
 					       size_t size, gfp_t gfp, int nid)
 {
@@ -130,7 +141,8 @@ static __always_inline struct devres *alloc_dr(dr_release_t release,
 	if (!(gfp & __GFP_ZERO))
 		memset(dr, 0, offsetof(struct devres, data));
 
-	devres_node_init(&dr->node, release);
+	devres_node_init(&dr->node, dr_node_release);
+	dr->release = release;
 	return dr;
 }
 
@@ -209,7 +221,7 @@ void devres_for_each_res(struct device *dev, dr_release_t release,
 			&dev->devres_head, entry) {
 		struct devres *dr = container_of(node, struct devres, node);
 
-		if (node->release != release)
+		if (dr->release != release)
 			continue;
 		if (match && !match(dev, dr->data, match_data))
 			continue;
@@ -268,7 +280,7 @@ static struct devres *find_dr(struct device *dev, dr_release_t release,
 	list_for_each_entry_reverse(node, &dev->devres_head, entry) {
 		struct devres *dr = container_of(node, struct devres, node);
 
-		if (node->release != release)
+		if (dr->release != release)
 			continue;
 		if (match && !match(dev, dr->data, match_data))
 			continue;
@@ -330,7 +342,7 @@ void *devres_get(struct device *dev, void *new_res,
 	unsigned long flags;
 
 	spin_lock_irqsave(&dev->devres_lock, flags);
-	dr = find_dr(dev, new_dr->node.release, match, match_data);
+	dr = find_dr(dev, new_dr->release, match, match_data);
 	if (!dr) {
 		add_dr(dev, &new_dr->node);
 		dr = new_dr;
@@ -504,15 +516,15 @@ static int remove_nodes(struct device *dev,
 
 static void release_nodes(struct device *dev, struct list_head *todo)
 {
-	struct devres *dr, *tmp;
+	struct devres_node *node, *tmp;
 
-	/* Release.  Note that both devres and devres_group are
-	 * handled as devres in the following loop.  This is safe.
+	/* Release.  Note that devres, devres_action and devres_group are
+	 * handled as devres_node in the following loop.  This is safe.
 	 */
-	list_for_each_entry_safe_reverse(dr, tmp, todo, node.entry) {
-		devres_log(dev, &dr->node, "REL");
-		dr->node.release(dev, dr->data);
-		kfree(dr);
+	list_for_each_entry_safe_reverse(node, tmp, todo, entry) {
+		devres_log(dev, node, "REL");
+		node->release(dev, node);
+		kfree(node);
 	}
 }
 
@@ -720,20 +732,22 @@ struct action_devres {
 	void (*action)(void *);
 };
 
-static int devm_action_match(struct device *dev, void *res, void *p)
-{
-	struct action_devres *devres = res;
-	struct action_devres *target = p;
+struct devres_action {
+	struct devres_node node;
+	struct action_devres action;
+};
 
-	return devres->action == target->action &&
-	       devres->data == target->data;
+static int devm_action_match(struct devres_action *devres, struct action_devres *target)
+{
+	return devres->action.action == target->action &&
+	       devres->action.data == target->data;
 }
 
-static void devm_action_release(struct device *dev, void *res)
+static void devm_action_release(struct device *dev, struct devres_node *node)
 {
-	struct action_devres *devres = res;
+	struct devres_action *devres = container_of(node, struct devres_action, node);
 
-	devres->action(devres->data);
+	devres->action.action(devres->action.data);
 }
 
 /**
@@ -748,32 +762,71 @@ static void devm_action_release(struct device *dev, void *res)
  */
 int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name)
 {
-	struct action_devres *devres;
+	struct devres_action *devres;
 
-	devres = __devres_alloc_node(devm_action_release, sizeof(struct action_devres),
-				     GFP_KERNEL, NUMA_NO_NODE, name);
+	devres = kzalloc(sizeof(*devres), GFP_KERNEL);
 	if (!devres)
 		return -ENOMEM;
 
-	devres->data = data;
-	devres->action = action;
+	devres_node_init(&devres->node, devm_action_release);
+	set_node_dbginfo(&devres->node, name, sizeof(*devres));
 
-	devres_add(dev, devres);
+	devres->action.data = data;
+	devres->action.action = action;
+
+	devres_node_add(dev, &devres->node);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__devm_add_action);
 
-bool devm_is_action_added(struct device *dev, void (*action)(void *), void *data)
+static struct devres_action *devres_action_find(struct device *dev,
+						void (*action)(void *),
+						void *data)
 {
-	struct action_devres devres = {
+	struct devres_node *node;
+	struct action_devres target = {
 		.data = data,
 		.action = action,
 	};
 
-	return devres_find(dev, devm_action_release, devm_action_match, &devres);
+	list_for_each_entry_reverse(node, &dev->devres_head, entry) {
+		struct devres_action *dr = container_of(node, struct devres_action, node);
+
+		if (node->release != devm_action_release)
+			continue;
+		if (devm_action_match(dr, &target))
+			return dr;
+	}
+
+	return NULL;
+}
+
+bool devm_is_action_added(struct device *dev, void (*action)(void *), void *data)
+{
+	guard(spinlock_irqsave)(&dev->devres_lock);
+
+	return !!devres_action_find(dev, action, data);
 }
 EXPORT_SYMBOL_GPL(devm_is_action_added);
 
+static struct devres_action *remove_action(struct device *dev,
+					   void (*action)(void *),
+					   void *data)
+{
+	struct devres_action *dr;
+
+	guard(spinlock_irqsave)(&dev->devres_lock);
+
+	dr = devres_action_find(dev, action, data);
+	if (!dr)
+		return ERR_PTR(-ENOENT);
+
+	list_del_init(&dr->node.entry);
+	devres_log(dev, &dr->node, "REM");
+
+	return dr;
+}
+
 /**
  * devm_remove_action_nowarn() - removes previously added custom action
  * @dev: Device that owns the action
@@ -798,13 +851,15 @@ int devm_remove_action_nowarn(struct device *dev,
 			      void (*action)(void *),
 			      void *data)
 {
-	struct action_devres devres = {
-		.data = data,
-		.action = action,
-	};
+	struct devres_action *dr;
+
+	dr = remove_action(dev, action, data);
+	if (IS_ERR(dr))
+		return PTR_ERR(dr);
 
-	return devres_destroy(dev, devm_action_release, devm_action_match,
-			      &devres);
+	kfree(dr);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
 
@@ -820,14 +875,16 @@ EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
  */
 void devm_release_action(struct device *dev, void (*action)(void *), void *data)
 {
-	struct action_devres devres = {
-		.data = data,
-		.action = action,
-	};
 
-	WARN_ON(devres_release(dev, devm_action_release, devm_action_match,
-			       &devres));
+	struct devres_action *dr;
+
+	dr = remove_action(dev, action, data);
+	if (WARN_ON(IS_ERR(dr)))
+		return;
+
+	dr->action.action(dr->action.data);
 
+	kfree(dr);
 }
 EXPORT_SYMBOL_GPL(devm_release_action);
 
-- 
2.52.0

[PATCH 5/7] devres: add free_node callback to struct devres_node

[Danilo Krummrich]

Currently, there are three "subclasses" of struct devres_node, which are
struct devres, struct devres_group, struct devres_action.

release_nodes(), which only knows about the base struct devres_node,
assumes that for all "subclasses" struct devres_node is the first member
in the structure and calls kfree() on struct devres_node.

While this technically works, we can still improve semantical
correctness and type safety with a corresponding free_node() callback.

Additionally, we will need this callback soon in the Rust Devres code,
to allocate and free the required memory on the Rust side.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 57 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 12 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index df015ef155e0..2006fe411b49 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -18,10 +18,12 @@
 
 struct devres_node;
 typedef void (*dr_node_release_t)(struct device *dev, struct devres_node *node);
+typedef void (*dr_node_free_t)(struct devres_node *node);
 
 struct devres_node {
 	struct list_head		entry;
 	dr_node_release_t		release;
+	dr_node_free_t			free_node;
 	const char			*name;
 	size_t				size;
 };
@@ -46,10 +48,18 @@ struct devres_group {
 	/* -- 8 pointers */
 };
 
-static void devres_node_init(struct devres_node *node, dr_node_release_t release)
+static void devres_node_init(struct devres_node *node,
+			     dr_node_release_t release,
+			     dr_node_free_t free_node)
 {
 	INIT_LIST_HEAD(&node->entry);
 	node->release = release;
+	node->free_node = free_node;
+}
+
+static inline void free_node(struct devres_node *node)
+{
+	node->free_node(node);
 }
 
 static void set_node_dbginfo(struct devres_node *node, const char *name,
@@ -124,6 +134,13 @@ static void dr_node_release(struct device *dev, struct devres_node *node)
 	dr->release(dev, dr->data);
 }
 
+static void dr_node_free(struct devres_node *node)
+{
+	struct devres *dr = container_of(node, struct devres, node);
+
+	kfree(dr);
+}
+
 static __always_inline struct devres *alloc_dr(dr_release_t release,
 					       size_t size, gfp_t gfp, int nid)
 {
@@ -141,7 +158,7 @@ static __always_inline struct devres *alloc_dr(dr_release_t release,
 	if (!(gfp & __GFP_ZERO))
 		memset(dr, 0, offsetof(struct devres, data));
 
-	devres_node_init(&dr->node, dr_node_release);
+	devres_node_init(&dr->node, dr_node_release, dr_node_free);
 	dr->release = release;
 	return dr;
 }
@@ -231,6 +248,11 @@ void devres_for_each_res(struct device *dev, dr_release_t release,
 }
 EXPORT_SYMBOL_GPL(devres_for_each_res);
 
+static inline void free_dr(struct devres *dr)
+{
+	free_node(&dr->node);
+}
+
 /**
  * devres_free - Free device resource data
  * @res: Pointer to devres data to free
@@ -243,7 +265,7 @@ void devres_free(void *res)
 		struct devres *dr = container_of(res, struct devres, data);
 
 		BUG_ON(!list_empty(&dr->node.entry));
-		kfree(dr);
+		free_dr(dr);
 	}
 }
 EXPORT_SYMBOL_GPL(devres_free);
@@ -518,13 +540,10 @@ static void release_nodes(struct device *dev, struct list_head *todo)
 {
 	struct devres_node *node, *tmp;
 
-	/* Release.  Note that devres, devres_action and devres_group are
-	 * handled as devres_node in the following loop.  This is safe.
-	 */
 	list_for_each_entry_safe_reverse(node, tmp, todo, entry) {
 		devres_log(dev, node, "REL");
 		node->release(dev, node);
-		kfree(node);
+		free_node(node);
 	}
 }
 
@@ -557,6 +576,13 @@ int devres_release_all(struct device *dev)
 	return cnt;
 }
 
+static void devres_group_free(struct devres_node *node)
+{
+	struct devres_group *grp = container_of(node, struct devres_group, node[0]);
+
+	kfree(grp);
+}
+
 /**
  * devres_open_group - Open a new devres group
  * @dev: Device to open devres group for
@@ -578,8 +604,8 @@ void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
 	if (unlikely(!grp))
 		return NULL;
 
-	devres_node_init(&grp->node[0], &group_open_release);
-	devres_node_init(&grp->node[1], &group_close_release);
+	devres_node_init(&grp->node[0], &group_open_release, devres_group_free);
+	devres_node_init(&grp->node[1], &group_close_release, NULL);
 	set_node_dbginfo(&grp->node[0], "grp<", 0);
 	set_node_dbginfo(&grp->node[1], "grp>", 0);
 	grp->id = grp;
@@ -750,6 +776,13 @@ static void devm_action_release(struct device *dev, struct devres_node *node)
 	devres->action.action(devres->action.data);
 }
 
+static void devm_action_free(struct devres_node *node)
+{
+	struct devres_action *action = container_of(node, struct devres_action, node);
+
+	kfree(action);
+}
+
 /**
  * __devm_add_action() - add a custom action to list of managed resources
  * @dev: Device that owns the action
@@ -768,7 +801,7 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
 	if (!devres)
 		return -ENOMEM;
 
-	devres_node_init(&devres->node, devm_action_release);
+	devres_node_init(&devres->node, devm_action_release, devm_action_free);
 	set_node_dbginfo(&devres->node, name, sizeof(*devres));
 
 	devres->action.data = data;
@@ -1012,7 +1045,7 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
 	old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
 	if (!old_dr) {
 		spin_unlock_irqrestore(&dev->devres_lock, flags);
-		kfree(new_dr);
+		free_dr(new_dr);
 		WARN(1, "Memory chunk not managed or managed by a different device.");
 		return NULL;
 	}
@@ -1032,7 +1065,7 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
 	 * list. This is also the reason why we must not use devm_kfree() - the
 	 * links are no longer valid.
 	 */
-	kfree(old_dr);
+	free_dr(old_dr);
 
 	return new_dr->data;
 }
-- 
2.52.0

[PATCH 6/7] devres: use guard(spinlock_irqsave) where applicable

[Danilo Krummrich]

Use guard(spinlock_irqsave)(&dev->devres_lock) where it improves the
code.

Some places still use manual spin_lock_irqsave() and spin_unlock() as
changing it to use a scoped_guard() would result in unnecessary churn.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 2006fe411b49..58e2f82a30f6 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -228,12 +228,11 @@ void devres_for_each_res(struct device *dev, dr_release_t release,
 {
 	struct devres_node *node;
 	struct devres_node *tmp;
-	unsigned long flags;
 
 	if (!fn)
 		return;
 
-	spin_lock_irqsave(&dev->devres_lock, flags);
+	guard(spinlock_irqsave)(&dev->devres_lock);
 	list_for_each_entry_safe_reverse(node, tmp,
 			&dev->devres_head, entry) {
 		struct devres *dr = container_of(node, struct devres, node);
@@ -244,7 +243,6 @@ void devres_for_each_res(struct device *dev, dr_release_t release,
 			continue;
 		fn(dev, dr->data, data);
 	}
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
 }
 EXPORT_SYMBOL_GPL(devres_for_each_res);
 
@@ -330,14 +328,12 @@ void *devres_find(struct device *dev, dr_release_t release,
 		  dr_match_t match, void *match_data)
 {
 	struct devres *dr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&dev->devres_lock, flags);
+	guard(spinlock_irqsave)(&dev->devres_lock);
 	dr = find_dr(dev, release, match, match_data);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
-
 	if (dr)
 		return dr->data;
+
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(devres_find);
@@ -396,18 +392,15 @@ void *devres_remove(struct device *dev, dr_release_t release,
 		    dr_match_t match, void *match_data)
 {
 	struct devres *dr;
-	unsigned long flags;
 
-	spin_lock_irqsave(&dev->devres_lock, flags);
+	guard(spinlock_irqsave)(&dev->devres_lock);
 	dr = find_dr(dev, release, match, match_data);
 	if (dr) {
 		list_del_init(&dr->node.entry);
 		devres_log(dev, &dr->node, "REM");
+		return dr->data;
 	}
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
 
-	if (dr)
-		return dr->data;
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(devres_remove);
@@ -655,17 +648,13 @@ static struct devres_group *find_group(struct device *dev, void *id)
 void devres_close_group(struct device *dev, void *id)
 {
 	struct devres_group *grp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->devres_lock, flags);
 
+	guard(spinlock_irqsave)(&dev->devres_lock);
 	grp = find_group(dev, id);
 	if (grp)
 		add_dr(dev, &grp->node[1]);
 	else
 		WARN_ON(1);
-
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
 }
 EXPORT_SYMBOL_GPL(devres_close_group);
 
-- 
2.52.0

[PATCH 7/7] devres: remove unnecessary unlocks in devres_release_group()

[Danilo Krummrich]

There is no need to call spin_unlock_irqrestore() in every conditional
block, as release_nodes() can safely be called with an empty list, in
case we hit the "if else" or "else" case.

We do not use a scoped_guard() here to not unnecessarily change the
indentation level.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 58e2f82a30f6..0fca73b56168 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -708,7 +708,6 @@ int devres_release_group(struct device *dev, void *id)
 	int cnt = 0;
 
 	spin_lock_irqsave(&dev->devres_lock, flags);
-
 	grp = find_group(dev, id);
 	if (grp) {
 		struct list_head *first = &grp->node[0].entry;
@@ -718,20 +717,18 @@ int devres_release_group(struct device *dev, void *id)
 			end = grp->node[1].entry.next;
 
 		cnt = remove_nodes(dev, first, end, &todo);
-		spin_unlock_irqrestore(&dev->devres_lock, flags);
-
-		release_nodes(dev, &todo);
 	} else if (list_empty(&dev->devres_head)) {
 		/*
 		 * dev is probably dying via devres_release_all(): groups
 		 * have already been removed and are on the process of
 		 * being released - don't touch and don't warn.
 		 */
-		spin_unlock_irqrestore(&dev->devres_lock, flags);
 	} else {
 		WARN_ON(1);
-		spin_unlock_irqrestore(&dev->devres_lock, flags);
 	}
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+
+	release_nodes(dev, &todo);
 
 	return cnt;
 }
-- 
2.52.0

Re: [PATCH 0/7] Miscellaneous devres improvements

[Greg KH]

On Tue, Feb 03, 2026 at 12:48:13AM +0100, Danilo Krummrich wrote:
> This patch series provides some miscellaneous devres improvements.
> 
> The most notable change is a more clear separation between the base struct
> devres_node and its "subclasses" (such as struct devres and struct devres_group)
> as well as the mixup between struct devres and struct devres_group.
> 
> This serves three purposes.
> 
>   (1) Better maintainability due to clear type separation.
> 
>   (2) No waste of memory due to unnecessary minimal alignment for devres
>       actions, by introducing its own struct devres_action "subclass".
> 
>   (3) Bootstrapping for Rust devres code to embed a struct devres_node itself
>       to avoid both the unnecessary minimum alignment of struct devres and the
>       indirection through devres action avoiding unnecessary additional memory
>       allocations. [1]
> 
> However, (3) is just for completeness, the series also stands on its own.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/commit/?h=devres/devres_node&id=6da8ca499fa65992d71c5cb7a51df7260666e695
> 
> Danilo Krummrich (7):
>   devres: fix missing node debug info in devm_krealloc()
>   devres: add devres_node_add()
>   devres: add devres_node_init()
>   devres: don't require ARCH_DMA_MINALIGN for devres actions
>   devres: add free_node callback to struct devres_node
>   devres: use guard(spinlock_irqsave) where applicable
>   devres: remove unnecessary unlocks in devres_release_group()
> 
>  drivers/base/devres.c | 248 ++++++++++++++++++++++++++++--------------
>  1 file changed, 165 insertions(+), 83 deletions(-)
> 
> 
> base-commit: 289b14592cefe95f7d0ef334873c12b96ce3824f
> -- 
> 2.52.0
> 
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 0/7] Miscellaneous devres improvements

[Danilo Krummrich]

On Tue Feb 3, 2026 at 12:48 AM CET, Danilo Krummrich wrote:

Applied to driver-core-testing, thanks!

> Danilo Krummrich (7):
>   devres: fix missing node debug info in devm_krealloc()
>   devres: add devres_node_add()
>   devres: add devres_node_init()
>   devres: don't require ARCH_DMA_MINALIGN for devres actions

    [ Add missing node->release check in devres_for_each_res() and
      find_dr(); use kzalloc_obj(). - Danilo ]

>   devres: add free_node callback to struct devres_node
>   devres: use guard(spinlock_irqsave) where applicable
>   devres: remove unnecessary unlocks in devres_release_group()