On Fri, 11 Dec 2009, Rafael J. Wysocki wrote: > > But fine, say we use the approach based on rwsems and consider suspend and > the inner lock. We acquire it using down_write(), because we want to wait for > multiple other dirvers. Now, in fact we could do literally > > down_write(dev->power.rwsem); > up_write(dev->power.rwsem); > > because the lock doesn't really protect anything from anyone. What it does is > to prevent _us_ from doing something too early. To me, personally, it's not a > usual use of locks. I agree that it's fairly unusual, but on the other hand, it's unusual only because you contrieved it to be. If you instead do down_write(dev->power.rwsem); .. do the actual suspend .. up_write(dev->power.rwsem); it doesn't look odd any more, does it? And while you don't _need_ to hold the power lock over the suspend call, it actually does make sense, and gives you some nicer guarantees. For an example of the kinds of guarantees it would give you - I think that you might actually be able to do a partial suspend and then a resume without any other locks, and you'd know that just the per-device locking would already guarantee that no device is ever tried to resume before it has finished its asynchronous suspend. Think about it. In the completion model, the "async_synchronize_full()" will synchronize all async work, and as a result you think that you don't need that level of robustness from the locking itself. But think about it this way: if you could abort a failed suspend, and start resuming devices immediately, without doing that "async_synchronize_full()" in between - simply because you know that the node locking itself will just "do the right thing". To me, that's a sign of a _good_ design. Using a rwsem is simply just more robust and natural for the problem in question. Exactly because it's a real lock. > > Don't try to make up problems. The _only_ subsystem we know wants this is > > USB, and we know USB is purely a tree. > > Not really. > > I've already said it once, but let me repeat. Some device objects have those > ACPI "shadow" device objects that represent the ACPI view of given "physical" > device and have their own suspend and resume routines. It turns out that > these ACPI "shadow" devices have to be suspended after their "physical" > counterparts and resumed before them, or else things beak really badly. > I don't know the reason for that, I only verified it experimentally (I also > don't like that design, but I didn't invent it and I have to live with it at > least for now). So if we don't enforce these constraints doing async > suspend and resume, we won't be able to handle _any_ devices with those > ACPI "shadow" things asynchronously. Ever. [That includes the majority > PCI devices, at least the "planar" ones (which is unfortunate, but that's how > it goes).] So? First off, you're wrong. It's not "ever". I'm happy to add complexity later, I just don't want to start out with a complex model. Adding complexity too early "just because we migth need it" is the wrong thing to do. Secondly, I repeat: we don't want to do those PCI devices asynchronously anyway. You're again digging yourself deeper by just continually bringing up this total non-issue. I realize you did it for testing, but I'm serious when I say that we should limit these things as much as possible, rather than see it as an opportunity to do crazy things. Solve the problem at hand _first_. Solve it as simply as you can. And hope that you never ever will need anything more complex. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
