Re: Async suspend-resume patch w/ completions (was: Re: Async suspend-resume patch w/ rwsems)

Linus Torvalds
2009-12-11T22:31:17Z

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