On Fri, Oct 14, 2011 at 11:33 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
