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@xxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> >> 
> >> Hi,
> >> 
> >> On Sun, Apr 5, 2026 at 11:32 PM Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx/

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.