[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[Pavel Machek]

Hi!

> > > "too heavy" (in fact it's much lighter weight than resuming all devices
> > > that your approach doesn't prevent from happening, so for example on my
> > > desktop/notebook machines I woulnd't mind at all if user space were
> > > thawed after all of the devices had been resumed).
> > 
> > Well, it would be behavior change for the user. I told the zaurus to
> > go s2ram, I do not expect it to wake up after 5 minutes because it
> > needed to check battery status.
> > 
> > I'd have to modify my userland to retry suspend again and again and
> > again...
> 
> And that's exactly what should be done.  Have a user space process controlling
> that, because avoiding to thaw user space doesn't buy us almost
> anything.

That makes Zaurus implement different user-kernel interface than PC
class machine, because of hardware quirk.

> Now, I know that it's probably easier to modify the kernel than to write
> a user space tool for that, test it and so on, but "easier" is not necessarily
> "better".

It is easier, allows us to keep same user-kernel interface on PC and
Zaurus, and is compatible with 2.6.38.

Heck, I'm used to typing "echo mem > /sys/power/state". I should not
have to learn different interface just because Zaurus does not have
proper hardware charger.

> > I'm not sure if we need to cover hibernation. Do you know any machine
> > that needs this for hibernation case?
> 
> Yes, any machine that "needs" it while suspended.  What's the difference,
> after all?  The only difference is that there's an image on permanent storage
> in the hibernation case.  You still can overheat a battery when charging it
> while hibernated, right?

No, you can not; not on Zaurus.

It can not really power down; it is always sleeping. s2ram is sleep in
operating system, hibernation or poweroff is sleep in bootloader.

Bootloader takes care of battery in that case.

> > > To conclude, I'm not sure about the approach.  In particular, I'm not sure
> > > if the benefit is worth the effort and the resulting complications (ie. the
> > > possibility of having to deal with wakeup signals not requested by user
> > > space) seem to be a bit too far reaching.
> > 
> > We already have platform-specific hacks to do exactly this at least on
> > Zaurus. Moving it to common code means that hacks are not duplicated..
> 
> Well, good to know they are there, but I'm still not sure what to do about
> that.  At the moment I feel like having too little information to really
> decide, so perhaps please point me to the code you're talking about for
> starters.

Ok, see the spitz_should_wakeup() function in arch/arm/mach-pxa/* and
should_wakeup() usage.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[Rafael J. Wysocki]

On Tuesday, April 26, 2011, Pavel Machek wrote:
> Hi!
> 
> > > > "too heavy" (in fact it's much lighter weight than resuming all devices
> > > > that your approach doesn't prevent from happening, so for example on my
> > > > desktop/notebook machines I woulnd't mind at all if user space were
> > > > thawed after all of the devices had been resumed).
> > > 
> > > Well, it would be behavior change for the user. I told the zaurus to
> > > go s2ram, I do not expect it to wake up after 5 minutes because it
> > > needed to check battery status.
> > > 
> > > I'd have to modify my userland to retry suspend again and again and
> > > again...
> > 
> > And that's exactly what should be done.  Have a user space process controlling
> > that, because avoiding to thaw user space doesn't buy us almost
> > anything.
> 
> That makes Zaurus implement different user-kernel interface than PC
> class machine, because of hardware quirk.

Let me say that again: If Zaurus needs to resume everything except for
user space periodically to monitor the battery charger, I'm not sure if our
suspend interface is the right one for it in the first place.

It seriously looks like a workaround for the lack of appropriately implemented
runtime PM, just like the Android's opportunistic suspend.

> > Now, I know that it's probably easier to modify the kernel than to write
> > a user space tool for that, test it and so on, but "easier" is not necessarily
> > "better".
> 
> It is easier, allows us to keep same user-kernel interface on PC and
> Zaurus, and is compatible with 2.6.38.
> 
> Heck, I'm used to typing "echo mem > /sys/power/state". I should not
> have to learn different interface just because Zaurus does not have
> proper hardware charger.

No, this interface should not be used on Zaurus at all.  It's not mean for
that and while you can hack it to kind of work, it still is hacking rather
than designing things.

> > > I'm not sure if we need to cover hibernation. Do you know any machine
> > > that needs this for hibernation case?
> > 
> > Yes, any machine that "needs" it while suspended.  What's the difference,
> > after all?  The only difference is that there's an image on permanent storage
> > in the hibernation case.  You still can overheat a battery when charging it
> > while hibernated, right?
> 
> No, you can not; not on Zaurus.
> 
> It can not really power down; it is always sleeping. s2ram is sleep in
> operating system,

Which is not the meaning of S2RAM I use.

> hibernation or poweroff is sleep in bootloader.
> 
> Bootloader takes care of battery in that case.

So the difference is that we let someone else worry.  Cool. :-)

> > > > To conclude, I'm not sure about the approach.  In particular, I'm not sure
> > > > if the benefit is worth the effort and the resulting complications (ie. the
> > > > possibility of having to deal with wakeup signals not requested by user
> > > > space) seem to be a bit too far reaching.
> > > 
> > > We already have platform-specific hacks to do exactly this at least on
> > > Zaurus. Moving it to common code means that hacks are not duplicated..
> > 
> > Well, good to know they are there, but I'm still not sure what to do about
> > that.  At the moment I feel like having too little information to really
> > decide, so perhaps please point me to the code you're talking about for
> > starters.
> 
> Ok, see the spitz_should_wakeup() function in arch/arm/mach-pxa/* and
> should_wakeup() usage.

OK, I will.

Thanks,
Rafael

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[Pavel Machek]

Hi!

> > > And that's exactly what should be done.  Have a user space process controlling
> > > that, because avoiding to thaw user space doesn't buy us almost
> > > anything.
> > 
> > That makes Zaurus implement different user-kernel interface than PC
> > class machine, because of hardware quirk.
> 
> Let me say that again: If Zaurus needs to resume everything except for
> user space periodically to monitor the battery charger, I'm not sure if our
> suspend interface is the right one for it in the first place.

Well, Zaurus does not need to resume everything. SPI+charger code is
enough, and that would be preffered. No need to touch disk etc.

> It seriously looks like a workaround for the lack of appropriately implemented
> runtime PM, just like the Android's opportunistic suspend.

No, not really. I'm running standard Debian; I do not want/need
anything like opportunistic suspend.

> > > Now, I know that it's probably easier to modify the kernel than to write
> > > a user space tool for that, test it and so on, but "easier" is not necessarily
> > > "better".
> > 
> > It is easier, allows us to keep same user-kernel interface on PC and
> > Zaurus, and is compatible with 2.6.38.
> > 
> > Heck, I'm used to typing "echo mem > /sys/power/state". I should not
> > have to learn different interface just because Zaurus does not have
> > proper hardware charger.
> 
> No, this interface should not be used on Zaurus at all.  It's not mean for
> that and while you can hack it to kind of work, it still is hacking rather
> than designing things.

Why not? I want the Zaurus to sleep. Why should I have to know how its
charging unit works? Why should I use different interface than on PC?
I want it to suspend, put it into backpack and move...

> > Bootloader takes care of battery in that case.
> 
> So the difference is that we let someone else worry.  Cool. :-)

Yes.

> > Ok, see the spitz_should_wakeup() function in arch/arm/mach-pxa/* and
> > should_wakeup() usage.
> 
> OK, I will.

Thanks.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[Rafael J. Wysocki]

On Tuesday, April 26, 2011, Rafael J. Wysocki wrote:
> On Tuesday, April 26, 2011, Pavel Machek wrote:
...
> > Ok, see the spitz_should_wakeup() function in arch/arm/mach-pxa/* and
> > should_wakeup() usage.
> 
> OK, I will.

Well, as far as I can tell, on Zaurus this is all done within the platform
->enter() callback, so it doesn't carry out the device resume, while the
proposed $subject patch does.  For this reason, I'm not even sure if the
proposed approach is really suitable for Zaurus (the resuming and suspending
of all devices every once a while will cost quite some enery I guess).

Still, in the unlikely case it is suitable, here's my opinion.

I don't think syscore_ops is the right place to put the new "suspend
again" callback for reasons stated previously.

I also don't think dev_pm_ops is the right place to put such a callback,
because it will only be necessary on very few platforms and there's no
reason why every driver, subsystem or power domain in the kernel should care.

Moreover, the usage cases appear to be suspend-specific.

For the above reasons, the only place the "suspend again" callback may be
put into is struct platform_suspend_ops.  Then, suspend_devices_and_enter()
may be modified so that it loops between dpm_suspend_start() and
dpm_suspend_end() until that callback returns "false" and there are no
wakeup events (as indicated by pm_wakeup_pending(); but please note that
this would require suspend_enter() to indicate that pm_wakeup_pending()
returned "true" and events_check_enabled should not be reset until
we're sure that we _really_ want to resume).

Alternatively, if that's sufficient, suspend_enter() may be called in a
loop until the "suspend again" callback returns "false" and there are no
wakeup events (in the above sense).

The callback can be called "repeat" and return bool as far as I'm concerned,
but I'm not insisting on that.

MyungJoo, would that be suitable to you?

Rafael

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[MyungJoo Ham]

On Wed, Apr 27, 2011 at 7:32 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, April 26, 2011, Rafael J. Wysocki wrote:
>> On Tuesday, April 26, 2011, Pavel Machek wrote:
> ...
>> > Ok, see the spitz_should_wakeup() function in arch/arm/mach-pxa/* and
>> > should_wakeup() usage.
>>
>> OK, I will.
>
> Well, as far as I can tell, on Zaurus this is all done within the platform
> ->enter() callback, so it doesn't carry out the device resume, while the
> proposed $subject patch does. ?For this reason, I'm not even sure if the
> proposed approach is really suitable for Zaurus (the resuming and suspending
> of all devices every once a while will cost quite some enery I guess).

In fact, the kernel "hacks" of mine have been looping at
sysdev-suspend/resume implemented with platform_suspend_ops; thus it
does not carry device resume either.
I've moved the looping point to general device-suspend/resume because
I thought that other systems may require active devices.
However, if others can be satisfied without resumed devices, it is
perfect with me to move that inside device resume as well.

>
> Still, in the unlikely case it is suitable, here's my opinion.
>
> I don't think syscore_ops is the right place to put the new "suspend
> again" callback for reasons stated previously.
>
> I also don't think dev_pm_ops is the right place to put such a callback,
> because it will only be necessary on very few platforms and there's no
> reason why every driver, subsystem or power domain in the kernel should care.
>
> Moreover, the usage cases appear to be suspend-specific.
>
> For the above reasons, the only place the "suspend again" callback may be
> put into is struct platform_suspend_ops. ?Then, suspend_devices_and_enter()
> may be modified so that it loops between dpm_suspend_start() and
> dpm_suspend_end() until that callback returns "false" and there are no
> wakeup events (as indicated by pm_wakeup_pending(); but please note that
> this would require suspend_enter() to indicate that pm_wakeup_pending()
> returned "true" and events_check_enabled should not be reset until
> we're sure that we _really_ want to resume).
>
> Alternatively, if that's sufficient, suspend_enter() may be called in a
> loop until the "suspend again" callback returns "false" and there are no
> wakeup events (in the above sense).
>
> The callback can be called "repeat" and return bool as far as I'm concerned,
> but I'm not insisting on that.
>
> MyungJoo, would that be suitable to you?

As long as sysdevs are resumed and some devices/subsystems (I2C-PMIC,
ADC, and RTC in my cases) can be selectively resumed and suspended, it
is fine.
Thus, your "alternative" suggestion is perfect with me. Actually, this
is almost going back to the original hack. =)

I'll submit next revision with platform_suspend_ops later.

Thank you!

>
> Rafael
>

Cheers!
- MyungJoo
-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[Stanislav Brabec]

MyungJoo Ham wrote:

> As long as sysdevs are resumed and some devices/subsystems (I2C-PMIC,
> ADC, and RTC in my cases) can be selectively resumed and suspended, it
> is fine.
> Thus, your "alternative" suggestion is perfect with me. Actually, this
> is almost going back to the original hack. =)
> 
> I'll submit next revision with platform_suspend_ops later.

Does kernel have something like "sleepy task" interface, e. g. special
mode that is triggered by some sort of interrupt and instead of going to
perform full resume, it just lets a hook to wake up "manually" needed
devices, perform the "sleepy task" and then tell the system whether full
resume is requested?

It can fit for Zaurus battery charging monitoring - timer interrupt
needs to wake just the SPI bus.

But I can imagine a GPS logger using such interface to save energy -
serial interrupt semi-wakes the system each second, but will not go to
do full resume. It just processes NMEA sentence and buffers the result.
Only if buffer becomes full, it issues full resume and writes data
somewhere.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec at suse.cz
Lihovarsk? 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                  fax: +420 284 028 951
Czech Republic                                    http://www.suse.cz/

[RFC PATCH v2 1/3] PM / Core: suspend_again callback for device PM.

[MyungJoo Ham]

On Wed, Apr 27, 2011 at 6:46 PM, Stanislav Brabec <utx@penguin.cz> wrote:
> MyungJoo Ham wrote:
>
>> As long as sysdevs are resumed and some devices/subsystems (I2C-PMIC,
>> ADC, and RTC in my cases) can be selectively resumed and suspended, it
>> is fine.
>> Thus, your "alternative" suggestion is perfect with me. Actually, this
>> is almost going back to the original hack. =)
>>
>> I'll submit next revision with platform_suspend_ops later.
>
> Does kernel have something like "sleepy task" interface, e. g. special
> mode that is triggered by some sort of interrupt and instead of going to
> perform full resume, it just lets a hook to wake up "manually" needed
> devices, perform the "sleepy task" and then tell the system whether full
> resume is requested?

Not that I know of. I don't see anything that stops the resuming flow
(full resume will be performed at any wakeup event before this patch)

The patch that is going to be revised as mentioned will allow you to perform
1) "manually" wake up some devices
2) perform "sleepy tasks"
3) "manually" suspend devices waked up at 1)
4) tell the system whether full resume is requested.
at the suspend_again callback of struct platform_suspend_ops suspend_ops.

However, you may need some "helpers" to do 1 and 3 easily.

Anyway, for the "helpers" for 1) and 3), I expect that allowing the
one who provide suspend_ops to provide a list of struct dev pointers
that required wakeup would be enough.

I'd look at this issue as well because I have the same issue, but I'd
do it separately with the "suspend_again".

>
> It can fit for Zaurus battery charging monitoring - timer interrupt
> needs to wake just the SPI bus.
>
> But I can imagine a GPS logger using such interface to save energy -
> serial interrupt semi-wakes the system each second, but will not go to
> do full resume. It just processes NMEA sentence and buffers the result.
> Only if buffer becomes full, it issues full resume and writes data
> somewhere.

Sure, as long as the platform file (or machine/board file) has the
suspend_again callback implemented correctly, it will work as you
expected.

>
> --
> Best Regards / S pozdravem,
>
> Stanislav Brabec
> software developer
> ---------------------------------------------------------------------
> SUSE LINUX, s. r. o. ? ? ? ? ? ? ? ? ? ? ? ? ?e-mail: sbrabec at suse.cz
> Lihovarsk? 1060/12 ? ? ? ? ? ? ? ? ? ? ? ? ? ?tel: +49 911 7405384547
> 190 00 Praha 9 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?fax: +420 284 028 951
> Czech Republic ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?http://www.suse.cz/
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


Cheers!

- MyungJoo
-- 
MyungJoo Ham (???), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858