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

Andy Shevchenko
2026-03-17T07:10:37Z

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@linux.intel.com>

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