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

[PATCH v2] 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()")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
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.

Changes in v2:
- Add/use fwnode_assign_flag() (Andy).

 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              | 45 +++++++++++++++++++++--------
 9 files changed, 53 insertions(+), 32 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..c1ebcc6fd896 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,16 +213,36 @@ 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 void fwnode_assign_flag(struct fwnode_handle *fwnode,
+				      unsigned int bit, bool value)
+{
+	assign_bit(bit, &fwnode->flags, value);
+}
+
+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)
 {
 	if (IS_ERR_OR_NULL(fwnode))
 		return;
-
-	if (initialized)
-		fwnode->flags |= FWNODE_FLAG_INITIALIZED;
-	else
-		fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;
+	fwnode_assign_flag(fwnode, FWNODE_FLAG_INITIALIZED, initialized);
 }
 
 int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
-- 
2.53.0.851.ga537e3e6e9-goog

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

[Rafael J. Wysocki]

On Tue, Mar 17, 2026 at 5:04 PM Douglas Anderson <dianders@chromium.org> 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.
>
> Cc: stable@vger.kernel.org
> Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Rafael J. Wysocki (Intel) <rafael@kernel.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.
>
> Changes in v2:
> - Add/use fwnode_assign_flag() (Andy).
>
>  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              | 45 +++++++++++++++++++++--------
>  9 files changed, 53 insertions(+), 32 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..c1ebcc6fd896 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,16 +213,36 @@ 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 void fwnode_assign_flag(struct fwnode_handle *fwnode,
> +                                     unsigned int bit, bool value)
> +{
> +       assign_bit(bit, &fwnode->flags, value);
> +}
> +
> +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)
>  {
>         if (IS_ERR_OR_NULL(fwnode))
>                 return;
> -
> -       if (initialized)
> -               fwnode->flags |= FWNODE_FLAG_INITIALIZED;
> -       else
> -               fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;
> +       fwnode_assign_flag(fwnode, FWNODE_FLAG_INITIALIZED, initialized);
>  }
>
>  int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
> --
> 2.53.0.851.ga537e3e6e9-goog
>
>

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

[Danilo Krummrich]

On 3/17/2026 5:11 PM, Rafael J. Wysocki wrote:
> On Tue, Mar 17, 2026 at 5:04 PM Douglas Anderson <dianders@chromium.org> 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.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Acked-by: Mark Brown <broonie@kernel.org>
>> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> 
> Rafael J. Wysocki (Intel) <rafael@kernel.org>

ACK or RB?

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

[Rafael J. Wysocki]

On Tue, Mar 17, 2026 at 5:20 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On 3/17/2026 5:11 PM, Rafael J. Wysocki wrote:
> > On Tue, Mar 17, 2026 at 5:04 PM Douglas Anderson <dianders@chromium.org> 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.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> Acked-by: Mark Brown <broonie@kernel.org>
> >> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >
> > Rafael J. Wysocki (Intel) <rafael@kernel.org>
>
> ACK or RB?

RB, sorry.

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

[Saravana Kannan]

On Tue, Mar 17, 2026 at 9:04 AM Douglas Anderson <dianders@chromium.org> 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.
>
> Cc: stable@vger.kernel.org
> Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 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.

Reviewed-by: Saravana Kannan <saravanak@kernel.org>

Thanks Doug. Hope this isn't the cause of the hisenbug. If you report
it here, I might be able to take a look at it too (no promises).


-Saravana

>
> Changes in v2:
> - Add/use fwnode_assign_flag() (Andy).
>
>  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              | 45 +++++++++++++++++++++--------
>  9 files changed, 53 insertions(+), 32 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..c1ebcc6fd896 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,16 +213,36 @@ 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 void fwnode_assign_flag(struct fwnode_handle *fwnode,
> +                                     unsigned int bit, bool value)
> +{
> +       assign_bit(bit, &fwnode->flags, value);
> +}
> +
> +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)
>  {
>         if (IS_ERR_OR_NULL(fwnode))
>                 return;
> -
> -       if (initialized)
> -               fwnode->flags |= FWNODE_FLAG_INITIALIZED;
> -       else
> -               fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;
> +       fwnode_assign_flag(fwnode, FWNODE_FLAG_INITIALIZED, initialized);
>  }
>
>  int fwnode_link_add(struct fwnode_handle *con, struct fwnode_handle *sup,
> --
> 2.53.0.851.ga537e3e6e9-goog
>

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

[Doug Anderson]

Hi,

On Thu, Mar 19, 2026 at 10:25 AM Saravana Kannan <saravanak@kernel.org> wrote:
>
> On Tue, Mar 17, 2026 at 9:04 AM Douglas Anderson <dianders@chromium.org> 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.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Acked-by: Mark Brown <broonie@kernel.org>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 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.
>
> Reviewed-by: Saravana Kannan <saravanak@kernel.org>

Thanks for the review!


> Thanks Doug. Hope this isn't the cause of the hisenbug. If you report
> it here, I might be able to take a look at it too (no promises).

I don't _think_ it fixes my bug, but I'm still not 100% sure because
the bug can take a day or so to reproduce and it appears to only
reproduce on official kernels built by the builder. :( This makes it
hard to say anything for certain and also hard for me to inject extra
debug logic.

I did finally capture a ramdump of a device in the bad state and
managed to inspect the state of all the fwnode and fwnode_link
objects.

The problem (simplified) is that I have a pinctrl device:

  max77779_pinctrl {
    pin1: pin1 {
      ...;
    };
    pin2: pin2 {
      ...;
    };
  };

...and then a client of the pinctrl device:

  client1 {
    pinctrl-0 = <&pin1>;
    ...
  };

  client2 {
    pinctrl-0 = <&pin2>;
    ...
  };

At parse time, we get a link between the "client1" node and the "pin1"
node (and "client2" and "pin2").

In a normal boot (where the bug doesn't reproduce), when
max77779_pinctrl finishes probing, all of the links to its children
(which don't have a "dev" associated with them) get moved to point to
"max77779_pinctrl" (__fw_devlink_pickup_dangling_consumers).

When I detect the bug, the max77779_pinctrl device clearly has
finished probing, but the client devices still point to the child
nodes.

In the debugger, I can inspect things. I can see:
* The fwnodes for "pin1" and "pin2" clearly don't have a "dev" node.
They also don't have "FWNODE_FLAG_NOT_DEVICE" set.
* The fwnode for "max77779_pinctrl" clearly has a dev node (and a bus).
* The fwnodes for "pin1" and "pin2" show zero consumers.
* The fwnodes for "client1" and "client2" _do_ still show suppliers.
* When I look at the fwnode_link between the clients and the pins, the
"supplier" points to the node for the pin.
* When I look at the fwnode_link between the clients and the pins, the
"c_hook" is non-empty.
* When I look at the fwnode_link between the clients and the pins, the
"consumer" points to the node for the client.
* When I look at the fwnode_link between the clients and the pins, the
"s_hook" is an empty list (next == prev).

It's a bit baffling because everything looks nice and consistent with
half the link being severed, which doesn't seem possible when looking
at the code. The code seems to always add to a consumer list and
supplier list at the same time and remove at the same time.

I've started to run out of ideas on how it could possibly get into
this state. If you have any, or want me to look at any other data
structures in the ramdump let me know.

-Doug

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

[Doug Anderson]

Hi,

On Thu, Mar 19, 2026 at 10:52 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Mar 19, 2026 at 10:25 AM Saravana Kannan <saravanak@kernel.org> wrote:
> >
> > On Tue, Mar 17, 2026 at 9:04 AM Douglas Anderson <dianders@chromium.org> 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.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Acked-by: Mark Brown <broonie@kernel.org>
> > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > 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.
> >
> > Reviewed-by: Saravana Kannan <saravanak@kernel.org>
>
> Thanks for the review!
>
>
> > Thanks Doug. Hope this isn't the cause of the hisenbug. If you report
> > it here, I might be able to take a look at it too (no promises).
>
> I don't _think_ it fixes my bug, but I'm still not 100% sure because
> the bug can take a day or so to reproduce and it appears to only
> reproduce on official kernels built by the builder. :( This makes it
> hard to say anything for certain and also hard for me to inject extra
> debug logic.

Just in case anyone out there was wracking their brains based on my
description of the bug...

I've made progress in getting the issue to reproduce even with debug
information added. With that, I've found that
device_links_driver_bound() is getting called where `dev->fwnode->dev`
is NULL. That prevents it from running the ever-important
__fw_devlink_pickup_dangling_consumers().

I can see that device_add() has started, but it just hasn't made it to
the `dev->fwnode->dev = dev;` line yet.  My printout next to that line
shows up _after_ my printout in device_links_driver_bound().

So obviously something can happen to cause the device to probe before
the call to bus_probe_device().

OK, I managed to get a stack crawl for when `dev->fwnode->dev ==
NULL`. It looks like this (FWIW, it's a 6.6 kernel but issue also
reproduces on our 6.12 kernel, and I see no reason it wouldn't
reproduce on mainline):

  Call trace:
  dump_backtrace+0xe8/0x108
  show_stack+0x18/0x28
  dump_stack_lvl+0x50/0x6c
  dump_stack+0x18/0x24
  device_links_driver_bound+0xa4/0x4b4
  driver_bound+0x48/0x1c4
  really_probe+0x244/0x374
  __driver_probe_device+0xa0/0x12c
  driver_probe_device+0x3c/0x218
  __driver_attach+0x110/0x1ec
  bus_for_each_dev+0x104/0x160
  driver_attach+0x24/0x34
  bus_add_driver+0x154/0x270
  driver_register+0x68/0x104
  __platform_driver_register+0x24/0x34
  init_module+0x20/0xfe4 [max77779_pmic_pinctrl
e09198e651272bc5df70245355346d6eb1ba3a8f]
  do_one_initcall+0xdc/0x360
  do_init_module+0x58/0x23c
  load_module+0xffc/0x1130
  __arm64_sys_finit_module+0x260/0x300
  invoke_syscall+0x58/0x114

It looks like what happens is that immediately after device_add()
calls bus_add_device() there's a possibility of another thread
inserting the module holding the device driver. That means that the
driver can start probing much earlier than we expect.

I've posted an RFC patch to fix this. If folks are interested, please review it:

https://lore.kernel.org/r/20260320200656.RFC.1.Id750b0fbcc94f23ed04b7aecabcead688d0d8c17@changeid

Given the stack crawl I got, I'm fairly certain that this will fix the
problem, but I'll also let reboot tests run over the weekend to
confirm.

-Doug

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

[Danilo Krummrich]

On Tue Mar 17, 2026 at 5:01 PM CET, 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.
>
> Cc: stable@vger.kernel.org
> Fixes: c2c724c868c4 ("driver core: Add fw_devlink_parse_fwtree()")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

    [ Fix fwnode_clear_flag() argument alignment, restore dropped blank
      line in fwnode_dev_initialized(), and remove unnecessary parentheses
      around fwnode_test_flag() calls. - Danilo ]

Applied to driver-core-testing, thanks!