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@xxxxxxxxxx> 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