Re: [PATCH] device property: Make modifications of fwnode "flags" thread safe

[Andy Shevchenko]

On Mon, Mar 16, 2026 at 03:42:06PM -0700, Douglas Anderson wrote:
> In various places in the kernel, we modify the fwnode "flags" member
> by doing either:
>   fwnode->flags |= SOME_FLAG;
>   fwnode->flags &= ~SOME_FLAG;
> 
> This type of modification is not thread-safe. If two threads are both
> mucking with the flags at the same time then one can clobber the
> other.
> 
> While flags are often modified while under the "fwnode_link_lock",
> this is not universally true.
> 
> Create some accessor functions for setting, clearing, and testing the
> FWNODE flags and move all users to these accessor functions. New
> accessor functions use set_bit() and clear_bit(), which are
> thread-safe.

In general I like the idea (independently on the treewide patch or a series).
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

See one nit-pick below, though.

...

> While this patch is not known for sure to fix any specific issues, it
> seems possible that it could fix some rare problems. I'm currently
> trying to track down a hard-to-reproduce heisenbug and one (currently
> unproven) theory I had was that the fwnode flags could be getting
> messed up like this. Even if turns out not to fix my heisenbug,
> though, this seems like a worthwhile change to take.

Totally agree. This makes code more OOP like and robust against subtle
modifications.

...

> +static inline void fwnode_set_flag(struct fwnode_handle *fwnode,
> +				   unsigned int bit)
> +{
> +	set_bit(bit, &fwnode->flags);
> +}
> +
> +static inline void fwnode_clear_flag(struct fwnode_handle *fwnode,
> +				   unsigned int bit)
> +{
> +	clear_bit(bit, &fwnode->flags);
> +}

Perhaps you also want to have fwnode_assign_flag() (assign_bit() underneath)
as...

> +static inline bool fwnode_test_flag(struct fwnode_handle *fwnode,
> +				    unsigned int bit)
> +{
> +	return test_bit(bit, &fwnode->flags);
> +}

>  	if (initialized)
> -		fwnode->flags |= FWNODE_FLAG_INITIALIZED;
> +		fwnode_set_flag(fwnode, FWNODE_FLAG_INITIALIZED);
>  	else
> -		fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;
> +		fwnode_clear_flag(fwnode, FWNODE_FLAG_INITIALIZED);

...at least one immediate user here.

-- 
With Best Regards,
Andy Shevchenko