[PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Douglas Anderson]

The moment we link a "struct device" into the list of devices for the
bus, it's possible probe can happen. This is because another thread
can load the driver at any time and that can cause the device to
probe. This has been seen in practice with a stack crawl that looks
like this [1]:

  really_probe()
  __driver_probe_device()
  driver_probe_device()
  __driver_attach()
  bus_for_each_dev()
  driver_attach()
  bus_add_driver()
  driver_register()
  __platform_driver_register()
  init_module() [some module]
  do_one_initcall()
  do_init_module()
  load_module()
  __arm64_sys_finit_module()
  invoke_syscall()

As a result of the above, it was seen that device_links_driver_bound()
could be called for the device before "dev->fwnode->dev" was
assigned. This prevented __fw_devlink_pickup_dangling_consumers() from
being called which meant that other devices waiting on our driver's
sub-nodes were stuck deferring forever.

It's believed that this problem is showing up suddenly for two
reasons:
1. Android has recently (last ~1 year) implemented an optimization to
   the order it loads modules [2]. When devices opt-in to this faster
   loading, modules are loaded one-after-the-other very quickly. This
   is unlike how other distributions do it. The reproduction of this
   problem has only been seen on devices that opt-in to Android's
   "parallel module loading".
2. Android devices typically opt-in to fw_devlink, and the most
   noticeable issue is the NULL "dev->fwnode->dev" in
   device_links_driver_bound(). fw_devlink is somewhat new code and
   also not in use by all Linux devices.

Even though the specific symptom where "dev->fwnode->dev" wasn't
assigned could be fixed by moving that assignment higher in
device_add(), other parts of device_add() (like the call to
device_pm_add()) are also important to run before probe. Only moving
the "dev->fwnode->dev" assignment would likely fix the current
symptoms but lead to difficult-to-debug problems in the future.

Fix the problem by preventing probe until device_add() has run far
enough that the device is ready to probe. If somehow we end up trying
to probe before we're allowed, __driver_probe_device() will return
-EPROBE_DEFER which will make certain the device is noticed.

In the race condition that was seen with Android's faster module
loading, we will temporarily add the device to the deferred list and
then take it off immediately when device_add() probes the device.

Instead of adding another flag to the bitfields already in "struct
device", instead add a new "flags" field and use that. This allows us
to freely change the bit from different thread without holding the
device lock and without worrying about corrupting nearby bits.

[1] Captured on a machine running a downstream 6.6 kernel
[2] https://cs.android.com/android/platform/superproject/main/+/main:system/core/libmodprobe/libmodprobe.cpp?q=LoadModulesParallel

Cc: stable@vger.kernel.org
Fixes: 2023c610dc54 ("Driver core: add new device to bus's list before probing")
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Rafael J. Wysocki (Intel) <rafael@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
v1: https://lore.kernel.org/r/20260320200656.RFC.1.Id750b0fbcc94f23ed04b7aecabcead688d0d8c17@changeid
v2: https://lore.kernel.org/r/20260330072839.v2.1.Id750b0fbcc94f23ed04b7aecabcead688d0d8c17@changeid
v3: https://lore.kernel.org/r/20260403005005.30424-1-dianders@chromium.org/

As of v2 this feels like a very safe change. It doesn't change the
ordering of any steps of probe and it _just_ prevents the early probe
from happening.

I ran tests where I turned the printout "Device not ready_to_probe" on
and I could see the printout happening, evidence of the race occurring
from other printouts, and things successfully being resolved.

I kept Alan and Rafael's Reviewed-by tags in v3 even though I changed
the implementation to use "flags" as per Danilo's request. This didn't
seem like a major enough change to remove tags, but please shout if
you'd like your tag removed.

Changes in v4:
- Use accessor functions for flags

Changes in v3:
- Use a new "flags" bitfield
- Add missing \n in probe error message

Changes in v2:
- Instead of adjusting the ordering, use "ready_to_probe" flag

 drivers/base/core.c    | 13 +++++++++++++
 drivers/base/dd.c      | 12 ++++++++++++
 include/linux/device.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 09b98f02f559..f07745659de3 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3688,6 +3688,19 @@ int device_add(struct device *dev)
 		fw_devlink_link_device(dev);
 	}
 
+	/*
+	 * The moment the device was linked into the bus's "klist_devices" in
+	 * bus_add_device() then it's possible that probe could have been
+	 * attempted in a different thread via userspace loading a driver
+	 * matching the device. "ready_to_prove" being unset would have
+	 * blocked those attempts. Now that all of the above initialization has
+	 * happened, unblock probe. If probe happens through another thread
+	 * after this point but before bus_probe_device() runs then it's fine.
+	 * bus_probe_device() -> device_initial_probe() -> __device_attach()
+	 * will notice (under device_lock) that the device is already bound.
+	 */
+	dev_set_ready_to_probe(dev);
+
 	bus_probe_device(dev);
 
 	/*
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 37c7e54e0e4c..8ec93128ea98 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -848,6 +848,18 @@ static int __driver_probe_device(const struct device_driver *drv, struct device
 	if (dev->driver)
 		return -EBUSY;
 
+	/*
+	 * In device_add(), the "struct device" gets linked into the subsystem's
+	 * list of devices and broadcast to userspace (via uevent) before we're
+	 * quite ready to probe. Those open pathways to driver probe before
+	 * we've finished enough of device_add() to reliably support probe.
+	 * Detect this and tell other pathways to try again later. device_add()
+	 * itself will also try to probe immediately after setting
+	 * "ready_to_probe".
+	 */
+	if (!dev_ready_to_probe(dev))
+		return dev_err_probe(dev, -EPROBE_DEFER, "Device not ready to probe\n");
+
 	dev->can_match = true;
 	dev_dbg(dev, "bus: '%s': %s: matched device with driver %s\n",
 		drv->bus->name, __func__, drv->name);
diff --git a/include/linux/device.h b/include/linux/device.h
index e65d564f01cd..5eb0b22958e4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -458,6 +458,21 @@ struct device_physical_location {
 	bool lid;
 };
 
+/**
+ * enum struct_device_flags - Flags in struct device
+ *
+ * Each flag should have a set of accessor functions created via
+ * __create_dev_flag_accessors() for each access.
+ *
+ * @DEV_FLAG_READY_TO_PROBE: If set then device_add() has finished enough
+ *		initialization that probe could be called.
+ */
+enum struct_device_flags {
+	DEV_FLAG_READY_TO_PROBE = 0,
+
+	DEV_FLAG_COUNT
+};
+
 /**
  * struct device - The basic device structure
  * @parent:	The device's "parent" device, the device to which it is attached.
@@ -553,6 +568,7 @@ struct device_physical_location {
  * @dma_skip_sync: DMA sync operations can be skipped for coherent buffers.
  * @dma_iommu: Device is using default IOMMU implementation for DMA and
  *		doesn't rely on dma_ops structure.
+ * @flags:	DEV_FLAG_XXX flags. Use atomic bitfield operations to modify.
  *
  * At the lowest level, every device in a Linux system is represented by an
  * instance of struct device. The device structure contains the information
@@ -675,8 +691,34 @@ struct device {
 #ifdef CONFIG_IOMMU_DMA
 	bool			dma_iommu:1;
 #endif
+
+	DECLARE_BITMAP(flags, DEV_FLAG_COUNT);
 };
 
+#define __create_dev_flag_accessors(accessor_name, flag_name) \
+static inline bool dev_##accessor_name(const struct device *dev) \
+{ \
+	return test_bit(flag_name, dev->flags); \
+} \
+static inline void dev_set_##accessor_name(struct device *dev) \
+{ \
+	set_bit(flag_name, dev->flags); \
+} \
+static inline void dev_clear_##accessor_name(struct device *dev) \
+{ \
+	clear_bit(flag_name, dev->flags); \
+} \
+static inline void dev_assign_##accessor_name(struct device *dev, bool value) \
+{ \
+	assign_bit(flag_name, dev->flags, value); \
+} \
+static inline bool dev_test_and_set_##accessor_name(struct device *dev) \
+{ \
+	return test_and_set_bit(flag_name, dev->flags); \
+}
+
+__create_dev_flag_accessors(ready_to_probe, DEV_FLAG_READY_TO_PROBE);
+
 /**
  * struct device_link - Device link representation.
  * @supplier: The device on the supplier end of the link.
-- 
2.53.0.1213.gd9a14994de-goog

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Danilo Krummrich]

On Sat Apr 4, 2026 at 2:04 AM CEST, Douglas Anderson wrote:
> +#define __create_dev_flag_accessors(accessor_name, flag_name) \
> +static inline bool dev_##accessor_name(const struct device *dev) \
> +{ \
> +	return test_bit(flag_name, dev->flags); \
> +} \
> +static inline void dev_set_##accessor_name(struct device *dev) \
> +{ \
> +	set_bit(flag_name, dev->flags); \
> +} \
> +static inline void dev_clear_##accessor_name(struct device *dev) \
> +{ \
> +	clear_bit(flag_name, dev->flags); \
> +} \
> +static inline void dev_assign_##accessor_name(struct device *dev, bool value) \
> +{ \
> +	assign_bit(flag_name, dev->flags, value); \
> +} \
> +static inline bool dev_test_and_set_##accessor_name(struct device *dev) \
> +{ \
> +	return test_and_set_bit(flag_name, dev->flags); \
> +}
> +
> +__create_dev_flag_accessors(ready_to_probe, DEV_FLAG_READY_TO_PROBE);

Since this is a public header included in a lot of places, it might be worth to
#undef the macro once done defining all accessors.

> +
>  /**
>   * struct device_link - Device link representation.
>   * @supplier: The device on the supplier end of the link.

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Danilo Krummrich]

On Sat Apr 4, 2026 at 2:04 AM CEST, Douglas Anderson wrote:
> Instead of adding another flag to the bitfields already in "struct
> device", instead add a new "flags" field and use that. This allows us
> to freely change the bit from different thread without holding the
> device lock and without worrying about corrupting nearby bits.

I was just about to pick up this patch series (Greg mentioned to pick it up next
week, but we agreed offlist that I will pick it now, so it gets a few more
cycles in linux-next).

Due to this, taking a second glance at the code, I noticed the below issue.

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 09b98f02f559..f07745659de3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3688,6 +3688,19 @@ int device_add(struct device *dev)
>  		fw_devlink_link_device(dev);
>  	}
>  
> +	/*
> +	 * The moment the device was linked into the bus's "klist_devices" in
> +	 * bus_add_device() then it's possible that probe could have been
> +	 * attempted in a different thread via userspace loading a driver
> +	 * matching the device. "ready_to_prove" being unset would have
> +	 * blocked those attempts. Now that all of the above initialization has
> +	 * happened, unblock probe. If probe happens through another thread
> +	 * after this point but before bus_probe_device() runs then it's fine.
> +	 * bus_probe_device() -> device_initial_probe() -> __device_attach()
> +	 * will notice (under device_lock) that the device is already bound.
> +	 */
> +	dev_set_ready_to_probe(dev);

By converting this to a bitop, we now avoid races with other bitfields (such as
dev->can_match), but I think we still need to take the device lock for this one
specifically:

	Task 0 (device_add):		Task 1 (__driver_probe_device):

	dev->fwnode->dev = dev;
					device_lock(dev);
	device_lock(dev);               	if (dev_ready_to_probe())
	dev_set_ready_to_probe()			access(fwnode->dev);
	device_unlock(dev);		device_unlock(dev);

Otherwise, nothing prevents the above dev->fwnode->dev = dev assignment to be
re-ordered with dev_set_ready_to_probe() and we are back to the problem the
commit attempts to solve in the first place.

(Technically, this could also be solved with explicit memory barriers - here and
below -, but __driver_probe_device() is always called with the device lock held,
so just taking the device lock seems *much* simpler. Also, in the absolute
majority of cases the lock should be uncontended in device_add() anyways.)

> +
>  	bus_probe_device(dev);
>  
>  	/*
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 37c7e54e0e4c..8ec93128ea98 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -848,6 +848,18 @@ static int __driver_probe_device(const struct device_driver *drv, struct device
>  	if (dev->driver)
>  		return -EBUSY;
>  
> +	/*
> +	 * In device_add(), the "struct device" gets linked into the subsystem's
> +	 * list of devices and broadcast to userspace (via uevent) before we're
> +	 * quite ready to probe. Those open pathways to driver probe before
> +	 * we've finished enough of device_add() to reliably support probe.
> +	 * Detect this and tell other pathways to try again later. device_add()
> +	 * itself will also try to probe immediately after setting
> +	 * "ready_to_probe".
> +	 */
> +	if (!dev_ready_to_probe(dev))
> +		return dev_err_probe(dev, -EPROBE_DEFER, "Device not ready to probe\n");
> +
>  	dev->can_match = true;

Focused on ordering from the above, I also noticed that this ordering of
dev_ready_to_probe() and dev->can_match = true is actually pretty subtle and we
should add the following comment.

	/*
	 * Set can_match = true after calling dev_ready_to_probe(), so
	 * driver_deferred_probe_add() won't actually add the device to the
	 * deferred probe list when dev_ready_to_probe() returns false.
	 *
	 * When dev_ready_to_probe() returns false, it means that device_add()
	 * will do another probe() attempt for us.
	 */

As it would be nice to land this for v7.1-rc1, I can apply both changes on
apply, i.e. not need to resend AFAIC.

Greg, Rafael: does that work for you?

Thanks,
Danilo

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Doug Anderson]

Hi,

On Sun, Apr 5, 2026 at 1:58 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sat Apr 4, 2026 at 2:04 AM CEST, Douglas Anderson wrote:
> > Instead of adding another flag to the bitfields already in "struct
> > device", instead add a new "flags" field and use that. This allows us
> > to freely change the bit from different thread without holding the
> > device lock and without worrying about corrupting nearby bits.
>
> I was just about to pick up this patch series (Greg mentioned to pick it up next
> week, but we agreed offlist that I will pick it now, so it gets a few more
> cycles in linux-next).
>
> Due to this, taking a second glance at the code, I noticed the below issue.
>
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 09b98f02f559..f07745659de3 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -3688,6 +3688,19 @@ int device_add(struct device *dev)
> >               fw_devlink_link_device(dev);
> >       }
> >
> > +     /*
> > +      * The moment the device was linked into the bus's "klist_devices" in
> > +      * bus_add_device() then it's possible that probe could have been
> > +      * attempted in a different thread via userspace loading a driver
> > +      * matching the device. "ready_to_prove" being unset would have
> > +      * blocked those attempts. Now that all of the above initialization has
> > +      * happened, unblock probe. If probe happens through another thread
> > +      * after this point but before bus_probe_device() runs then it's fine.
> > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> > +      * will notice (under device_lock) that the device is already bound.
> > +      */
> > +     dev_set_ready_to_probe(dev);
>
> By converting this to a bitop, we now avoid races with other bitfields (such as
> dev->can_match), but I think we still need to take the device lock for this one
> specifically:
>
>         Task 0 (device_add):            Task 1 (__driver_probe_device):
>
>         dev->fwnode->dev = dev;
>                                         device_lock(dev);
>         device_lock(dev);                       if (dev_ready_to_probe())
>         dev_set_ready_to_probe()                        access(fwnode->dev);
>         device_unlock(dev);             device_unlock(dev);
>
> Otherwise, nothing prevents the above dev->fwnode->dev = dev assignment to be
> re-ordered with dev_set_ready_to_probe() and we are back to the problem the
> commit attempts to solve in the first place.

Ah, that sounds like a reasonable concern, and I agree that taking the
device_lock() here seems like the cleanest solution.


> > @@ -848,6 +848,18 @@ static int __driver_probe_device(const struct device_driver *drv, struct device
> >       if (dev->driver)
> >               return -EBUSY;
> >
> > +     /*
> > +      * In device_add(), the "struct device" gets linked into the subsystem's
> > +      * list of devices and broadcast to userspace (via uevent) before we're
> > +      * quite ready to probe. Those open pathways to driver probe before
> > +      * we've finished enough of device_add() to reliably support probe.
> > +      * Detect this and tell other pathways to try again later. device_add()
> > +      * itself will also try to probe immediately after setting
> > +      * "ready_to_probe".
> > +      */
> > +     if (!dev_ready_to_probe(dev))
> > +             return dev_err_probe(dev, -EPROBE_DEFER, "Device not ready to probe\n");
> > +
> >       dev->can_match = true;
>
> Focused on ordering from the above, I also noticed that this ordering of
> dev_ready_to_probe() and dev->can_match = true is actually pretty subtle and we
> should add the following comment.
>
>         /*
>          * Set can_match = true after calling dev_ready_to_probe(), so
>          * driver_deferred_probe_add() won't actually add the device to the
>          * deferred probe list when dev_ready_to_probe() returns false.
>          *
>          * When dev_ready_to_probe() returns false, it means that device_add()
>          * will do another probe() attempt for us.
>          */

Sure. That seems useful for future readers.


> As it would be nice to land this for v7.1-rc1, I can apply both changes on
> apply, i.e. not need to resend AFAIC.

Thanks! I'm happy to resend a new version if need be, but I'm also
happy if you want to make changes when applying.

-Doug

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Greg Kroah-Hartman]

On Sun, Apr 05, 2026 at 03:39:26PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sun, Apr 5, 2026 at 1:58 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sat Apr 4, 2026 at 2:04 AM CEST, Douglas Anderson wrote:
> > > Instead of adding another flag to the bitfields already in "struct
> > > device", instead add a new "flags" field and use that. This allows us
> > > to freely change the bit from different thread without holding the
> > > device lock and without worrying about corrupting nearby bits.
> >
> > I was just about to pick up this patch series (Greg mentioned to pick it up next
> > week, but we agreed offlist that I will pick it now, so it gets a few more
> > cycles in linux-next).
> >
> > Due to this, taking a second glance at the code, I noticed the below issue.
> >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 09b98f02f559..f07745659de3 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -3688,6 +3688,19 @@ int device_add(struct device *dev)
> > >               fw_devlink_link_device(dev);
> > >       }
> > >
> > > +     /*
> > > +      * The moment the device was linked into the bus's "klist_devices" in
> > > +      * bus_add_device() then it's possible that probe could have been
> > > +      * attempted in a different thread via userspace loading a driver
> > > +      * matching the device. "ready_to_prove" being unset would have
> > > +      * blocked those attempts. Now that all of the above initialization has
> > > +      * happened, unblock probe. If probe happens through another thread
> > > +      * after this point but before bus_probe_device() runs then it's fine.
> > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> > > +      * will notice (under device_lock) that the device is already bound.
> > > +      */
> > > +     dev_set_ready_to_probe(dev);
> >
> > By converting this to a bitop, we now avoid races with other bitfields (such as
> > dev->can_match), but I think we still need to take the device lock for this one
> > specifically:
> >
> >         Task 0 (device_add):            Task 1 (__driver_probe_device):
> >
> >         dev->fwnode->dev = dev;
> >                                         device_lock(dev);
> >         device_lock(dev);                       if (dev_ready_to_probe())
> >         dev_set_ready_to_probe()                        access(fwnode->dev);
> >         device_unlock(dev);             device_unlock(dev);
> >
> > Otherwise, nothing prevents the above dev->fwnode->dev = dev assignment to be
> > re-ordered with dev_set_ready_to_probe() and we are back to the problem the
> > commit attempts to solve in the first place.
> 
> Ah, that sounds like a reasonable concern, and I agree that taking the
> device_lock() here seems like the cleanest solution.
> 
> 
> > > @@ -848,6 +848,18 @@ static int __driver_probe_device(const struct device_driver *drv, struct device
> > >       if (dev->driver)
> > >               return -EBUSY;
> > >
> > > +     /*
> > > +      * In device_add(), the "struct device" gets linked into the subsystem's
> > > +      * list of devices and broadcast to userspace (via uevent) before we're
> > > +      * quite ready to probe. Those open pathways to driver probe before
> > > +      * we've finished enough of device_add() to reliably support probe.
> > > +      * Detect this and tell other pathways to try again later. device_add()
> > > +      * itself will also try to probe immediately after setting
> > > +      * "ready_to_probe".
> > > +      */
> > > +     if (!dev_ready_to_probe(dev))
> > > +             return dev_err_probe(dev, -EPROBE_DEFER, "Device not ready to probe\n");
> > > +
> > >       dev->can_match = true;
> >
> > Focused on ordering from the above, I also noticed that this ordering of
> > dev_ready_to_probe() and dev->can_match = true is actually pretty subtle and we
> > should add the following comment.
> >
> >         /*
> >          * Set can_match = true after calling dev_ready_to_probe(), so
> >          * driver_deferred_probe_add() won't actually add the device to the
> >          * deferred probe list when dev_ready_to_probe() returns false.
> >          *
> >          * When dev_ready_to_probe() returns false, it means that device_add()
> >          * will do another probe() attempt for us.
> >          */
> 
> Sure. That seems useful for future readers.
> 
> 
> > As it would be nice to land this for v7.1-rc1, I can apply both changes on
> > apply, i.e. not need to resend AFAIC.
> 
> Thanks! I'm happy to resend a new version if need be, but I'm also
> happy if you want to make changes when applying.

New version is always best :)

thanks,

greg k-h

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Danilo Krummrich]

On Mon Apr 6, 2026 at 8:39 AM CEST, Greg Kroah-Hartman wrote:
> On Sun, Apr 05, 2026 at 03:39:26PM -0700, Doug Anderson wrote:
>> Thanks! I'm happy to resend a new version if need be, but I'm also
>> happy if you want to make changes when applying.
>
> New version is always best :)

Yeah, a new version sounds good -- thanks Doug!

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Marc Zyngier]

Hi Doug,

On Sat, 04 Apr 2026 01:04:55 +0100,
Douglas Anderson <dianders@chromium.org> wrote:
> 
> The moment we link a "struct device" into the list of devices for the
> bus, it's possible probe can happen. This is because another thread
> can load the driver at any time and that can cause the device to
> probe. This has been seen in practice with a stack crawl that looks
> like this [1]:

[...]

> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 09b98f02f559..f07745659de3 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3688,6 +3688,19 @@ int device_add(struct device *dev)
>  		fw_devlink_link_device(dev);
>  	}
>  
> +	/*
> +	 * The moment the device was linked into the bus's "klist_devices" in
> +	 * bus_add_device() then it's possible that probe could have been
> +	 * attempted in a different thread via userspace loading a driver
> +	 * matching the device. "ready_to_prove" being unset would have

nit; s/prove/probe/

> +	 * blocked those attempts. Now that all of the above initialization has
> +	 * happened, unblock probe. If probe happens through another thread
> +	 * after this point but before bus_probe_device() runs then it's fine.
> +	 * bus_probe_device() -> device_initial_probe() -> __device_attach()
> +	 * will notice (under device_lock) that the device is already bound.
> +	 */
> +	dev_set_ready_to_probe(dev);

I think this lacks some ordering properties that we should be allowed
to rely on. In this case, the 'ready_to_probe' flag being set should
that all of the data structures are observable by another CPU.

Unfortunately, this doesn't seem to be the case, see below.

> +
>  	bus_probe_device(dev);
>  
>  	/*
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 37c7e54e0e4c..8ec93128ea98 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -848,6 +848,18 @@ static int __driver_probe_device(const struct device_driver *drv, struct device
>  	if (dev->driver)
>  		return -EBUSY;
>  
> +	/*
> +	 * In device_add(), the "struct device" gets linked into the subsystem's
> +	 * list of devices and broadcast to userspace (via uevent) before we're
> +	 * quite ready to probe. Those open pathways to driver probe before
> +	 * we've finished enough of device_add() to reliably support probe.
> +	 * Detect this and tell other pathways to try again later. device_add()
> +	 * itself will also try to probe immediately after setting
> +	 * "ready_to_probe".
> +	 */
> +	if (!dev_ready_to_probe(dev))
> +		return dev_err_probe(dev, -EPROBE_DEFER, "Device not ready to probe\n");
> +
>  	dev->can_match = true;
>  	dev_dbg(dev, "bus: '%s': %s: matched device with driver %s\n",
>  		drv->bus->name, __func__, drv->name);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index e65d564f01cd..5eb0b22958e4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -458,6 +458,21 @@ struct device_physical_location {
>  	bool lid;
>  };
>  
> +/**
> + * enum struct_device_flags - Flags in struct device
> + *
> + * Each flag should have a set of accessor functions created via
> + * __create_dev_flag_accessors() for each access.
> + *
> + * @DEV_FLAG_READY_TO_PROBE: If set then device_add() has finished enough
> + *		initialization that probe could be called.
> + */
> +enum struct_device_flags {
> +	DEV_FLAG_READY_TO_PROBE = 0,
> +
> +	DEV_FLAG_COUNT
> +};
> +
>  /**
>   * struct device - The basic device structure
>   * @parent:	The device's "parent" device, the device to which it is attached.
> @@ -553,6 +568,7 @@ struct device_physical_location {
>   * @dma_skip_sync: DMA sync operations can be skipped for coherent buffers.
>   * @dma_iommu: Device is using default IOMMU implementation for DMA and
>   *		doesn't rely on dma_ops structure.
> + * @flags:	DEV_FLAG_XXX flags. Use atomic bitfield operations to modify.
>   *
>   * At the lowest level, every device in a Linux system is represented by an
>   * instance of struct device. The device structure contains the information
> @@ -675,8 +691,34 @@ struct device {
>  #ifdef CONFIG_IOMMU_DMA
>  	bool			dma_iommu:1;
>  #endif
> +
> +	DECLARE_BITMAP(flags, DEV_FLAG_COUNT);
>  };
>  
> +#define __create_dev_flag_accessors(accessor_name, flag_name) \
> +static inline bool dev_##accessor_name(const struct device *dev) \
> +{ \
> +	return test_bit(flag_name, dev->flags); \
> +} \
> +static inline void dev_set_##accessor_name(struct device *dev) \
> +{ \
> +	set_bit(flag_name, dev->flags); \

Atomic operations that are not RMW or that do not return a value are
unordered (see Documentation/atomic_bitops.txt). This implies that
observing the flag being set from another CPU does not guarantee that
the previous stores in program order are observed.

For that guarantee to hold, you'd need to have an
smp_mb__before_atomic() just before set_bit(), giving it release
semantics. This is equally valid for the test, clear and assign
variants.

I doubt this issue is visible on a busy system (which would be the
case at boot time), but I thought I'd mention it anyway.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Doug Anderson]

Hi,

On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
>
> > +      * blocked those attempts. Now that all of the above initialization has
> > +      * happened, unblock probe. If probe happens through another thread
> > +      * after this point but before bus_probe_device() runs then it's fine.
> > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> > +      * will notice (under device_lock) that the device is already bound.
> > +      */
> > +     dev_set_ready_to_probe(dev);
>
> I think this lacks some ordering properties that we should be allowed
> to rely on. In this case, the 'ready_to_probe' flag being set should
> that all of the data structures are observable by another CPU.
>
> Unfortunately, this doesn't seem to be the case, see below.

I agree. I think Danilo was proposing fixing this by just doing:

device_lock(dev);
dev_set_ready_to_probe(dev);
device_unlock(dev);

While that's a bit of an overkill, it also works I think. Do folks
have a preference for what they'd like to see in v5?


> > @@ -675,8 +691,34 @@ struct device {
> >  #ifdef CONFIG_IOMMU_DMA
> >       bool                    dma_iommu:1;
> >  #endif
> > +
> > +     DECLARE_BITMAP(flags, DEV_FLAG_COUNT);
> >  };
> >
> > +#define __create_dev_flag_accessors(accessor_name, flag_name) \
> > +static inline bool dev_##accessor_name(const struct device *dev) \
> > +{ \
> > +     return test_bit(flag_name, dev->flags); \
> > +} \
> > +static inline void dev_set_##accessor_name(struct device *dev) \
> > +{ \
> > +     set_bit(flag_name, dev->flags); \
>
> Atomic operations that are not RMW or that do not return a value are
> unordered (see Documentation/atomic_bitops.txt). This implies that
> observing the flag being set from another CPU does not guarantee that
> the previous stores in program order are observed.
>
> For that guarantee to hold, you'd need to have an
> smp_mb__before_atomic() just before set_bit(), giving it release
> semantics. This is equally valid for the test, clear and assign
> variants.
>
> I doubt this issue is visible on a busy system (which would be the
> case at boot time), but I thought I'd mention it anyway.

Are you suggesting I add smp memory barriers directly in all the
accessors? ...or just that clients of these functions should use
memory barriers as appropriate?

In other words, would I do:

smp_mb__before_atomic();
dev_set_ready_to_probe(dev);

...or add the barrier into all of the accessor?

My thought was to not add the barrier into the accessors since at
least one of the accessors talks about being run from a hot path
(dma_reset_need_sync()). ...but I just want to make sure.

-Doug

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Danilo Krummrich]

On Mon Apr 6, 2026 at 4:41 PM CEST, Doug Anderson wrote:
> Hi,
>
> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
>>
>> > +      * blocked those attempts. Now that all of the above initialization has
>> > +      * happened, unblock probe. If probe happens through another thread
>> > +      * after this point but before bus_probe_device() runs then it's fine.
>> > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
>> > +      * will notice (under device_lock) that the device is already bound.
>> > +      */
>> > +     dev_set_ready_to_probe(dev);
>>
>> I think this lacks some ordering properties that we should be allowed
>> to rely on. In this case, the 'ready_to_probe' flag being set should
>> that all of the data structures are observable by another CPU.
>>
>> Unfortunately, this doesn't seem to be the case, see below.
>
> I agree. I think Danilo was proposing fixing this by just doing:
>
> device_lock(dev);
> dev_set_ready_to_probe(dev);
> device_unlock(dev);
>
> While that's a bit of an overkill, it also works I think. Do folks
> have a preference for what they'd like to see in v5?

Except for the rare case where device_add() races with driver_attach(), which is
exactly the race that should be fixed by this, the device lock will be
uncontended in device_add(), so I don't consider this overkill.

>> > @@ -675,8 +691,34 @@ struct device {
>> >  #ifdef CONFIG_IOMMU_DMA
>> >       bool                    dma_iommu:1;
>> >  #endif
>> > +
>> > +     DECLARE_BITMAP(flags, DEV_FLAG_COUNT);
>> >  };
>> >
>> > +#define __create_dev_flag_accessors(accessor_name, flag_name) \
>> > +static inline bool dev_##accessor_name(const struct device *dev) \
>> > +{ \
>> > +     return test_bit(flag_name, dev->flags); \
>> > +} \
>> > +static inline void dev_set_##accessor_name(struct device *dev) \
>> > +{ \
>> > +     set_bit(flag_name, dev->flags); \
>>
>> Atomic operations that are not RMW or that do not return a value are
>> unordered (see Documentation/atomic_bitops.txt). This implies that
>> observing the flag being set from another CPU does not guarantee that
>> the previous stores in program order are observed.
>>
>> For that guarantee to hold, you'd need to have an
>> smp_mb__before_atomic() just before set_bit(), giving it release
>> semantics. This is equally valid for the test, clear and assign
>> variants.
>>
>> I doubt this issue is visible on a busy system (which would be the
>> case at boot time), but I thought I'd mention it anyway.
>
> Are you suggesting I add smp memory barriers directly in all the
> accessors? ...or just that clients of these functions should use
> memory barriers as appropriate?
>
> In other words, would I do:
>
> smp_mb__before_atomic();
> dev_set_ready_to_probe(dev);
>
> ...or add the barrier into all of the accessor?

I think this would be a bit overkill; all (other) fields are either already
protected by a lock, or are not prone to reordering races otherwise.

> My thought was to not add the barrier into the accessors since at
> least one of the accessors talks about being run from a hot path
> (dma_reset_need_sync()). ...but I just want to make sure.
>
> -Doug

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Marc Zyngier]

On Mon, 06 Apr 2026 15:41:08 +0100,
Doug Anderson <dianders@chromium.org> wrote:
> 
> Hi,
> 
> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > > +      * blocked those attempts. Now that all of the above initialization has
> > > +      * happened, unblock probe. If probe happens through another thread
> > > +      * after this point but before bus_probe_device() runs then it's fine.
> > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> > > +      * will notice (under device_lock) that the device is already bound.
> > > +      */
> > > +     dev_set_ready_to_probe(dev);
> >
> > I think this lacks some ordering properties that we should be allowed
> > to rely on. In this case, the 'ready_to_probe' flag being set should
> > that all of the data structures are observable by another CPU.
> >
> > Unfortunately, this doesn't seem to be the case, see below.
> 
> I agree. I think Danilo was proposing fixing this by just doing:
> 
> device_lock(dev);
> dev_set_ready_to_probe(dev);
> device_unlock(dev);
> 
> While that's a bit of an overkill, it also works I think. Do folks
> have a preference for what they'd like to see in v5?

It would work, but I find the construct rather obscure, and it implies
that there is a similar lock taken on the read path. Looking at the
code for a couple of minutes doesn't lead to an immediate clue that
such lock is indeed taken on all read paths.

>
> 
> > > @@ -675,8 +691,34 @@ struct device {
> > >  #ifdef CONFIG_IOMMU_DMA
> > >       bool                    dma_iommu:1;
> > >  #endif
> > > +
> > > +     DECLARE_BITMAP(flags, DEV_FLAG_COUNT);
> > >  };
> > >
> > > +#define __create_dev_flag_accessors(accessor_name, flag_name) \
> > > +static inline bool dev_##accessor_name(const struct device *dev) \
> > > +{ \
> > > +     return test_bit(flag_name, dev->flags); \
> > > +} \
> > > +static inline void dev_set_##accessor_name(struct device *dev) \
> > > +{ \
> > > +     set_bit(flag_name, dev->flags); \
> >
> > Atomic operations that are not RMW or that do not return a value are
> > unordered (see Documentation/atomic_bitops.txt). This implies that
> > observing the flag being set from another CPU does not guarantee that
> > the previous stores in program order are observed.
> >
> > For that guarantee to hold, you'd need to have an
> > smp_mb__before_atomic() just before set_bit(), giving it release
> > semantics. This is equally valid for the test, clear and assign
> > variants.
> >
> > I doubt this issue is visible on a busy system (which would be the
> > case at boot time), but I thought I'd mention it anyway.
> 
> Are you suggesting I add smp memory barriers directly in all the
> accessors? ...or just that clients of these functions should use
> memory barriers as appropriate?
> 
> In other words, would I do:
> 
> smp_mb__before_atomic();
> dev_set_ready_to_probe(dev);
> 
> ...or add the barrier into all of the accessor?
> 
> My thought was to not add the barrier into the accessors since at
> least one of the accessors talks about being run from a hot path
> (dma_reset_need_sync()). ...but I just want to make sure.

I don't think this needs to be inflicted on all flags, specially the
ones you are simply moving into the bitmap and that didn't have any
particular ordering requirements. 'ready_to_probe' is a bit different,
as it is new and tries to offer ordering semantics.

So an open-coded barrier on both sides would do the trick, unless you
go for the lock and can convince yourself that it is indeed always
acquired on all the read paths.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Danilo Krummrich]

On Mon Apr 6, 2026 at 6:34 PM CEST, Marc Zyngier wrote:
> On Mon, 06 Apr 2026 15:41:08 +0100,
> Doug Anderson <dianders@chromium.org> wrote:
>> 
>> Hi,
>> 
>> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
>> >
>> > > +      * blocked those attempts. Now that all of the above initialization has
>> > > +      * happened, unblock probe. If probe happens through another thread
>> > > +      * after this point but before bus_probe_device() runs then it's fine.
>> > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
>> > > +      * will notice (under device_lock) that the device is already bound.
>> > > +      */
>> > > +     dev_set_ready_to_probe(dev);
>> >
>> > I think this lacks some ordering properties that we should be allowed
>> > to rely on. In this case, the 'ready_to_probe' flag being set should
>> > that all of the data structures are observable by another CPU.
>> >
>> > Unfortunately, this doesn't seem to be the case, see below.
>> 
>> I agree. I think Danilo was proposing fixing this by just doing:
>> 
>> device_lock(dev);
>> dev_set_ready_to_probe(dev);
>> device_unlock(dev);
>> 
>> While that's a bit of an overkill, it also works I think. Do folks
>> have a preference for what they'd like to see in v5?
>
> It would work, but I find the construct rather obscure, and it implies
> that there is a similar lock taken on the read path. Looking at the
> code for a couple of minutes doesn't lead to an immediate clue that
> such lock is indeed taken on all read paths.

Why do you think this is obscure? As I mentioned in [1], the whole purpose of
dev_set_ready_to_probe() is to protect against a concurrent probe() attempt of
driver_attach() in __driver_probe_device(), while __driver_probe_device() is
protected by the device lock is by design.

[1] https://lore.kernel.org/driver-core/DHM5TCBT6GDE.EFG3IPRP99G7@kernel.org/

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Marc Zyngier]

On Mon, 06 Apr 2026 17:43:22 +0100,
"Danilo Krummrich" <dakr@kernel.org> wrote:
> 
> On Mon Apr 6, 2026 at 6:34 PM CEST, Marc Zyngier wrote:
> > On Mon, 06 Apr 2026 15:41:08 +0100,
> > Doug Anderson <dianders@chromium.org> wrote:
> >> 
> >> Hi,
> >> 
> >> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
> >> >
> >> > > +      * blocked those attempts. Now that all of the above initialization has
> >> > > +      * happened, unblock probe. If probe happens through another thread
> >> > > +      * after this point but before bus_probe_device() runs then it's fine.
> >> > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> >> > > +      * will notice (under device_lock) that the device is already bound.
> >> > > +      */
> >> > > +     dev_set_ready_to_probe(dev);
> >> >
> >> > I think this lacks some ordering properties that we should be allowed
> >> > to rely on. In this case, the 'ready_to_probe' flag being set should
> >> > that all of the data structures are observable by another CPU.
> >> >
> >> > Unfortunately, this doesn't seem to be the case, see below.
> >> 
> >> I agree. I think Danilo was proposing fixing this by just doing:
> >> 
> >> device_lock(dev);
> >> dev_set_ready_to_probe(dev);
> >> device_unlock(dev);
> >> 
> >> While that's a bit of an overkill, it also works I think. Do folks
> >> have a preference for what they'd like to see in v5?
> >
> > It would work, but I find the construct rather obscure, and it implies
> > that there is a similar lock taken on the read path. Looking at the
> > code for a couple of minutes doesn't lead to an immediate clue that
> > such lock is indeed taken on all read paths.
> 
> Why do you think this is obscure?

Because you're not using the lock to protect any data. You're using
the lock for its release effect. Yes, it works. But the combination of
atomics *and* locking is just odd. You normally pick one model or the
other, not a combination of both.

> As I mentioned in [1], the whole purpose of
> dev_set_ready_to_probe() is to protect against a concurrent probe() attempt of
> driver_attach() in __driver_probe_device(), while __driver_probe_device() is
> protected by the device lock is by design.
> 
> [1] https://lore.kernel.org/driver-core/DHM5TCBT6GDE.EFG3IPRP99G7@kernel.org/

I don't have much skin in this game, and you seem to have strong
opinions about how these things are supposed to work. So whatever
floats your boat, as long as it is correct.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Danilo Krummrich]

On Mon Apr 6, 2026 at 7:06 PM CEST, Marc Zyngier wrote:
> On Mon, 06 Apr 2026 17:43:22 +0100,
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>> 
>> On Mon Apr 6, 2026 at 6:34 PM CEST, Marc Zyngier wrote:
>> > On Mon, 06 Apr 2026 15:41:08 +0100,
>> > Doug Anderson <dianders@chromium.org> wrote:
>> >> 
>> >> Hi,
>> >> 
>> >> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
>> >> >
>> >> > > +      * blocked those attempts. Now that all of the above initialization has
>> >> > > +      * happened, unblock probe. If probe happens through another thread
>> >> > > +      * after this point but before bus_probe_device() runs then it's fine.
>> >> > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
>> >> > > +      * will notice (under device_lock) that the device is already bound.
>> >> > > +      */
>> >> > > +     dev_set_ready_to_probe(dev);
>> >> >
>> >> > I think this lacks some ordering properties that we should be allowed
>> >> > to rely on. In this case, the 'ready_to_probe' flag being set should
>> >> > that all of the data structures are observable by another CPU.
>> >> >
>> >> > Unfortunately, this doesn't seem to be the case, see below.
>> >> 
>> >> I agree. I think Danilo was proposing fixing this by just doing:
>> >> 
>> >> device_lock(dev);
>> >> dev_set_ready_to_probe(dev);
>> >> device_unlock(dev);
>> >> 
>> >> While that's a bit of an overkill, it also works I think. Do folks
>> >> have a preference for what they'd like to see in v5?
>> >
>> > It would work, but I find the construct rather obscure, and it implies
>> > that there is a similar lock taken on the read path. Looking at the
>> > code for a couple of minutes doesn't lead to an immediate clue that
>> > such lock is indeed taken on all read paths.
>> 
>> Why do you think this is obscure?
>
> Because you're not using the lock to protect any data. You're using
> the lock for its release effect. Yes, it works. But the combination of
> atomics *and* locking is just odd. You normally pick one model or the
> other, not a combination of both.

Yeah, the choice of bitops was purely because previously (in v2) this was a C
bitfield member in struct device protected with the device lock. But, not all of
the bitfield members were protected by the same lock or protected by a lock at
all, which would have made this racy with the other bitfield members. I.e. the
choice of bitops was independent; see also [2] for context.

[2] https://lore.kernel.org/driver-core/DHH1PD0ASG8H.1K3KG9L658DYN@kernel.org/

>> As I mentioned in [1], the whole purpose of
>> dev_set_ready_to_probe() is to protect against a concurrent probe() attempt of
>> driver_attach() in __driver_probe_device(), while __driver_probe_device() is
>> protected by the device lock is by design.
>> 
>> [1] https://lore.kernel.org/driver-core/DHM5TCBT6GDE.EFG3IPRP99G7@kernel.org/
>
> I don't have much skin in this game, and you seem to have strong
> opinions about how these things are supposed to work. So whatever
> floats your boat, as long as it is correct.

Not overly, it's more about calling out the fact that probe() paths are
serialized through the device lock by design, so it seems natural to protect
dev_set_ready_to_probe() with the device lock.

The fact that dev_set_ready_to_probe() uses a bitop under the hood is an
implementation detail, i.e. it could also be an independent boolean.

That said, as I caught the issue in [3], I also mentioned the option of an
explicit memory barrier in device_add() and __driver_probe_device(). I.e. I'm
not entirely against it, but I think the device lock is a bit cleaner.

[3] https://lore.kernel.org/driver-core/DHLITCTY913U.J59JSQOVL0NH@kernel.org/

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Doug Anderson]

Hi,

On Mon, Apr 6, 2026 at 11:11 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Apr 6, 2026 at 7:06 PM CEST, Marc Zyngier wrote:
> > On Mon, 06 Apr 2026 17:43:22 +0100,
> > "Danilo Krummrich" <dakr@kernel.org> wrote:
> >>
> >> On Mon Apr 6, 2026 at 6:34 PM CEST, Marc Zyngier wrote:
> >> > On Mon, 06 Apr 2026 15:41:08 +0100,
> >> > Doug Anderson <dianders@chromium.org> wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
> >> >> >
> >> >> > > +      * blocked those attempts. Now that all of the above initialization has
> >> >> > > +      * happened, unblock probe. If probe happens through another thread
> >> >> > > +      * after this point but before bus_probe_device() runs then it's fine.
> >> >> > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> >> >> > > +      * will notice (under device_lock) that the device is already bound.
> >> >> > > +      */
> >> >> > > +     dev_set_ready_to_probe(dev);
> >> >> >
> >> >> > I think this lacks some ordering properties that we should be allowed
> >> >> > to rely on. In this case, the 'ready_to_probe' flag being set should
> >> >> > that all of the data structures are observable by another CPU.
> >> >> >
> >> >> > Unfortunately, this doesn't seem to be the case, see below.
> >> >>
> >> >> I agree. I think Danilo was proposing fixing this by just doing:
> >> >>
> >> >> device_lock(dev);
> >> >> dev_set_ready_to_probe(dev);
> >> >> device_unlock(dev);
> >> >>
> >> >> While that's a bit of an overkill, it also works I think. Do folks
> >> >> have a preference for what they'd like to see in v5?
> >> >
> >> > It would work, but I find the construct rather obscure, and it implies
> >> > that there is a similar lock taken on the read path. Looking at the
> >> > code for a couple of minutes doesn't lead to an immediate clue that
> >> > such lock is indeed taken on all read paths.
> >>
> >> Why do you think this is obscure?
> >
> > Because you're not using the lock to protect any data. You're using
> > the lock for its release effect. Yes, it works. But the combination of
> > atomics *and* locking is just odd. You normally pick one model or the
> > other, not a combination of both.
>
> Yeah, the choice of bitops was purely because previously (in v2) this was a C
> bitfield member in struct device protected with the device lock. But, not all of
> the bitfield members were protected by the same lock or protected by a lock at
> all, which would have made this racy with the other bitfield members. I.e. the
> choice of bitops was independent; see also [2] for context.
>
> [2] https://lore.kernel.org/driver-core/DHH1PD0ASG8H.1K3KG9L658DYN@kernel.org/

I've changed the snippet in the commit description to now justify the
use of bitops like this:

Instead of adding another flag to the bitfields already in "struct
device", instead add a new "flags" field and use that. This allows us
to freely change the bit from different thread without worrying about
corrupting nearby bits (and means threads changing other bit won't
corrupt us).


> >> As I mentioned in [1], the whole purpose of
> >> dev_set_ready_to_probe() is to protect against a concurrent probe() attempt of
> >> driver_attach() in __driver_probe_device(), while __driver_probe_device() is
> >> protected by the device lock is by design.
> >>
> >> [1] https://lore.kernel.org/driver-core/DHM5TCBT6GDE.EFG3IPRP99G7@kernel.org/
> >
> > I don't have much skin in this game, and you seem to have strong
> > opinions about how these things are supposed to work. So whatever
> > floats your boat, as long as it is correct.
>
> Not overly, it's more about calling out the fact that probe() paths are
> serialized through the device lock by design, so it seems natural to protect
> dev_set_ready_to_probe() with the device lock.
>
> The fact that dev_set_ready_to_probe() uses a bitop under the hood is an
> implementation detail, i.e. it could also be an independent boolean.
>
> That said, as I caught the issue in [3], I also mentioned the option of an
> explicit memory barrier in device_add() and __driver_probe_device(). I.e. I'm
> not entirely against it, but I think the device lock is a bit cleaner.
>
> [3] https://lore.kernel.org/driver-core/DHLITCTY913U.J59JSQOVL0NH@kernel.org/

I've got the series all prepped and it sounds as if the alignment is
on using device_lock(). I'll give it a few more hours in case there
are additional responses, then send a v5. ;-)

-Doug

Re: [PATCH v4 1/9] driver core: Don't let a device probe until it's ready

[Doug Anderson]

Hi,

On Mon, Apr 6, 2026 at 9:35 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 06 Apr 2026 15:41:08 +0100,
> Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > > +      * blocked those attempts. Now that all of the above initialization has
> > > > +      * happened, unblock probe. If probe happens through another thread
> > > > +      * after this point but before bus_probe_device() runs then it's fine.
> > > > +      * bus_probe_device() -> device_initial_probe() -> __device_attach()
> > > > +      * will notice (under device_lock) that the device is already bound.
> > > > +      */
> > > > +     dev_set_ready_to_probe(dev);
> > >
> > > I think this lacks some ordering properties that we should be allowed
> > > to rely on. In this case, the 'ready_to_probe' flag being set should
> > > that all of the data structures are observable by another CPU.
> > >
> > > Unfortunately, this doesn't seem to be the case, see below.
> >
> > I agree. I think Danilo was proposing fixing this by just doing:
> >
> > device_lock(dev);
> > dev_set_ready_to_probe(dev);
> > device_unlock(dev);
> >
> > While that's a bit of an overkill, it also works I think. Do folks
> > have a preference for what they'd like to see in v5?
>
> It would work, but I find the construct rather obscure, and it implies
> that there is a similar lock taken on the read path. Looking at the
> code for a couple of minutes doesn't lead to an immediate clue that
> such lock is indeed taken on all read paths.

Yeah, it's definitely taken on all read paths. It is only accessed in
__driver_probe_device(). __driver_probe_device() is called in two
places.

1. From device_driver_attach(), it's called with
"__device_driver_lock()" held, which includes the device lock.

2. From driver_probe_device().

...then we look at driver_probe_device(). That's called from three places.

1. From __driver_attach_async_helper, it's called with
"__device_driver_lock()" held, which includes the device lock.

2. From __driver_attach(), it's called with "__device_driver_lock()"
held, which includes the device lock.

3. From __device_attach_driver()

...then we look at __device_attach_driver(). That's called from two places:

1. From __device_attach_async_helper(), which holds the device lock.

2. From __device_attach(), which holds the device lock.

...assuming I didn't mess up, that covers them all.


> > Are you suggesting I add smp memory barriers directly in all the
> > accessors? ...or just that clients of these functions should use
> > memory barriers as appropriate?
> >
> > In other words, would I do:
> >
> > smp_mb__before_atomic();
> > dev_set_ready_to_probe(dev);
> >
> > ...or add the barrier into all of the accessor?
> >
> > My thought was to not add the barrier into the accessors since at
> > least one of the accessors talks about being run from a hot path
> > (dma_reset_need_sync()). ...but I just want to make sure.
>
> I don't think this needs to be inflicted on all flags, specially the
> ones you are simply moving into the bitmap and that didn't have any
> particular ordering requirements. 'ready_to_probe' is a bit different,
> as it is new and tries to offer ordering semantics.

OK, cool. Just wanted to make sure I was understanding!


> So an open-coded barrier on both sides would do the trick, unless you
> go for the lock and can convince yourself that it is indeed always
> acquired on all the read paths.

I'm pretty convinced it's acquired on all read paths. :-)

-Doug