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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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