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

[Doug Anderson]

Hi,

On Mon, Apr 6, 2026 at 9:35 AM Marc Zyngier <maz@xxxxxxxxxx> 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.

Yeah, it's definitely taken on all read paths. It is only accessed in
__driver_probe_device(). __driver_probe_device() is called in two
places.

1. From device_driver_attach(), it's called with
"__device_driver_lock()" held, which includes the device lock.

2. From driver_probe_device().

...then we look at driver_probe_device(). That's called from three places.

1. From __driver_attach_async_helper, it's called with
"__device_driver_lock()" held, which includes the device lock.

2. From __driver_attach(), it's called with "__device_driver_lock()"
held, which includes the device lock.

3. From __device_attach_driver()

...then we look at __device_attach_driver(). That's called from two places:

1. From __device_attach_async_helper(), which holds the device lock.

2. From __device_attach(), which holds the device lock.

...assuming I didn't mess up, that covers them all.


> > Are you suggesting I add smp memory barriers directly in all the
> > accessors? ...or just that clients of these functions should use
> > memory barriers as appropriate?
> >
> > In other words, would I do:
> >
> > smp_mb__before_atomic();
> > dev_set_ready_to_probe(dev);
> >
> > ...or add the barrier into all of the accessor?
> >
> > My thought was to not add the barrier into the accessors since at
> > least one of the accessors talks about being run from a hot path
> > (dma_reset_need_sync()). ...but I just want to make sure.
>
> I don't think this needs to be inflicted on all flags, specially the
> ones you are simply moving into the bitmap and that didn't have any
> particular ordering requirements. 'ready_to_probe' is a bit different,
> as it is new and tries to offer ordering semantics.

OK, cool. Just wanted to make sure I was understanding!


> So an open-coded barrier on both sides would do the trick, unless you
> go for the lock and can convince yourself that it is indeed always
> acquired on all the read paths.

I'm pretty convinced it's acquired on all read paths. :-)

-Doug