| Message ID | 20240611093710.1066510-1-andrea.calabrese@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] devres: Refactor using guards | expand |
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@amarulasolutions.com> > > --- > 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 > + 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.
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@amarulasolutions.com> >> >> --- >> 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.
On Wed, Jun 12, 2024 at 12:00:06PM +0200, Andrea Calabrese wrote: > 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@amarulasolutions.com> > > > > > > --- > > > 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. Look at the difference between calling: spin_lock(&lock); and spin_lock_irqsave(&lock, flags); They are functionally NOT the same. thanks, greg k-h
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@amarulasolutions.com> > > > > > > --- > > > 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.
Hi Greg, On 12/06/2024 12:05, Greg KH wrote: > On Wed, Jun 12, 2024 at 12:00:06PM +0200, Andrea Calabrese wrote: >> 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@amarulasolutions.com> >>>> >>>> --- >>>> 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. > Look at the difference between calling: > spin_lock(&lock); > and > spin_lock_irqsave(&lock, flags); > > They are functionally NOT the same. > > thanks, > > greg k-h Ouch, I saw it now... I will fix it and send a V4. Thank you! Andrea
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); + 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.
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@amarulasolutions.com> --- Changed commit message from V2. Also changed title, shortened the file name.