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

Doug Anderson
2026-04-05T22:39:44Z

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