* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-17 16:01 [PATCH v2] device property: Make modifications of fwnode "flags" thread safe Douglas Anderson
@ 2026-03-17 16:11 ` Rafael J. Wysocki
2026-03-17 16:20 ` Danilo Krummrich
2026-03-19 17:25 ` Saravana Kannan
2026-03-26 21:03 ` Danilo Krummrich
2 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2026-03-17 16:11 UTC (permalink / raw)
To: Douglas Anderson
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich, stable,
Andy Shevchenko, Mark Brown, Wolfram Sang, Andrew Lunn,
Daniel Scally, David S. Miller, Eric Dumazet, Fabio Estevam,
Frank Li, Heikki Krogerus, Heiner Kallweit, Jakub Kicinski,
Len Brown, Paolo Abeni, Pengutronix Kernel Team, Rob Herring,
Russell King, Sakari Ailus, Saravana Kannan, Sascha Hauer,
devicetree, driver-core, imx, linux-acpi, linux-arm-kernel,
linux-i2c, linux-kernel, linux-spi, netdev
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-17 16:11 ` Rafael J. Wysocki
@ 2026-03-17 16:20 ` Danilo Krummrich
2026-03-17 16:22 ` Rafael J. Wysocki
0 siblings, 1 reply; 8+ messages in thread
From: Danilo Krummrich @ 2026-03-17 16:20 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Douglas Anderson, Greg Kroah-Hartman, stable, Andy Shevchenko,
Mark Brown, Wolfram Sang, Andrew Lunn, Daniel Scally,
David S. Miller, Eric Dumazet, Fabio Estevam, Frank Li,
Heikki Krogerus, Heiner Kallweit, Jakub Kicinski, Len Brown,
Paolo Abeni, Pengutronix Kernel Team, Rob Herring, Russell King,
Sakari Ailus, Saravana Kannan, Sascha Hauer, devicetree,
driver-core, imx, linux-acpi, linux-arm-kernel, linux-i2c,
linux-kernel, linux-spi, netdev
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?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-17 16:20 ` Danilo Krummrich
@ 2026-03-17 16:22 ` Rafael J. Wysocki
0 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2026-03-17 16:22 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Rafael J. Wysocki, Douglas Anderson, Greg Kroah-Hartman, stable,
Andy Shevchenko, Mark Brown, Wolfram Sang, Andrew Lunn,
Daniel Scally, David S. Miller, Eric Dumazet, Fabio Estevam,
Frank Li, Heikki Krogerus, Heiner Kallweit, Jakub Kicinski,
Len Brown, Paolo Abeni, Pengutronix Kernel Team, Rob Herring,
Russell King, Sakari Ailus, Saravana Kannan, Sascha Hauer,
devicetree, driver-core, imx, linux-acpi, linux-arm-kernel,
linux-i2c, linux-kernel, linux-spi, netdev
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-17 16:01 [PATCH v2] device property: Make modifications of fwnode "flags" thread safe Douglas Anderson
2026-03-17 16:11 ` Rafael J. Wysocki
@ 2026-03-19 17:25 ` Saravana Kannan
2026-03-19 17:52 ` Doug Anderson
2026-03-26 21:03 ` Danilo Krummrich
2 siblings, 1 reply; 8+ messages in thread
From: Saravana Kannan @ 2026-03-19 17:25 UTC (permalink / raw)
To: Douglas Anderson
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich, stable,
Andy Shevchenko, Mark Brown, Wolfram Sang, Andrew Lunn,
Daniel Scally, David S. Miller, Eric Dumazet, Fabio Estevam,
Frank Li, Heikki Krogerus, Heiner Kallweit, Jakub Kicinski,
Len Brown, Paolo Abeni, Pengutronix Kernel Team, Rob Herring,
Russell King, Sakari Ailus, Saravana Kannan, Sascha Hauer,
devicetree, driver-core, imx, linux-acpi, linux-arm-kernel,
linux-i2c, linux-kernel, linux-spi, netdev
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-19 17:25 ` Saravana Kannan
@ 2026-03-19 17:52 ` Doug Anderson
2026-03-21 3:10 ` Doug Anderson
0 siblings, 1 reply; 8+ messages in thread
From: Doug Anderson @ 2026-03-19 17:52 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich, stable,
Andy Shevchenko, Mark Brown, Wolfram Sang, Andrew Lunn,
Daniel Scally, David S. Miller, Eric Dumazet, Fabio Estevam,
Frank Li, Heikki Krogerus, Heiner Kallweit, Jakub Kicinski,
Len Brown, Paolo Abeni, Pengutronix Kernel Team, Rob Herring,
Russell King, Sakari Ailus, Sascha Hauer, devicetree, driver-core,
imx, linux-acpi, linux-arm-kernel, linux-i2c, linux-kernel,
linux-spi, netdev
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-19 17:52 ` Doug Anderson
@ 2026-03-21 3:10 ` Doug Anderson
0 siblings, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2026-03-21 3:10 UTC (permalink / raw)
To: Saravana Kannan
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich, stable,
Andy Shevchenko, Mark Brown, Wolfram Sang, Andrew Lunn,
Daniel Scally, David S. Miller, Eric Dumazet, Fabio Estevam,
Frank Li, Heikki Krogerus, Heiner Kallweit, Jakub Kicinski,
Len Brown, Paolo Abeni, Pengutronix Kernel Team, Rob Herring,
Russell King, Sakari Ailus, Sascha Hauer, devicetree, driver-core,
imx, linux-acpi, linux-arm-kernel, linux-i2c, linux-kernel,
linux-spi, netdev
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] device property: Make modifications of fwnode "flags" thread safe
2026-03-17 16:01 [PATCH v2] device property: Make modifications of fwnode "flags" thread safe Douglas Anderson
2026-03-17 16:11 ` Rafael J. Wysocki
2026-03-19 17:25 ` Saravana Kannan
@ 2026-03-26 21:03 ` Danilo Krummrich
2 siblings, 0 replies; 8+ messages in thread
From: Danilo Krummrich @ 2026-03-26 21:03 UTC (permalink / raw)
To: Douglas Anderson
Cc: Greg Kroah-Hartman, Rafael J . Wysocki, stable, Andy Shevchenko,
Mark Brown, Wolfram Sang, Andrew Lunn, Daniel Scally,
David S. Miller, Eric Dumazet, Fabio Estevam, Frank Li,
Heikki Krogerus, Heiner Kallweit, Jakub Kicinski, Len Brown,
Paolo Abeni, Pengutronix Kernel Team, Rob Herring, Russell King,
Sakari Ailus, Saravana Kannan, Sascha Hauer, devicetree,
driver-core, imx, linux-acpi, linux-arm-kernel, linux-i2c,
linux-kernel, linux-spi, netdev
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!
^ permalink raw reply [flat|nested] 8+ messages in thread