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

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

[Douglas Anderson]

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.

Cc: stable@vger.kernel.org
Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
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.

 drivers/base/core.c                 | 24 +++++++++----------
 drivers/bus/imx-weim.c              |  2 +-
 drivers/i2c/i2c-core-of.c           |  2 +-
 drivers/net/phy/mdio_bus_provider.c |  4 ++--
 drivers/of/base.c                   |  2 +-
 drivers/of/dynamic.c                |  2 +-
 drivers/of/platform.c               |  2 +-
 drivers/spi/spi.c                   |  2 +-
 include/linux/fwnode.h              | 37 ++++++++++++++++++++++-------
 9 files changed, 48 insertions(+), 29 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 791f9e444df8..f65492a4afc8 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -182,7 +182,7 @@ void fw_devlink_purge_absent_suppliers(struct fwnode_handle *fwnode)
 	if (fwnode->dev)
 		return;
 
-	fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
+	fwnode_set_flag(fwnode, FWNODE_FLAG_NOT_DEVICE);
 	fwnode_links_purge_consumers(fwnode);
 
 	fwnode_for_each_available_child_node(fwnode, child)
@@ -228,7 +228,7 @@ static void __fw_devlink_pickup_dangling_consumers(struct fwnode_handle *fwnode,
 	if (fwnode->dev && fwnode->dev->bus)
 		return;
 
-	fwnode->flags |= FWNODE_FLAG_NOT_DEVICE;
+	fwnode_set_flag(fwnode, FWNODE_FLAG_NOT_DEVICE);
 	__fwnode_links_move_consumers(fwnode, new_sup);
 
 	fwnode_for_each_available_child_node(fwnode, child)
@@ -1012,7 +1012,7 @@ static void device_links_missing_supplier(struct device *dev)
 static bool dev_is_best_effort(struct device *dev)
 {
 	return (fw_devlink_best_effort && dev->can_match) ||
-		(dev->fwnode && (dev->fwnode->flags & FWNODE_FLAG_BEST_EFFORT));
+		(dev->fwnode && (fwnode_test_flag(dev->fwnode, FWNODE_FLAG_BEST_EFFORT)));
 }
 
 static struct fwnode_handle *fwnode_links_check_suppliers(
@@ -1723,11 +1723,11 @@ bool fw_devlink_is_strict(void)
 
 static void fw_devlink_parse_fwnode(struct fwnode_handle *fwnode)
 {
-	if (fwnode->flags & FWNODE_FLAG_LINKS_ADDED)
+	if (fwnode_test_flag(fwnode, FWNODE_FLAG_LINKS_ADDED))
 		return;
 
 	fwnode_call_int_op(fwnode, add_links);
-	fwnode->flags |= FWNODE_FLAG_LINKS_ADDED;
+	fwnode_set_flag(fwnode, FWNODE_FLAG_LINKS_ADDED);
 }
 
 static void fw_devlink_parse_fwtree(struct fwnode_handle *fwnode)
@@ -1885,7 +1885,7 @@ static bool fwnode_init_without_drv(struct fwnode_handle *fwnode)
 	struct device *dev;
 	bool ret;
 
-	if (!(fwnode->flags & FWNODE_FLAG_INITIALIZED))
+	if (!(fwnode_test_flag(fwnode, FWNODE_FLAG_INITIALIZED)))
 		return false;
 
 	dev = get_dev_from_fwnode(fwnode);
@@ -2001,10 +2001,10 @@ static bool __fw_devlink_relax_cycles(struct fwnode_handle *con_handle,
 	 * We aren't trying to find all cycles. Just a cycle between con and
 	 * sup_handle.
 	 */
-	if (sup_handle->flags & FWNODE_FLAG_VISITED)
+	if (fwnode_test_flag(sup_handle, FWNODE_FLAG_VISITED))
 		return false;
 
-	sup_handle->flags |= FWNODE_FLAG_VISITED;
+	fwnode_set_flag(sup_handle, FWNODE_FLAG_VISITED);
 
 	/* Termination condition. */
 	if (sup_handle == con_handle) {
@@ -2074,7 +2074,7 @@ static bool __fw_devlink_relax_cycles(struct fwnode_handle *con_handle,
 	}
 
 out:
-	sup_handle->flags &= ~FWNODE_FLAG_VISITED;
+	fwnode_clear_flag(sup_handle, FWNODE_FLAG_VISITED);
 	put_device(sup_dev);
 	put_device(con_dev);
 	put_device(par_dev);
@@ -2127,7 +2127,7 @@ static int fw_devlink_create_devlink(struct device *con,
 	 * When such a flag is set, we can't create device links where P is the
 	 * supplier of C as that would delay the probe of C.
 	 */
-	if (sup_handle->flags & FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD &&
+	if (fwnode_test_flag(sup_handle, FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD) &&
 	    fwnode_is_ancestor_of(sup_handle, con->fwnode))
 		return -EINVAL;
 
@@ -2150,7 +2150,7 @@ static int fw_devlink_create_devlink(struct device *con,
 	else
 		flags = FW_DEVLINK_FLAGS_PERMISSIVE;
 
-	if (sup_handle->flags & FWNODE_FLAG_NOT_DEVICE)
+	if (fwnode_test_flag(sup_handle, FWNODE_FLAG_NOT_DEVICE))
 		sup_dev = fwnode_get_next_parent_dev(sup_handle);
 	else
 		sup_dev = get_dev_from_fwnode(sup_handle);
@@ -2162,7 +2162,7 @@ static int fw_devlink_create_devlink(struct device *con,
 		 * supplier device indefinitely.
 		 */
 		if (sup_dev->links.status == DL_DEV_NO_DRIVER &&
-		    sup_handle->flags & FWNODE_FLAG_INITIALIZED) {
+		    fwnode_test_flag(sup_handle, FWNODE_FLAG_INITIALIZED)) {
 			dev_dbg(con,
 				"Not linking %pfwf - dev might never probe\n",
 				sup_handle);
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 83d623d97f5f..f735e0462c55 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -332,7 +332,7 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
 			 * fw_devlink doesn't skip adding consumers to this
 			 * device.
 			 */
-			rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
+			fwnode_clear_flag(&rd->dn->fwnode, FWNODE_FLAG_NOT_DEVICE);
 			if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
 				dev_err(&pdev->dev,
 					"Failed to create child device '%pOF'\n",
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index eb7fb202355f..354a88d0599e 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -180,7 +180,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 		 * Clear the flag before adding the device so that fw_devlink
 		 * doesn't skip adding consumers to this device.
 		 */
-		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
+		fwnode_clear_flag(&rd->dn->fwnode, FWNODE_FLAG_NOT_DEVICE);
 		client = of_i2c_register_device(adap, rd->dn);
 		if (IS_ERR(client)) {
 			dev_err(&adap->dev, "failed to create client for '%pOF'\n",
diff --git a/drivers/net/phy/mdio_bus_provider.c b/drivers/net/phy/mdio_bus_provider.c
index 4b0637405740..fd691c5424ea 100644
--- a/drivers/net/phy/mdio_bus_provider.c
+++ b/drivers/net/phy/mdio_bus_provider.c
@@ -294,8 +294,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 		return -EINVAL;
 
 	if (bus->parent && bus->parent->of_node)
-		bus->parent->of_node->fwnode.flags |=
-					FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD;
+		fwnode_set_flag(&bus->parent->of_node->fwnode,
+				FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD);
 
 	WARN(bus->state != MDIOBUS_ALLOCATED &&
 	     bus->state != MDIOBUS_UNREGISTERED,
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 57420806c1a2..8d1972e18161 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1915,7 +1915,7 @@ void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		if (name)
 			of_stdout = of_find_node_opts_by_path(name, &of_stdout_options);
 		if (of_stdout)
-			of_stdout->fwnode.flags |= FWNODE_FLAG_BEST_EFFORT;
+			fwnode_set_flag(&of_stdout->fwnode, FWNODE_FLAG_BEST_EFFORT);
 	}
 
 	if (!of_aliases)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1a06175def37..ade288372101 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -225,7 +225,7 @@ static void __of_attach_node(struct device_node *np)
 	np->sibling = np->parent->child;
 	np->parent->child = np;
 	of_node_clear_flag(np, OF_DETACHED);
-	np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
+	fwnode_set_flag(&np->fwnode, FWNODE_FLAG_NOT_DEVICE);
 
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ba591fbceb56..7eeaf8e27b5b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -742,7 +742,7 @@ static int of_platform_notify(struct notifier_block *nb,
 		 * Clear the flag before adding the device so that fw_devlink
 		 * doesn't skip adding consumers to this device.
 		 */
-		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
+		fwnode_clear_flag(&rd->dn->fwnode, FWNODE_FLAG_NOT_DEVICE);
 		/* pdev_parent may be NULL when no bus platform device */
 		pdev_parent = of_find_device_by_node(parent);
 		pdev = of_platform_device_create(rd->dn, NULL,
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 61f7bde8c7fb..ba8098f1a88c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4938,7 +4938,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 		 * Clear the flag before adding the device so that fw_devlink
 		 * doesn't skip adding consumers to this device.
 		 */
-		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
+		fwnode_clear_flag(&rd->dn->fwnode, FWNODE_FLAG_NOT_DEVICE);
 		spi = of_register_spi_device(ctlr, rd->dn);
 		put_device(&ctlr->dev);
 
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 097be89487bf..4f7f80155f69 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -15,6 +15,7 @@
 #define _LINUX_FWNODE_H_
 
 #include <linux/bits.h>
+#include <linux/bitops.h>
 #include <linux/err.h>
 #include <linux/list.h>
 #include <linux/types.h>
@@ -42,12 +43,12 @@ struct device;
  *		suppliers. Only enforce ordering with suppliers that have
  *		drivers.
  */
-#define FWNODE_FLAG_LINKS_ADDED			BIT(0)
-#define FWNODE_FLAG_NOT_DEVICE			BIT(1)
-#define FWNODE_FLAG_INITIALIZED			BIT(2)
-#define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	BIT(3)
-#define FWNODE_FLAG_BEST_EFFORT			BIT(4)
-#define FWNODE_FLAG_VISITED			BIT(5)
+#define FWNODE_FLAG_LINKS_ADDED			0
+#define FWNODE_FLAG_NOT_DEVICE			1
+#define FWNODE_FLAG_INITIALIZED			2
+#define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD	3
+#define FWNODE_FLAG_BEST_EFFORT			4
+#define FWNODE_FLAG_VISITED			5
 
 struct fwnode_handle {
 	struct fwnode_handle *secondary;
@@ -57,7 +58,7 @@ struct fwnode_handle {
 	struct device *dev;
 	struct list_head suppliers;
 	struct list_head consumers;
-	u8 flags;
+	unsigned long flags;
 };
 
 /*
@@ -212,6 +213,24 @@ static inline void fwnode_init(struct fwnode_handle *fwnode,
 	INIT_LIST_HEAD(&fwnode->suppliers);
 }
 
+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);
+}
+
+static inline bool fwnode_test_flag(struct fwnode_handle *fwnode,
+				    unsigned int bit)
+{
+	return test_bit(bit, &fwnode->flags);
+}
+
 static inline void fwnode_dev_initialized(struct fwnode_handle *fwnode,
 					  bool initialized)
 {
@@ -219,9 +238,9 @@ static inline void fwnode_dev_initialized(struct fwnode_handle *fwnode,
 		return;
 
 	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);
 }
 
 int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
-- 
2.53.0.851.ga537e3e6e9-goog

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@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

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

[Wolfram Sang]

Hi Doug,

thanks for tackling this issue! I agree it should be fixed, just
wondered about one thing:

> While flags are often modified while under the "fwnode_link_lock",
> this is not universally true.

Is it a possibility to use the lock in all code paths instead?
Because...

>  	struct list_head consumers;
> -	u8 flags;
> +	unsigned long flags;

... this change costs some memory on every system. Maybe it can be
avoided?

Happy hacking,

   Wolfram

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

[Andy Shevchenko]

On Tue, Mar 17, 2026 at 08:26:53AM +0100, Wolfram Sang wrote:

...

> Thanks for tackling this issue! I agree it should be fixed, just
> wondered about one thing:
> 
> > While flags are often modified while under the "fwnode_link_lock",
> > this is not universally true.
> 
> Is it a possibility to use the lock in all code paths instead?
> Because...
> 
> >  	struct list_head consumers;
> > -	u8 flags;
> > +	unsigned long flags;
> 
> ... this change costs some memory on every system. Maybe it can be
> avoided?

How much memory does it cost? On most 64-bit architectures is +4 bytes,
rarely +0 bytes, on m68k it might be +2bytes. On 32-bit it most likely
+0 bytes. I expect that 64-bit machines will cope with this bump.

-- 
With Best Regards,
Andy Shevchenko

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

[Wolfram Sang]

> > ... this change costs some memory on every system. Maybe it can be
> > avoided?
> 
> How much memory does it cost? On most 64-bit architectures is +4 bytes,
> rarely +0 bytes, on m68k it might be +2bytes. On 32-bit it most likely
> +0 bytes. I expect that 64-bit machines will cope with this bump.

I am not opposing that the issue should be fixed. If it is not possible
to take the lock everywhere, this is a proper solution. But if we don't
have to use more memory, then we could save it. Our new SoC easily has
'struct device' in the hundreds.

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

[Andy Shevchenko]

On Tue, Mar 17, 2026 at 08:42:08AM +0100, Wolfram Sang wrote:

> > > ... this change costs some memory on every system. Maybe it can be
> > > avoided?
> > 
> > How much memory does it cost? On most 64-bit architectures is +4 bytes,
> > rarely +0 bytes, on m68k it might be +2bytes. On 32-bit it most likely
> > +0 bytes. I expect that 64-bit machines will cope with this bump.
> 
> I am not opposing that the issue should be fixed. If it is not possible
> to take the lock everywhere, this is a proper solution. But if we don't
> have to use more memory, then we could save it. Our new SoC easily has
> 'struct device' in the hundreds.

What's the alignment for the u8 member in your SoC? 4 bytes or 8 bytes?
(I assume it's 64-bit SoC.)

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 17, 2026 at 10:04:43AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 17, 2026 at 08:42:08AM +0100, Wolfram Sang wrote:
> 
> > > > ... this change costs some memory on every system. Maybe it can be
> > > > avoided?
> > > 
> > > How much memory does it cost? On most 64-bit architectures is +4 bytes,
> > > rarely +0 bytes, on m68k it might be +2bytes. On 32-bit it most likely
> > > +0 bytes. I expect that 64-bit machines will cope with this bump.
> > 
> > I am not opposing that the issue should be fixed. If it is not possible
> > to take the lock everywhere, this is a proper solution. But if we don't
> > have to use more memory, then we could save it. Our new SoC easily has
> > 'struct device' in the hundreds.
> 
> What's the alignment for the u8 member in your SoC? 4 bytes or 8 bytes?
> (I assume it's 64-bit SoC.)

FWIW, with the given change it will be still inside 64-byte data structure
which most likely occupies a single cache line (before this patch and after
as well).

-- 
With Best Regards,
Andy Shevchenko

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

[Wolfram Sang]

> > What's the alignment for the u8 member in your SoC? 4 bytes or 8 bytes?
> > (I assume it's 64-bit SoC.)
> 
> FWIW, with the given change it will be still inside 64-byte data structure
> which most likely occupies a single cache line (before this patch and after
> as well).

I consider this directon of the discussion irrelevant. If the number is
(maybe? That's to be discussed!) needlessly bigger than 0, then it
doesn't matter how big the number is.

Why don't you like the idea of taking the lock?

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

[Danilo Krummrich]

On Tue Mar 17, 2026 at 9:42 AM CET, Wolfram Sang wrote:
>
>> > What's the alignment for the u8 member in your SoC? 4 bytes or 8 bytes?
>> > (I assume it's 64-bit SoC.)
>> 
>> FWIW, with the given change it will be still inside 64-byte data structure
>> which most likely occupies a single cache line (before this patch and after
>> as well).
>
> I consider this directon of the discussion irrelevant. If the number is
> (maybe? That's to be discussed!) needlessly bigger than 0, then it
> doesn't matter how big the number is.
>
> Why don't you like the idea of taking the lock?

Which lock are we talking about, the device lock? If so, please not use it to
protect arbitrary other structures.

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

[Andy Shevchenko]

On Tue, Mar 17, 2026 at 09:42:38AM +0100, Wolfram Sang wrote:
> 
> > > What's the alignment for the u8 member in your SoC? 4 bytes or 8 bytes?
> > > (I assume it's 64-bit SoC.)
> > 
> > FWIW, with the given change it will be still inside 64-byte data structure
> > which most likely occupies a single cache line (before this patch and after
> > as well).
> 
> I consider this directon of the discussion irrelevant. If the number is
> (maybe? That's to be discussed!) needlessly bigger than 0, then it
> doesn't matter how big the number is.

That's why it was written 'FWIW', so it doesn't worth :-)

> Why don't you like the idea of taking the lock?

Like Danilo I am also not sure what lock protects fwnode accesses.

-- 
With Best Regards,
Andy Shevchenko

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

[Wolfram Sang]

> Like Danilo I am also not sure what lock protects fwnode accesses.

This is basically the question I asked to Doug. If he also don't see
one, let's take this patch as is.

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

[Rafael J. Wysocki]

On Tue, Mar 17, 2026 at 11:34 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > Like Danilo I am also not sure what lock protects fwnode accesses.
>
> This is basically the question I asked to Doug. If he also don't see
> one, let's take this patch as is.

I don't recall using any lock for this purpose.

IIRC, the original assumption was that device fwnodes wouldn't be
updated once set.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 264 bytes --]

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;

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Wolfram Sang]

> I don't recall using any lock for this purpose.

In that case:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

counts as an ack for i2c, too, of course.

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

[Geert Uytterhoeven]

Hi Andy,

On Tue, 17 Mar 2026 at 08:34, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Mar 17, 2026 at 08:26:53AM +0100, Wolfram Sang wrote:
>
> ...
>
> > Thanks for tackling this issue! I agree it should be fixed, just
> > wondered about one thing:
> >
> > > While flags are often modified while under the "fwnode_link_lock",
> > > this is not universally true.
> >
> > Is it a possibility to use the lock in all code paths instead?
> > Because...
> >
> > >     struct list_head consumers;
> > > -   u8 flags;
> > > +   unsigned long flags;
> >
> > ... this change costs some memory on every system. Maybe it can be
> > avoided?
>
> How much memory does it cost? On most 64-bit architectures is +4 bytes,
> rarely +0 bytes, on m68k it might be +2bytes. On 32-bit it most likely
> +0 bytes. I expect that 64-bit machines will cope with this bump.

On all architectures with natural alignment of pointers and longs,
it won't cost a thing: struct list_head contains pointers, so the
struct must be padded to a multiple of 4 or 8 bytes anyway.
On m68k[*],  it will cost 2 bytes, as the existing padding is just a
single byte.

[*] Iff m68k ever switches to 32-bit alignment, there won't be an
    additional cost due to the change of flags here, but of course
    there would be a cost all over the place.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Andy Shevchenko]

On Wed, Mar 18, 2026 at 09:41:44AM +0100, Geert Uytterhoeven wrote:
> On Tue, 17 Mar 2026 at 08:34, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Mar 17, 2026 at 08:26:53AM +0100, Wolfram Sang wrote:

...

> > > Thanks for tackling this issue! I agree it should be fixed, just
> > > wondered about one thing:
> > >
> > > > While flags are often modified while under the "fwnode_link_lock",
> > > > this is not universally true.
> > >
> > > Is it a possibility to use the lock in all code paths instead?
> > > Because...
> > >
> > > >     struct list_head consumers;
> > > > -   u8 flags;
> > > > +   unsigned long flags;
> > >
> > > ... this change costs some memory on every system. Maybe it can be
> > > avoided?
> >
> > How much memory does it cost? On most 64-bit architectures is +4 bytes,
> > rarely +0 bytes, on m68k it might be +2bytes. On 32-bit it most likely
> > +0 bytes. I expect that 64-bit machines will cope with this bump.
> 
> On all architectures with natural alignment of pointers and longs,
> it won't cost a thing: struct list_head contains pointers, so the
> struct must be padded to a multiple of 4 or 8 bytes anyway.
> On m68k[*],  it will cost 2 bytes, as the existing padding is just a
> single byte.
> 
> [*] Iff m68k ever switches to 32-bit alignment, there won't be an
>     additional cost due to the change of flags here, but of course
>     there would be a cost all over the place.

Thanks!

Yes, the worst case is 64-bit architecture with 4-byte alignment. This
will cost +4 bytes.

But I think we are really wasting time on this part of the discussion
(and any similar) as long as nobody targets struct device or any other
BIG FAT data type, that will bring much more benefit than saving 4 bytes
in some struct fwnode_handle.

-- 
With Best Regards,
Andy Shevchenko