Re: [PATCH 3/5] gpiolib: provide gpiod_update_flags()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] gpiolib: gpiod_update_flags() open-codes set_mask_bits()
--

commit ecaf66699b4205d02175949ed385d130f83c667e
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

gpiolib: provide gpiod_update_flags()

This commit adds gpiod_update_flags() in drivers/gpio/gpiolib.h to atomically
update a masked set of bits in desc->flags using a try_cmpxchg() loop so
concurrent updates to other bits are not lost.

> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 650a702741dfb..10659ffcf2bae 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -9,6 +9,7 @@
>  #ifndef GPIOLIB_H
>  #define GPIOLIB_H
>  
> +#include <linux/atomic.h>
>  #include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -223,6 +224,26 @@ struct gpio_desc {
>  #endif
>  };
>  
> +/**
> + * gpiod_update_flags() - Atomically update several descriptor flags at once
> + * @desc: GPIO descriptor to update.
> + * @mask: Bits (not bit numbers) to modify.
> + * @val: New values for the bits selected by @mask.
> + *
> + * Replace the bits in @mask with the corresponding bits of @val in a single
> + * atomic step, so that no other context can observe a half-updated state of
> + * these bits. Bits outside of @mask are never rewritten with stale values, so
> + * concurrent atomic bit operations on them are preserved.
> + */
> +static inline void gpiod_update_flags(struct gpio_desc *desc,
> +				      unsigned long mask, unsigned long val)
> +{
> +	unsigned long old = READ_ONCE(desc->flags);
> +
> +	while (!try_cmpxchg(&desc->flags, &old, (old & ~mask) | (val & mask)))
> +		;
> +}

[Severity: Low]
This isn't a bug, but include/linux/bitops.h already provides set_mask_bits():

#define set_mask_bits(ptr, mask, bits)	\
({								\
	const typeof(*(ptr)) mask__ = (mask), bits__ = (bits);	\
	typeof(*(ptr)) old__, new__;				\
								\
	old__ = READ_ONCE(*(ptr));				\
	do {							\
		new__ = (old__ & ~mask__) | bits__;		\
	} while (!try_cmpxchg(ptr, &old__, new__));		\
								\
	old__;							\
})

Could gpiod_update_flags() call set_mask_bits(&desc->flags, mask, val & mask)
instead of open-coding the READ_ONCE() and try_cmpxchg() loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-gpio-desc-flags-atomic-v1-0-e23cedf619ba@oss.qualcomm.com?part=3