Re: [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines

[Andy Shevchenko]

On Mon, Nov 09, 2026 at 11:27:09AM +0100, Bartosz Golaszewski wrote:
> linehandle_flags_to_desc_flags() and
> gpio_v2_line_config_flags_to_desc_flags() read the flags word with
> READ_ONCE(), modify the local copy and write it back with WRITE_ONCE().
> 
> The per-request config_mutex does not exclude sysfs, the irq core or
> drivers touching the same descriptor, so bits they set or clear
> concurrently can be lost.
> 
> Build the mask of bits owned by the line configuration and their new
> values locally and apply them with gpiod_update_flags(). The whole
> group changes in a single atomic step.
> 
> While at it: rename the functions whose semantics changed from
> "conversion" to "flags update" accordingly.

...

> @@ -3,6 +3,7 @@
>  #include <linux/anon_inodes.h>
>  #include <linux/atomic.h>
>  #include <linux/bitmap.h>
> +#include <linux/bitops.h>
>  #include <linux/build_bug.h>
>  #include <linux/cdev.h>
>  #include <linux/cleanup.h>

Unneeded hunk as explained in the previous reply.

...

> -static void linehandle_flags_to_desc_flags(u32 lflags, unsigned long *flagsp)
> +static void linehandle_update_desc_flags(u32 lflags, struct gpio_desc *desc)
>  {
> -	unsigned long flags = READ_ONCE(*flagsp);
> +	const unsigned long mask = BIT(GPIOD_FLAG_ACTIVE_LOW) |
> +				   BIT(GPIOD_FLAG_OPEN_DRAIN) |
> +				   BIT(GPIOD_FLAG_OPEN_SOURCE) |
> +				   BIT(GPIOD_FLAG_PULL_UP) |
> +				   BIT(GPIOD_FLAG_PULL_DOWN) |
> +				   BIT(GPIOD_FLAG_BIAS_DISABLE);

Usually we don't do such an assignments in the definition blocks.
I believe the split will look better.

> +	unsigned long flags = 0;
>  

	mask = ...;

(and with longer line you may combine close bits, like OD/OS or PD,PU, and
 DISABLE).

> -	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
> -		   lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
> -	assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags,
> -		   lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
> -	assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags,
> -		   lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
> -	assign_bit(GPIOD_FLAG_PULL_UP, &flags,
> -		   lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
> -	assign_bit(GPIOD_FLAG_PULL_DOWN, &flags,
> -		   lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
> -	assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags,
> -		   lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
> +	__assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
> +		     lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
> +	__assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags,
> +		     lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
> +	__assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags,
> +		     lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
> +	__assign_bit(GPIOD_FLAG_PULL_UP, &flags,
> +		     lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
> +	__assign_bit(GPIOD_FLAG_PULL_DOWN, &flags,
> +		     lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
> +	__assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags,
> +		     lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);

...

> +static void linereq_update_desc_flags(u64 lflags,
> +						    struct gpio_desc *desc)

This is ugliest part of this patch in my opinion. Just make it a single line.
`clang-format` is in use I assume?

...

> +	const unsigned long mask = BIT(GPIOD_FLAG_ACTIVE_LOW) |
> +				   BIT(GPIOD_FLAG_EDGE_RISING) |
> +				   BIT(GPIOD_FLAG_EDGE_FALLING) |
> +				   BIT(GPIOD_FLAG_OPEN_DRAIN) |
> +				   BIT(GPIOD_FLAG_OPEN_SOURCE) |
> +				   BIT(GPIOD_FLAG_PULL_UP) |
> +				   BIT(GPIOD_FLAG_PULL_DOWN) |
> +				   BIT(GPIOD_FLAG_BIAS_DISABLE) |
> +				   BIT(GPIOD_FLAG_EVENT_CLOCK_REALTIME) |
> +				   BIT(GPIOD_FLAG_EVENT_CLOCK_HTE);

Same comment as before.

> +	unsigned long flags = 0;

-- 
With Best Regards,
Andy Shevchenko