From: Rafael J. Wysocki on
On Monday, August 09, 2010, Arve Hj�nnev�g wrote:
> 2010/8/8 Rafael J. Wysocki <rjw(a)sisk.pl>:
> > On Saturday, August 07, 2010, Arve Hj�nnev�g wrote:
> >> 2010/8/7 Arve Hj�nnev�g <arve(a)android.com>:
> >> > 2010/8/7 Rafael J. Wysocki <rjw(a)sisk.pl>:
> >> >> On Saturday, August 07, 2010, Arve Hj�nnev�g wrote:
> >> >>> 2010/8/6 Alan Stern <stern(a)rowland.harvard.edu>:
> >> >>> > On Thu, 5 Aug 2010, Arve Hj�nnev�g wrote:
> >> ...
> >> >>> >> total_time, total time the wake lock has been active. This one should
> >> >>> >> be obvious.
> >> >>> >
> >> >>> > Also easily added.
> >> >>> >
> >> >>> Only with a handle passed to all the calls.
> >> >>
> >> >> Well, I'm kind of tired of this "my solution is the only acceptable one"
> >> >> mindset. IMHO, it's totally counter productive.
> >> >>
> >> >
> >> > How do you propose to track how long a driver has blocked suspend when
> >> > you have an unblock call that takes no arguments.
> >> >
> >>
> >> Also, I did not not see a response to my question about why you don't
> >> want to pass a handle.
> >
> > It doesn't really matter what I personally want. In fact, I'm not totally
> > opposed to that idea, although there are disadvantages (eg. a "handle"
> > would really mean a pointer to an object with certain life cycle that needs to
> > be managed by the caller and it's not that clear to me who should manage the
> > objects that the PCI wakeup code would pass to pm_wakeup_event(), for one
>
> Wouldn't a single global handle work for the way you are handling pci
> wakeup events?

Not really, because I'd like to know the number of wakeups associated with
the given device.

> It looked like you just reset a global timeout every time a pci wakeup event
> occurs.

We bump up the per-device counter of wakeup events in addition to that.

> > example). I sent a pull request for your original patchset to Linus after all. :-)
> >
> > I said I didn't think "it would fly", meaning that I was afraid the other kernel
> > developers wouldn't like that change.
> >
> > The reason why I think so is that you'd like to add a whole new infrastructure
> > whose only purpose would be debugging that would only be useful to systems
> > using opportunistic suspend. That, however, is only Android right now and it
> > cannot use the mainline kernel for other reasons, so basically we would add
> > infrastructure that's useful to no one.
> >
>
> I'm not sure what you mean by this. The debugging is useful for anyone
> using the api, not just Android, and a handle is also needed to mix
> timeouts and pm_relax.

The purpose of the debugging would be to be able to figure out why the system
is staying in the working state, which is only relevant for systems that use
opportunistic suspend.

If opportunistic suspend isn't used, it makes sense to ask which
device caused suspend (initiated by the user) to be aborted and for this
purpose it is sufficient to count wakeup events associated with each
device (you need to preserve the pre-suspend values of these counters, but
that can be done by a user space power manager just fine).

> The handle can be the device, but some drivers need several handles per
> device.

That depends on how precise the collected debug information should be and
that, in turn, depends on what it's going to be used for.

Anyway, as I said I'm not opposed to the idea of using a special type of
objects for collecting debug information on wakeup events, so please free to
submit patches modifying the current mainline kernel code in that direction.

Thanks,
Rafael
--
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: Arve Hjønnevåg on
2010/8/9 Rafael J. Wysocki <rjw(a)sisk.pl>:
> On Monday, August 09, 2010, Arve Hj�nnev�g wrote:
>> 2010/8/8 Rafael J. Wysocki <rjw(a)sisk.pl>:
>> > On Saturday, August 07, 2010, Arve Hj�nnev�g wrote:
>> >> 2010/8/7 Arve Hj�nnev�g <arve(a)android.com>:
>> >> > 2010/8/7 Rafael J. Wysocki <rjw(a)sisk.pl>:
>> >> >> On Saturday, August 07, 2010, Arve Hj�nnev�g wrote:
>> >> >>> 2010/8/6 Alan Stern <stern(a)rowland.harvard.edu>:
>> >> >>> > On Thu, 5 Aug 2010, Arve Hj�nnev�g wrote:
>> >> ...
>> >> >>> >> total_time, total time the wake lock has been active. This one should
>> >> >>> >> be obvious.
>> >> >>> >
>> >> >>> > Also easily added.
>> >> >>> >
>> >> >>> Only with a handle passed to all the calls.
>> >> >>
>> >> >> Well, I'm kind of tired of this "my solution is the only acceptable one"
>> >> >> mindset. �IMHO, it's totally counter productive.
>> >> >>
>> >> >
>> >> > How do you propose to track how long a driver has blocked suspend when
>> >> > you have an unblock call that takes no arguments.
>> >> >
>> >>
>> >> Also, I did not not see a response to my question about why you don't
>> >> want to pass a handle.
>> >
>> > It doesn't really matter what I personally want. �In fact, I'm not totally
>> > opposed to that idea, although there are disadvantages (eg. a "handle"
>> > would really mean a pointer to an object with certain life cycle that needs to
>> > be managed by the caller and it's not that clear to me who should manage the
>> > objects that the PCI wakeup code would pass to pm_wakeup_event(), for one
>>
>> Wouldn't a single global handle work for the way you are handling pci
>> wakeup events?
>
> Not really, because I'd like to know the number of wakeups associated with
> the given device.
>

For debugging purposes right? I'm not sure automatically counting
wakeup events from pm_stay_awake and pm_wakeup_event is the best way
to do this. To avoid race conditions these calls have to be made for
every event that could also be a wakeup event, which means the actual
wakeup event easily gets lost in the noise. This is why I had a
separate wakeup count that only increments on the fist call to
wake_lock after each suspend, but this did not work that well either.

>> It looked like you just reset a global timeout every time a pci wakeup event
>> occurs.
>
> We bump up the per-device counter of wakeup events in addition to that.
>
>> > example). �I sent a pull request for your original patchset to Linus after all. :-)
>> >
>> > I said I didn't think "it would fly", meaning that I was afraid the other kernel
>> > developers wouldn't like that change.
>> >
>> > The reason why I think so is that you'd like to add a whole new infrastructure
>> > whose only purpose would be debugging that would only be useful to systems
>> > using opportunistic suspend. �That, however, is only Android right now and it
>> > cannot use the mainline kernel for other reasons, so basically we would add
>> > infrastructure that's useful to no one.
>> >
>>
>> I'm not sure what you mean by this. The debugging is useful for anyone
>> using the api, not just Android, and a handle is also needed to mix
>> timeouts and pm_relax.
>
> The purpose of the debugging would be to be able to figure out why the system
> is staying in the working state, which is only relevant for systems that use
> opportunistic suspend.
>

Only if the drivers do not have bugs. A driver calling pm_say_awake
without a matching pm_relax call will prevent any race free suspend
from succeeding.

> If opportunistic suspend isn't used, it makes sense to ask which
> device caused suspend (initiated by the user) to be aborted and for this
> purpose it is sufficient to count wakeup events associated with each
> device (you need to preserve the pre-suspend values of these counters, but
> that can be done by a user space power manager just fine).
>
>> The handle can be the device, but some drivers need several handles per
>> device.
>
> That depends on how precise the collected debug information should be and
> that, in turn, depends on what it's going to be used for.
>

It is not just debug information. Drivers that mix wake_lock_timeout
and wake_unlock do not map to the current api.

> Anyway, as I said I'm not opposed to the idea of using a special type of
> objects for collecting debug information on wakeup events, so please free to
> submit patches modifying the current mainline kernel code in that direction.
>

How do you prefer to handle your pci wakeup events? Add a handle to
every device or pci device? Or use a global handle to avoid the race
and report wakeup events for debugging separately?

--
Arve Hj�nnev�g
--
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: Rafael J. Wysocki on
On Tuesday, August 10, 2010, Arve Hj�nnev�g wrote:
> 2010/8/9 Rafael J. Wysocki <rjw(a)sisk.pl>:
> > On Monday, August 09, 2010, Arve Hj�nnev�g wrote:
....
>
> > Anyway, as I said I'm not opposed to the idea of using a special type of
> > objects for collecting debug information on wakeup events, so please free to
> > submit patches modifying the current mainline kernel code in that direction.
> >
>
> How do you prefer to handle your pci wakeup events? Add a handle to
> every device or pci device? Or use a global handle to avoid the race
> and report wakeup events for debugging separately?

The latter, maybe?

Thanks,
Rafael
--
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: Felipe Contreras on
On Sun, Aug 8, 2010 at 9:34 PM, Mark Brown
<broonie(a)opensource.wolfsonmicro.com> wrote:
> On 8 Aug 2010, at 18:08, Felipe Contreras <felipe.contreras(a)gmail.com> wrote:
>
>> On Sun, Aug 8, 2010 at 7:08 PM, Matthew Garrett <mjg59(a)srcf.ucam.org> wrote:
>>> It's clearly possible for a pathological Android application to destroy
>>> the power management policy. But to do that, the author would have to
>>> explicitly take a wakelock. That's difficult to do by accident.
>>
>> The writer can take a wakelock the whole time the application is
>> running (isn't that the typical case?), because perhaps the author
>> realizes that way the application works correctly, or he copy-pasted
>> it from somewhere else.
>
> That would be exceptionally unusual. A more common case is that the application will take a wakelock while performing some specific long running task which needs no user intervention such as downloading a file or displaying constantly update status that the user is not expected to respond to. There's no need for applications to take wakelocks while the user is directly interacting with them since the system will be kept awake as a result of the user interaction, the wakelocks are used to override the default suspend that occurs when the user is not interacting with the device.

Fair enough, but if that the case, if suspend blockers are to be used
in desktop software, everything would need extensive modifications
just to work. I remember somebody said that was not the case, I
thought it was because the lock could be held the whole time the
application is running.

--
Felipe Contreras
--
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/