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

[Marc Zyngier <maz@kernel.org>]

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.