[PATCH v3] devres: Refactor using guards
Lucas Stach
l.stach at pengutronix.de
Wed Jun 12 09:26:24 UTC 2024
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 at 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.
More information about the dri-devel
mailing list