[PATCH 2/5] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Fri, Oct 14, 2011 at 11:33 AM, Alan Stern <stern at rowland.harvard.edu> wrote:
> On Fri, 14 Oct 2011, Grant Likely wrote:
>
>> > I don't think the second part needs to be quite so invasive.
>> > Certainly drivers that never defer probes shouldn't require anything to
>> > be moved.
>>
>> Think about that a minute.  Consider a dpm_list of devices:
>>         abcDefGh
>>
>> Now assume that D has an implicit dependency on G.  If D gets probed
>> first, then it will be deferred until G gets probed and then D will
>> get retried and moved to the end of the list resulting in:
>>         abcefGhD
>> Everything is good now for the order that things need to be suspended in.
>>
>> Now lets assume that the drivers get linked into the kernel in a
>> different order (or the modules get loaded in a different order) and G
>> gets probed first, followed by D.  No deferral occurred and so no
>> reordering occurs, resulting in:
>>         abcDefGh (unchanged)
>> But now suspend is broken because D depends on G, but G will be
>> suspended before D.
>
> However D sometimes does defer probes.  Therefore the device always
> needs to be moved, even though this particular probe wasn't deferred.

Yes, that's part of my point.

>>  This looks and smells like a bug to me.  In fact,
>> even without using probe deferral it looks like a bug because the
>> dap_list isn't taking into account the fact that there are very likely
>> to be implicit dependencies between devices that are not represented
>> in the device hierarchy (maybe not common in PCs, but certainly is the
>> case for embedded).  But, it is also easy to solve by ensuring the
>> dap_list is also probe-order sorted.
>>
>> > A deferred probe _should_ move the device to the end of the list.  But
>> > this needs to happen in the probe routine itself (after it has verified
>> > that all the other required devices are present and before it has
>> > registered any children) to prevent races.  It can't be done in a
>> > central location.
>>
>> I'm really concerned about drivers having to implement this and not
>> getting it correct; particularly when moving a device to the end of
>> the list is cheap, and it shouldn't be a problem to do the move
>> unconditionally after a driver has been matches, but before probe() is
>> called.
>
> But that's too early.  What if D gets moved to the end of the list,
> then G gets added to the list and probed, and then D's probe succeeds?

So the argument is that if really_probe() called dpm_move_last()
immediately before the dev->bus->probe()/drv->probe() call then there
is a race condition that G could get both registered and probed before
D's probe() starts using G's resources.  And so, the list would still
have G after D which is in the wrong order.  Do I understand
correctly?

> And after the probe returns is too late, because by then there may well
> be child devices.

Agreed, after probe returns is definitely too late.

>>  We may be able to keep watch to make sure that drivers using
>> probe deferral are also reordering themselves, but that does nothing
>> for the cases described above where the link order of the drivers
>> determines probe order, not the dap_list order.
>
> Devices need to be moved whenever they have any external dependencies,
> regardless of the particular order they get probed in.

I suspect this gets messy in a hurry, but perhaps it is worth trying.
So, any device that makes use of a PHY, GPIO line, codec, etc will
need to call dpm_move_last() before adding child devices, correct?
I'd be much happier to find a way to do this in core code though.  And
there is still a potential race condition here.  For example, if G is
in the middle of it's probe routine, and D gets probed between G
registering GPIOs and calling dpm_move_last(), then we're in the same
boat again.  I think the window for this race can be considered to be
of the same magnitude as the moved to early race described above.  I
need to think about this more...

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.