From: Alan Stern on
On Tue, 8 Dec 2009, Rafael J. Wysocki wrote:

> > Well, one difficulty. It arises only because we are contemplating
> > having the PM core fire up the async tasks, rather than having the
> > drivers' suspend routines launch them (the way your original proposal
> > did -- the difficulty does not arise there).
> >
> > Suppose A and B are unrelated devices and we need to impose the
> > off-tree constraint that A suspends after B. With children taking
> > their parent's lock, the way to prevent A from suspending too soon is
> > by having B's suspend routine acquire A's lock.
> >
> > But B's suspend routine runs entirely in an async task, because that
> > task is started by the PM core and it does the method call. Hence by
> > the time B's suspend routine is called, A may already have begun
> > suspending -- it's too late to take A's lock. To make the locking
> > work, B would have to acquire A's lock _before_ B's async task starts.
> > Since the PM core is unaware of the off-tree dependency, there's no
> > simple way to make it work.
>
> Do not set async_suspend for B and instead start your own async thread
> from its suspend callback. The parent-children synchronization is done by the
> core anyway (at least I'd do it that way), so the only thing you need to worry
> about is the extra dependency.

I don't like that because it introduces "artificial" dependencies: It
makes B depend on all the preceding synchronous suspends, even totally
unrelated ones. But yes, it would work.

> I would be slightly more comfortable using completions, but the rwsem-based
> approach is fine with me as well.

On the principle of making things as easy and foolproof as possible for
driver authors, I also favor completions since it makes dealing with
non-tree dependencies easier.

However either way would be okay. I do have to handle some non-tree
dependencies in USB, but oddly enough they affect only resume, not
suspend. So this "who starts the async task" issue doesn't apply.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Tue, 8 Dec 2009, Linus Torvalds wrote:

> It's just that I think the "looping over children" is ugly, when I think
> that by doing it the other way around you can make the code simpler and
> only depend on the PM device list and a simple parent pointer access.

I agree that it is uglier. The only advantage is in handling
asynchronous non-tree suspend dependencies, of which we probably won't
have very many. In fact, I don't know of _any_ offhand.

Interestingly, this non-tree dependency problem does not affect resume.

> I also think that you are wrong that the above somehow protects against
> non-topological dependencies. If the device you want to keep delay
> yourself suspending for is after you in the list, the down_read() on that
> may succeed simply because it hasn't even done its down_write() yet and
> you got scheduled early.

You mean, if A comes before B in the list and A must suspend after B?
Then A's down_read() on B _can't_ occur before B's down_write() on
itself. The down_write() on B happens before the
list_for_each_entry_reverse() iteration reaches A; it even happens
before B's async task is launched.

> But I guess you could do that by walking the list twice (first to lock
> them all, then to actually call the suspend function). That whole
> two-phase thing, except the first phase _only_ locks, and doesn't do any
> callbacks.

Not necessary.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on


On Tue, 8 Dec 2009, Alan Stern wrote:
>
> You mean, if A comes before B in the list and A must suspend after B?

But if they are not topologically ordered, then A wouldn't necessarily be
before B on the list in the first place.

Of course, if we've mucked with the list by hand and made sure the
ordering is ok, then that's a different issue. But your whole point seemed
to be that the device could impose its own ordering in its suspend
callback, which is not true on its own without external ordering.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on
On Tue, Dec 08, 2009 at 09:35:59PM -0500, Alan Stern wrote:
> On Tue, 8 Dec 2009, Linus Torvalds wrote:

> > It's just that I think the "looping over children" is ugly, when I think
> > that by doing it the other way around you can make the code simpler and
> > only depend on the PM device list and a simple parent pointer access.

> I agree that it is uglier. The only advantage is in handling
> asynchronous non-tree suspend dependencies, of which we probably won't
> have very many. In fact, I don't know of _any_ offhand.

There's some potential for this in embedded audio - it wants to bring
down the entire embedded audio subsystem at once before the individual
devices (and their parents) get suspended since bringing them down out
of sync can result in audible artifacts. Depending on the system the
suspend may take a noticable amount of time so it'd be nice to be able
to run it asynchronously, though we don't currently do so.

At the minute we get away with this mostly through not being able to
represent the cases that are likely to actually trip up over it.

> Interestingly, this non-tree dependency problem does not affect resume.

Embedded audio does potentially - the resume needs all the individual
devices in the subsystem and can take a substantial proportion of the
overall resume time. Currently we get away with a combination of
assuming that all the drivers are live when we decide to start resuming
them and using the ALSA userspace API to deal with bringing the resume
out of line, but it's not ideal.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Tue, 8 Dec 2009, Linus Torvalds wrote:

> On Tue, 8 Dec 2009, Alan Stern wrote:
> >
> > You mean, if A comes before B in the list and A must suspend after B?
>
> But if they are not topologically ordered, then A wouldn't necessarily be
> before B on the list in the first place.

Okay, I see what you're getting at. Yes, this is quite true -- if A
doesn't precede B in dpm_list then A can't safely wait for B to
suspend. To put it another way, only list-compatible constraints are
feasible.

This shouldn't be a problem. If it were we'd be seeing it right now,
because A would _always_ suspend before B.

> Of course, if we've mucked with the list by hand and made sure the
> ordering is ok, then that's a different issue. But your whole point seemed
> to be that the device could impose its own ordering in its suspend
> callback, which is not true on its own without external ordering.

No, sorry for not making it clearer. I was assuming all long that the
non-tree constraints were compatible with the list ordering.

In fact these considerations already affect the USB resume operations,
even without asynchronous resume. The code relies on the fact that the
PCI layer registers sibling devices on a slot in order of increasing
function number. There's no guarantee this will remain true in the
future (it may already be wrong AFAIK), so putting in some explicit
list manipulation is the prudent thing to do.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/