Re: [PATCH v3] devres: Refactor using guards

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

 



Am Mittwoch, dem 12.06.2024 um 12:00 +0200 schrieb Andrea Calabrese:
> 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.

Blindly copying from a another subsystem, which may have a very
different locking architecture, is never a good idea. Please educate
yourself about the difference between spin_lock, spin_lock_irq and
spin_lock_irqsave.

You need to use guard(spinlock_irqsave) here for the new code to be
equivalent to the code you are removing. Since this is supposed to be a
cleanup/simplification the new code _needs_ to keep the semantics of
the original code. You can not hide a total change in locking
architecture in a change like that.

Regards,
Lucas

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