Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[David Brownell]

This is a repeat of an issue I posted before, but
which for some reason I never saw email back ...

basically, I think the notion of counting wakeup
events seems dubious on common hardware, so the
focus might perhaps better be placed on ensuring
userspace just receives events rather than
trying to track events which in one context ended
up being wakeup events.  (That's simpler, and the
system is by definition awake if it can handle any
events at all.)

Thing is, "wakeup" is, for e.g. most ARMs, just a hardware attribute of what's otherwise a routine
event, which happens in other contexts and needs
to be handled consistently .... nothing special
about having woken the system too, the result ought to
be the same regardless (from the user P.O.V.) ...  (Common Examples include SoC peripheral IRQs that can wake the system (including GPIO and other types
of external IRQ signal.)

BRIEFLY:  if that event doesn't arrive reliably,
it's an issue regardless of wakeup:  either TX from
kernel, or RX in userspace. Such bugs would need to
be fixed.  Having them fixed will help the wakeup scenarios too of course.

(The raciness issues might boil down to something as simple as not letting userspace know about transition events to/from suspend states, but that issue
ought to be cleanly separable; ISTR other messages on the suspend blocker threads have shown how to work with such clean factoring.)



So trying to track whether a given event is what
woke the system will often be implausible, since
several such events might each have fired (one
or more concurrent wakeup sources, even ... it
could be indeterminate which one[s] happened.)
And the event could fire without being a wakeup.

Yes, there are a few cases (like USB remote wakeup
signaling and some PCI mechanisms, plus a few BIOS
assisted situations) where certain events may be
identifiable as wakeup sources, perhaps runtime not
system-wide).  But the common case just includes
an event, not the ability to know that event had
the "woke whole system from low power state"
side effect too.




> +        The
> /sys/power/wakeup_count file allows user space to avoid
> +        losing wakeup events
> when transitioning the system into a sleep
> +        state.  Reading
> from it returns the current number of registered
> +        wakeup events and it
> blocks if some wakeup events are being
> +        processed at the
> time the file is read from. 

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Alan Stern]

On Sat, 26 Jun 2010, David Brownell wrote:

> This is a repeat of an issue I posted before, but
> which for some reason I never saw email back ...

I replied to one of your emails on this topic.  Maybe you didn't see 
the reply.

> basically, I think the notion of counting wakeup
> events seems dubious on common hardware, so the
> focus might perhaps better be placed on ensuring
> userspace just receives events rather than
> trying to track events which in one context ended
> up being wakeup events.  (That's simpler, and the
> system is by definition awake if it can handle any
> events at all.)

I disagree.  There's no question that userspace will eventually receive 
these events.  The question is whether they will be received in a 
timely manner.

The "counting" isn't meant as a way of keeping track of the absolute
number of these events.  It's more like a technique for seeing how many
remain outstanding at any time.

> Thing is, "wakeup" is, for e.g. most ARMs, just a hardware attribute of what's otherwise a routine
> event, which happens in other contexts and needs
> to be handled consistently .... nothing special
> about having woken the system too, the result ought to
> be the same regardless (from the user P.O.V.) ...  (Common Examples include SoC peripheral IRQs that can wake the system (including GPIO and other types
> of external IRQ signal.)

There's nothing wrong with that.  All these routine events may qualify
as "wakeup" events.  The real deciding factor is whether the event
should cause tasks to be unfrozen (if they have already been frozen by
the PM core).  If yes then it is a "wakeup" event; otherwise it isn't.

> BRIEFLY:  if that event doesn't arrive reliably,
> it's an issue regardless of wakeup:  either TX from
> kernel, or RX in userspace. Such bugs would need to
> be fixed.  Having them fixed will help the wakeup scenarios too of course.

That is irrelevant to the topic at hand.  We know that these events 
arrive; what we don't know is that they will properly block tasks 
from being frozen (or will unfreeze them if they are already frozen).

> (The raciness issues might boil down to something as simple as not letting userspace know about transition events to/from suspend states, but that issue
> ought to be cleanly separable; ISTR other messages on the suspend blocker threads have shown how to work with such clean factoring.)
> 
> 
> 
> So trying to track whether a given event is what
> woke the system will often be implausible, since
> several such events might each have fired (one
> or more concurrent wakeup sources, even ... it
> could be indeterminate which one[s] happened.)
> And the event could fire without being a wakeup.

It doesn't matter which event(s) woke the system, and Rafael's patch 
doesn't try to track this.

> Yes, there are a few cases (like USB remote wakeup
> signaling and some PCI mechanisms, plus a few BIOS
> assisted situations) where certain events may be
> identifiable as wakeup sources, perhaps runtime not
> system-wide).  But the common case just includes
> an event, not the ability to know that event had
> the "woke whole system from low power state"
> side effect too.

> > +        The
> > /sys/power/wakeup_count file allows user space to avoid
> > +        losing wakeup events
> > when transitioning the system into a sleep
> > +        state.  Reading

This could have been phrased better:  The /sys/power/wakeup_count file
allows user space to put the system into a sleep state conditionally
subject to the arrival of concurrent wakeup events, which will either
block the sleep transition or cause it to fail.

Alan Stern

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Rafael J. Wysocki]

On Saturday, June 26, 2010, Alan Stern wrote:
> On Sat, 26 Jun 2010, David Brownell wrote:
... 
> > > +        The
> > > /sys/power/wakeup_count file allows user space to avoid
> > > +        losing wakeup events
> > > when transitioning the system into a sleep
> > > +        state.  Reading
> 
> This could have been phrased better:  The /sys/power/wakeup_count file
> allows user space to put the system into a sleep state conditionally
> subject to the arrival of concurrent wakeup events, which will either
> block the sleep transition or cause it to fail.

I'll chage this in the final version.  Thanks!

Rafael

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Alan Stern]

On Sat, 26 Jun 2010, Rafael J. Wysocki wrote:

> On Saturday, June 26, 2010, Alan Stern wrote:
> > On Sat, 26 Jun 2010, David Brownell wrote:
> ... 
> > > > +        The
> > > > /sys/power/wakeup_count file allows user space to avoid
> > > > +        losing wakeup events
> > > > when transitioning the system into a sleep
> > > > +        state.  Reading
> > 
> > This could have been phrased better:  The /sys/power/wakeup_count file
> > allows user space to put the system into a sleep state conditionally
> > subject to the arrival of concurrent wakeup events, which will either
> > block the sleep transition or cause it to fail.
> 
> I'll chage this in the final version.  Thanks!

Even this should be simplified, if possible.  Maybe something like: ...  
put the system into a sleep state while taking into account the
concurrent arrival of wakeup events ...

I tried to put the word "reliably" in there, but it didn't seem to fit.  
After all, the current /sys/power/state is already very reliable about 
putting the system to sleep!

Alan Stern

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[David Brownell]

> > This is a repeat of an issue I posted before, but
> > which for some reason I never saw email back ...
> 
> I replied to one of your emails on this topic.  Maybe you didn't see the reply.

I guess not; I'll see if there's some message
buried in a too-much-email folder.)
> 
> > basically, I think the notion of counting wakeup
> > events seems dubious on common hardware, ...
> I disagree. 
> 
> The "counting" isn't meant as a way of keeping track of the absolute
> number of these events.  It's more like a technique  for seeing how many
> remain outstanding at any time.

But if you can't count them with any
reliability, you can't know *that* either... so
there must be a a problem with that model.

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Alan Stern]

On Sat, 26 Jun 2010, David Brownell wrote:

> > > basically, I think the notion of counting wakeup
> > > events seems dubious on common hardware, ...
> > I disagree. 
> > 
> > The "counting" isn't meant as a way of keeping track of the absolute
> > number of these events.  It's more like a technique  for seeing how many
> > remain outstanding at any time.
> 
> But if you can't count them with any
> reliability, you can't know *that* either... so
> there must be a a problem with that model.

Why do you say we can't count them with any reliability?

Or, let's put it another way: You'll grant that we can count _some_ set
of events.  Given that, you'll probably also grant that we can keep
track of their number reliably enough to know when the count has
dropped to 0.  Then this becomes a question of how closely does the set
of events we can count match up with the set of "wakeup" events?

In fact, it doesn't have to match up perfectly.  There may be a few
wakeup events where we don't really care if one of them occurs while
the system is going to sleep and the sleep isn't delayed or aborted.  
(Although by definition this is never _supposed_ to happen, there may
be cases where we just don't care.)  The other possibility is
relatively harmless too: an event that wouldn't wake up a sleeping
system nevertheless can delay or abort a suspend-in-progress.

So overall I don't see a problem with this.  Do you have any especially
pernicious failure modes in mind?

Alan Stern

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Rafael J. Wysocki]

On Saturday, June 26, 2010, David Brownell wrote:
> This is a repeat of an issue I posted before, but
> which for some reason I never saw email back ...

You've just got a very good reply from Alan.  I agree with what he says.

Rafael

Re: [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Rafael J. Wysocki]

On Sunday, June 27, 2010, Alan Stern wrote:
> On Sat, 26 Jun 2010, Rafael J. Wysocki wrote:
> 
> > +void pm_relax(void)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&events_lock, flags);
> > +	if (events_in_progress) {
> > +		event_count++;
> > +		if (!--events_in_progress)
> > +			wake_up_all(&events_wait_queue);
> > +	}
> > +	spin_unlock_irqrestore(&events_lock, flags);
> > +}
> 
> > +bool pm_get_wakeup_count(unsigned long *count)
> > +{
> > +	bool ret;
> > +
> > +	spin_lock_irq(&events_lock);
> > +	if (capable(CAP_SYS_ADMIN))
> > +		events_check_enabled = false;
> > +
> > +	if (events_in_progress) {
> > +		DEFINE_WAIT(wait);
> > +
> > +		do {
> > +			prepare_to_wait(&events_wait_queue, &wait,
> > +					TASK_INTERRUPTIBLE);
> > +			if (!events_in_progress)
> > +				break;
> > +			spin_unlock_irq(&events_lock);
> > +
> > +			schedule();
> > +
> > +			spin_lock_irq(&events_lock);
> > +		} while (!signal_pending(current));
> > +		finish_wait(&events_wait_queue, &wait);
> > +	}
> > +	*count = event_count;
> > +	ret = !events_in_progress;
> > +	spin_unlock_irq(&events_lock);
> > +	return ret;
> > +}
> 
> Here's a thought.  Presumably pm_relax() will end up getting called a 
> lot more often than pm_get_wakeup_count().  Instead of using a wait 
> queue, you could make pm_get_wakeup_count() poll at 100-ms intervals.  
> The total overhead would be smaller.

For that I'd need a separate kernel thread or a work item that would reschedule
itself periodically, because pm_get_wakeup_count() is only called via
/sys/power/wakeup_count.  It would complicate things quite a bit which I'm not
sure is worth it at this point.

> Here's another thought.  If event_count and events_in_progress were 
> atomic_t then the new spinlock wouldn't be needed at all.  (But you 
> would need an appropriate pair of memory barriers, to guarantee that 
> when a writer decrements events_in_progress to 0 and increments 
> event_count, a reader won't see events_in_progress == 0 without also 
> seeing the incremented event_count.)  Overall, this may not be a
> significant improvement.

No, I don't think it would be significant.  Still, we can go back to this
if the spinlock turns out to be a problem in future.

Rafael

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[David Brownell]

Did someone post the canonical driver changes
to make use of this?  Something like

  suspend() { /* if wake-enabled, up count */ }
  resume() { /* if upcounted, downcount */ }

is what first comes to mind.. expecting that
the suspend/resume methods in the driver are
already doing the right things for enabling
and later disabling the "system wake" behavior
on the various relevant hardware events...

Re: [linux-pm] [PATCH] PM: Make it possible to avoid wakeup events from being lost

[Alan Stern]

On Mon, 28 Jun 2010, David Brownell wrote:

> Did someone post the canonical driver changes
> to make use of this?

No, not really.  The patch itself contains an example (PCI) but it 
doesn't demonstrate the full range of possible usages.

>  Something like
> 
>   suspend() { /* if wake-enabled, up count */ }
>   resume() { /* if upcounted, downcount */ }
> 
> is what first comes to mind.. expecting that
> the suspend/resume methods in the driver are
> already doing the right things for enabling
> and later disabling the "system wake" behavior
> on the various relevant hardware events...

The PCI example looks like this:

	resume()
	{
		...
		if (device_may_wakeup(dev))
			pm_wakeup_event(dev, TIMEOUT_GUESS);
		...
	}

where TIMEOUT_GUESS is an estimate of how long to wait before allowing 
the system to sleep.

For things like keyboards, an example would go more like this:

	irq_handler()
	{
		...
		if (key-press event occurred) {
			...
			if (input queue is empty)
				pm_stay_awake(dev);
			add event to input queue;
			...
		}
		...
	}

	read_queue()
	{
		...
		send queued data to userspace
		if (input queue is empty)
			pm_relax();
		...
	}

I left out the device_may_wakeup tests; things become rather 
complicated if you can have more than one keyboard feeding the same 
input queue and some of them are wakeup-enabled while others aren't.

Clearly the appropriate changes will depend on the subsystem and the
kind of event.  They may also end up depending on the platform; perhaps
this will be used only on relatively small systems like an Android
phone.

Alan Stern