Re: [PATCH v3] devres: Refactor using guards

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Lucas,

On 12/06/2024 11:26, Lucas Stach wrote:

Am Dienstag, dem 11.06.2024 um 11:37 +0200 schrieb Andrea Calabrese:

Code refactoring using the recent guard and scoped_guard macros
for automatic cleanup of the spinlocks. This does not change the
effective behaviour of the kernel, but guarantees a cleaned-up exit from
each lock, automatically avoiding potential deadlocks.

Signed-off-by: Andrea Calabrese <andrea.calabrese@xxxxxxxxxxxxxxxxxxxx>

---
Changed commit message from V2. Also changed title, shortened the file
name.

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 3df0025d12aa..a98720e0cb2b 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -194,14 +194,12 @@ 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); 
-	list_for_each_entry_safe_reverse(node, tmp,
-			&dev->devres_head, entry) {
+	guard(spinlock)(&dev->devres_lock);

This is not equivalent to the current code. You are dropping the
_irqsave part of the locking scheme, totally changing the semantics
here. This issue is present in multiple places in this patch.

Regards,
Lucas
I don't see where is the issue here, as I am using both guard and scoped_guard similarly to how they are used in drivers/gpio/gpiolib-cdev.c, which I took as a reference for the usage of the construct. If so, probably we may  both be using it wrong. 
+	list_for_each_entry_safe_reverse(node, tmp, &dev->devres_head, entry) {
  		struct devres *dr = container_of(node, struct devres, node);
if (node->release != release) 
@@ -210,7 +208,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);
@@ -243,11 +240,9 @@ 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); 
+	guard(spinlock)(&dev->devres_lock);
  	add_dr(dev, &dr->node);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
  }
  EXPORT_SYMBOL_GPL(devres_add);
@@ -287,11 +282,8 @@ 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)(&dev->devres_lock);
  	dr = find_dr(dev, release, match, match_data);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
if (dr) 
  		return dr->data;
@@ -318,16 +310,14 @@ void * devres_get(struct device *dev, void *new_res,
  {
  	struct devres *new_dr = container_of(new_res, struct devres, data);
  	struct devres *dr;
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->devres_lock, flags);
-	dr = find_dr(dev, new_dr->node.release, match, match_data);
-	if (!dr) {
-		add_dr(dev, &new_dr->node);
-		dr = new_dr;
-		new_res = NULL;
+	scoped_guard(spinlock, &dev->devres_lock) {
+		dr = find_dr(dev, new_dr->node.release, match, match_data);
+		if (!dr) {
+			add_dr(dev, &new_dr->node);
+			dr = new_dr;
+			new_res = NULL;
+		}
  	}
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
  	devres_free(new_res);
return dr->data; 
@@ -353,15 +343,13 @@ 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);
-	dr = find_dr(dev, release, match, match_data);
-	if (dr) {
-		list_del_init(&dr->node.entry);
-		devres_log(dev, &dr->node, "REM");
+	scoped_guard(spinlock, &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");
+		}
  	}
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
if (dr) 
  		return dr->data;
@@ -516,7 +504,6 @@ static void release_nodes(struct device *dev, struct list_head *todo)
   */
  int devres_release_all(struct device *dev)
  {
-	unsigned long flags;
  	LIST_HEAD(todo);
  	int cnt;
@@ -528,9 +515,9 @@ int devres_release_all(struct device *dev) 
  	if (list_empty(&dev->devres_head))
  		return 0;
- spin_lock_irqsave(&dev->devres_lock, flags); 
-	cnt = remove_nodes(dev, dev->devres_head.next, &dev->devres_head, &todo);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
+	scoped_guard(spinlock, &dev->devres_lock) {
+		cnt = remove_nodes(dev, dev->devres_head.next, &dev->devres_head, &todo);
+	}
release_nodes(dev, &todo); 
  	return cnt;
@@ -552,7 +539,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))
@@ -568,9 +554,8 @@ void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
  	if (id)
  		grp->id = id;
- spin_lock_irqsave(&dev->devres_lock, flags); 
+	guard(spinlock)(&dev->devres_lock);
  	add_dr(dev, &grp->node[0]);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
  	return grp->id;
  }
  EXPORT_SYMBOL_GPL(devres_open_group);
@@ -609,17 +594,14 @@ 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)(&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);
@@ -635,19 +617,16 @@ EXPORT_SYMBOL_GPL(devres_close_group); 
  void devres_remove_group(struct device *dev, void *id)
  {
  	struct devres_group *grp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->devres_lock, flags);
-
-	grp = find_group(dev, id);
-	if (grp) {
-		list_del_init(&grp->node[0].entry);
-		list_del_init(&grp->node[1].entry);
-		devres_log(dev, &grp->node[0], "REM");
-	} else
-		WARN_ON(1);
- spin_unlock_irqrestore(&dev->devres_lock, flags); 
+	scoped_guard(spinlock, &dev->devres_lock) {
+		grp = find_group(dev, id);
+		if (grp) {
+			list_del_init(&grp->node[0].entry);
+			list_del_init(&grp->node[1].entry);
+			devres_log(dev, &grp->node[0], "REM");
+		} else
+			WARN_ON(1);
+	}
kfree(grp); 
  }
@@ -668,11 +647,10 @@ EXPORT_SYMBOL_GPL(devres_remove_group);
  int devres_release_group(struct device *dev, void *id)
  {
  	struct devres_group *grp;
-	unsigned long flags;
  	LIST_HEAD(todo);
  	int cnt = 0;
- spin_lock_irqsave(&dev->devres_lock, flags); 
+	guard(spinlock)(&dev->devres_lock);
grp = find_group(dev, id); 
  	if (grp) {
@@ -683,12 +661,9 @@ 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 {
  		WARN_ON(1);
-		spin_unlock_irqrestore(&dev->devres_lock, flags);
  	}
return cnt; 
@@ -860,7 +835,6 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
  {
  	size_t total_new_size, total_old_size;
  	struct devres *old_dr, *new_dr;
-	unsigned long flags;
if (unlikely(!new_size)) { 
  		devm_kfree(dev, ptr);
@@ -906,20 +880,17 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
  	 * The spinlock protects the linked list against concurrent
  	 * modifications but not the resource itself.
  	 */
-	spin_lock_irqsave(&dev->devres_lock, flags);
+	scoped_guard(spinlock, &dev->devres_lock) {
+		old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
+		if (!old_dr) {
+			kfree(new_dr);
+			WARN(1, "Memory chunk not managed or managed by a different device.");
+			return NULL;
+		}
- 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);
-		WARN(1, "Memory chunk not managed or managed by a different device.");
-		return NULL;
+		replace_dr(dev, &old_dr->node, &new_dr->node);
  	}
- replace_dr(dev, &old_dr->node, &new_dr->node); 
-
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
-
  	/*
  	 * We can copy the memory contents after releasing the lock as we're
  	 * no longer modifying the list links.
[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux