Hi,
If I saw it correctly, patches [2-3/3] only added the generic routine to
the platform and i2c bus types, right?
Now, this one is better than the previous one IMO, but (1) it also should
cover hibernation (I'd don't see a reason why suspend-to-RAM should be a
special case here) and (2) I don't really think that thawing user space is
"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).
On Tuesday, April 26, 2011, MyungJoo Ham wrote:
> A system or a device may need to control suspend/wakeup events. It may
> want to wakeup the system after a predefined amount of time or at a
> predefined event decided while entering suspend for polling or delayed
> work. Then, it may want to enter suspend again if its predefined wakeup
> condition is the only wakeup reason and there is no outstanding events;
> thus, it does not wakeup the userspace unnecessary and keeps suspended
> as long as possible (saving the power).
I'm not really sure if kernel subsystems should decide when to suspend,
because that's a matter of policy and as such I think it should belong to
user space. The kernel subsystems should be concerned with _how_ to
suspend, not _when_ to do that.
For example, even if your wakeup signal is the only one and it _seems_ it's
better to suspend again, it actually may be better to wake up and let user
space decide.
Moreover, I'm not sure if kernel subsystems (including drivers) should really
decide when to generate wakeup signals in the first place. Generally,
user space decides what devices should wake up the system from sleep and the
kernel should follow. So, there shouldn't be any wakeup signals enabled
beyond what user space has requested.
> Enabling a system to wakeup after a specified time can be easily
> achieved by using RTC. However, to enter suspend again immediately
> without invoking userland, we need additional features in the
> suspend framework.
>
> Such need comes from:
>
> 1. Monitoring a critical device status without interrupts that can
> wakeup the system. (in-suspend polling)
> An example is ambient temperature monitoring that needs to shut down
> the system or a specific device function if it is too hot or cold. The
> temperature of a specific device may be needed to be monitored as well;
> e.g., a charger monitors battery temperature in order to stop charging
> if overheated.
I'd say in those cases (ie. if polling is necessary to prevent some
disaster from happening) you shouldn't suspend at all and use runtime PM
instead (I know that's hard with Android, but please not that Android is
not using the mainline kernel).
> 2. Execute critical "delayed work" at suspend.
> A driver or a system/board may have a delayed work (or any similar
> things) that it wants to execute at the requested time.
> For example, some chargers want to check the battery voltage some
> time (e.g., 30 seconds) after the battery is fully charged and the
> charger has stopped. Then, the charger restarts charging if the voltage has
> dropped more than a threshold, which is smaller than "restart-charger"
> voltage, which is a threshold to restart charging regardless of the
> time passed.
Again, don't suspend if you have a usage case like this.
> This patch allows a device to provide "suspend_again" callback with
> struct dev_pm_ops in driver. With suspend_again callbacks registered
> along with supporting subsystems, the suspend framework
> (kernel/power/suspend.c) tries to enter suspend again if conditions are met.
> In general, in order to support suspend-again, subsystems simply need to
> return the value returned by the suspend_again of device as the
> pm_generic_suspend_again does.
>
> The system enters the suspend again if and only if all of the following
> conditions are met:
> 1. None of suspend_again ops returned "I want to stop suspend" by
> returning a negative number.
> 2. At least one of suspend_again ops returned "I want to suspend again"
> by returning a positive number.
>
> suspend_again ops may return "I do not care. This wakeup is not related
> with me." by providing zero.
>
> suspend_again ops may return a negative number in order to override
> other devices' "suspend again" request and to wakeup fully. Devices
> that poll sensors during suspend may need this if any outstanding status
> that requires to notify the userland is found. Conventional suspend
> wakeup sources may also need this to override SUSPEND_AGAIN devices.
> (we may need to lookup every IRQ registered with set_irq_wake and
> register IRQS that are related with suspend_ops separatedly. this is
> not implemented yet anyway and discusses below with item 3.)
>
> Anyway, the following features may need to be added later:
>
> 1. An API to allow devices to express next desired wakeup-time. Then,
> the framework will combine them and setup RTC alarm accordingly and
> save/restore previously registered RTC alarms.
> => For this, rtc_timer_init/start/cancel might work for any rtc that
> has wakeup enabled. We need to express whether the rtc_timer is going to
> be used for suspend_again so that it would not fully wakeup the system.
> => Then, we need a bogus platform-device of rtc to see if the
> current wakeup event is whether a) rtc-induced and b) suspend-again
> related rtc event. This may be implemented at drivers/rtc/interface.c?
>
> 2. Create a method to declare a specific instance of delayed-work is to
> be executed in suspend by waking up the system in the middle of
> suspend. Then, let the framework handle those "critical" delayed-work
> in suspend.
> => If the workitem 1 with identification method is done, this will
> probably be an easy feature to add. (virtually already implemented with 1.)
>
> 3. If a device says "suspend again, >0" and there is another wakeup
> source pending (e.g., power button) without suspend_again ops, the
> system will enter suspend again. In such a case, the system should not
> suspend again. We may need to see if irqs that are enabled by
> set_irq_wake() (and not related to suspend_ops devices)
> are pending at syscore_suspend_again(). Maybe we need to add
> something like "set_irq_wake_with_suspend_again" so that IRQs with
> suspend_again ops implemented are ignored for the
> "override-suspend-again-continue" checking.
> => For wakeup-enabled IRQs, such behavior of fully-waking up the system
> (by returning a negative number at suspend_again) should be default and
> should not require any modification; the change should be transparent to
> the conventional wakeup sources. Thus, we should modify IRQ core to
> do this by default and add an option not to return < 0 optionally
> (selected by suspend-again users). The implementation of 1 and 2 should
> include this in their code as well.
>
> In this patch, the suspend-again loops at "suspend_devices_and_enter"
> because at that point,
> 1. every device is resume and are ready to be used.
> 2. userspace is still not invoked (transparent to it)
> 3. console is accessible as it is outside of suspend/resume_console.
> 4. it is outside of trace_machine_suspend(); thus, we can have seperated
> traces for each instance of suspend_again.
>
> For the RFC release, I have set the point of "suspend-again" after
> suspend_ops->end(). However, I'm not so sure about where to set the
> suspend-again point. Because in-suspend polling, which may require
> I/O with other devices, is supposed to be executed at suspend-again ops,
> the suspend-again point is configured to be as late as possible in
> suspend_devices_and_enter(). In order to reduce the number of devices
> waked up, we may need to set the suspend-again point ealier.
>
> Signed-off-by: MyungJoo Ham <[email protected]>
> Signed-off-by: Kyungmin Park <[email protected]>
>
> --
> Changes from v1:
> - moved from syscore to dev_pm_ops
> - added generic ops for subsystems.
> - cleaned up suspend_again code at kernel/power/suspend.
> ---
> drivers/base/power/generic_ops.c | 17 ++++++++++
> drivers/base/power/main.c | 61 ++++++++++++++++++++++++++++++++++++++
> include/linux/pm.h | 17 ++++++++++
> kernel/power/suspend.c | 4 ++-
> 4 files changed, 98 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> index 42f97f9..0976452 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -213,6 +213,22 @@ int pm_generic_restore(struct device *dev)
> return __pm_generic_resume(dev, PM_EVENT_RESTORE);
> }
> EXPORT_SYMBOL_GPL(pm_generic_restore);
> +
> +/**
> + * pm_generic_suspend_again - Generic suspend-again callback for subsystems.
> + * @dev: Device to check for suspend-again request.
> + */
> +int pm_generic_suspend_again(struct device *dev)
> +{
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +
> + if (!pm || !pm->suspend_again)
> + return 0;
> +
> + return pm->suspend_again(dev);
> +}
> +EXPORT_SYMBOL_GPL(pm_generic_suspend_again);
> +
> #endif /* CONFIG_PM_SLEEP */
>
> struct dev_pm_ops generic_subsys_pm_ops = {
> @@ -223,6 +239,7 @@ struct dev_pm_ops generic_subsys_pm_ops = {
> .thaw = pm_generic_thaw,
> .poweroff = pm_generic_poweroff,
> .restore = pm_generic_restore,
> + .suspend_again = pm_generic_suspend_again,
> #endif
> #ifdef CONFIG_PM_RUNTIME
> .runtime_suspend = pm_generic_runtime_suspend,
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 052dc53..a5b659f 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1082,3 +1082,64 @@ int device_pm_wait_for_dev(struct device *subordinate, struct device *dev)
> return async_error;
> }
> EXPORT_SYMBOL_GPL(device_pm_wait_for_dev);
> +
> +/**
> + * dpm_suspend_again - Let devices poll or run code while the system is kept
> + * suspended in the view of userland.
> + *
> + * Execute all "suspend_again" callbacks of devices and return non-zero value
> + * if all of the following conditions are met:
> + * 1) There is a device that wants to suspend again (returned > 0)
> + * 2) There is no device that wants to wake up (returned < 0)
> + */
> +int dpm_suspend_again(void)
> +{
> + int suspend_again = 0;
> + int wakeup = 0;
> + struct device *dev;
> +
> + mutex_lock(&dpm_list_mtx);
> +
> + list_for_each_entry(dev, &dpm_list, power.entry) {
> + const struct dev_pm_ops *ops = NULL;
> + int result = 0;
> +
> + get_device(dev);
> + mutex_unlock(&dpm_list_mtx);
> + device_lock(dev);
> +
Please remember of power domains (that's being worked on at the moment).
> + if (dev->type && dev->type->pm) {
> + ops = dev->type->pm;
> + dev_dbg(dev, "type suspend_again ");
> + } else if (dev->class && dev->class->pm) {
> + ops = dev->class->pm;
> + dev_dbg(dev, "class suspend_again ");
> + } else if (dev->bus && dev->bus->pm) {
> + ops = dev->bus->pm;
> + dev_dbg(dev, "suspend_again ");
> + }
> + if (ops && ops->suspend_again)
> + result = ops->suspend_again(dev);
> +
> + device_unlock(dev);
> + mutex_lock(&dpm_list_mtx);
> + put_device(dev);
> +
> + if (result > 0)
> + suspend_again++;
> + if (result < 0)
> + wakeup++;
> + }
> + mutex_unlock(&dpm_list_mtx);
> +
> + pr_debug("%d devices wants to suspend again. "
> + "%d devices wants to wakeup.\n", suspend_again, wakeup);
> +
> + if (suspend_again && !wakeup) {
> + pr_info("Suspending Again.\n");
> + return 1;
> + }
> +
> + pr_info("No devices wants to suspend again or a device wants to wakeup.\n");
> + return 0;
> +}
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 512e091..d3a9c67 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -145,6 +145,15 @@ typedef struct pm_message {
> * actions required for resuming the device that need interrupts to be
> * disabled
> *
> + * @suspend_again: Tell the system whether the device wants to either
> + * suspend again (return > 0), wake up (return < 0), or not-care
> + * (return = 0). If a device wants to poll sensors or execute some code
> + * during suspended, suspend_again callback is the place assuming that
> + * periodic-wakeup or alarm-wakeup is already setup. Note that if a
> + * device returns "wakeup" with an under-zero value, it overrides
> + * other devices' "suspend again" return values. This allows to
> + * execute some codes while being kept suspended in the view of userland.
> + *
Since, as I said, I think that should cover hibernation too, I'd change the
name to, say, sleep_again().
> * @freeze_noirq: Complete the operations of ->freeze() by carrying out any
> * actions required for freezing the device that need interrupts to be
> * disabled
> @@ -212,6 +221,7 @@ struct dev_pm_ops {
> int (*restore)(struct device *dev);
> int (*suspend_noirq)(struct device *dev);
> int (*resume_noirq)(struct device *dev);
> + int (*suspend_again)(struct device *dev);
> int (*freeze_noirq)(struct device *dev);
> int (*thaw_noirq)(struct device *dev);
> int (*poweroff_noirq)(struct device *dev);
> @@ -544,6 +554,7 @@ extern void dpm_resume_end(pm_message_t state);
> extern void device_pm_unlock(void);
> extern int dpm_suspend_noirq(pm_message_t state);
> extern int dpm_suspend_start(pm_message_t state);
> +extern int dpm_suspend_again(void);
>
> extern void __suspend_report_result(const char *function, void *fn, int ret);
>
> @@ -563,6 +574,11 @@ static inline int dpm_suspend_start(pm_message_t state)
> return 0;
> }
>
> +static int dpm_suspend_again(void)
> +{
> + return 0;
> +}
> +
> #define suspend_report_result(fn, ret) do {} while (0)
>
> static inline int device_pm_wait_for_dev(struct device *a, struct device *b)
> @@ -585,5 +601,6 @@ extern int pm_generic_freeze(struct device *dev);
> extern int pm_generic_thaw(struct device *dev);
> extern int pm_generic_restore(struct device *dev);
> extern int pm_generic_poweroff(struct device *dev);
> +extern int pm_generic_suspend_again(struct device *dev);
>
> #endif /* _LINUX_PM_H */
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 08515b4..b5a84a4 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -291,7 +291,9 @@ int enter_state(suspend_state_t state)
> goto Finish;
>
> pr_debug("PM: Entering %s sleep\n", pm_states[state]);
> - error = suspend_devices_and_enter(state);
> + do {
> + error = suspend_devices_and_enter(state);
> + } while (!error && dpm_suspend_again());
>
> Finish:
> pr_debug("PM: Finishing wakeup.\n");
>
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.
Greg, what do you think?
Thanks,
Rafael
On Tue, Apr 26, 2011 at 01:47:21PM +0200, Rafael J. Wysocki wrote:
> Moreover, I'm not sure if kernel subsystems (including drivers) should really
> decide when to generate wakeup signals in the first place. Generally,
> user space decides what devices should wake up the system from sleep and the
> kernel should follow. So, there shouldn't be any wakeup signals enabled
> beyond what user space has requested.
The RTC wakeup signals are ok though, right? Userspace is the one
asking for that from what I can tell.
> 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.
>
> Greg, what do you think?
I agree with you in that I don't think that this type of feature is
valid at the moment.
I don't understand why our current situation doesn't work, what are we
lacking that is needed for these systems that we have not seen before?
What is the root problem that this is trying to solve?
thanks,
greg k-h
Hi!
> If I saw it correctly, patches [2-3/3] only added the generic routine to
> the platform and i2c bus types, right?
>
> Now, this one is better than the previous one IMO, but (1) it also should
> cover hibernation (I'd don't see a reason why suspend-to-RAM should be a
> special case here) and (2) I don't really think that thawing user
> space is
Suspend-to-RAM really is special here... at least for zaurus-like
machine.
In hibernation, you are "powered down"; that means suspend to RAM with
bootloader active (operating system is off). You do not need to do any
kind of maintenance -- bootloader takes care of battery charging and
protection.
> "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...
> > @@ -145,6 +145,15 @@ typedef struct pm_message {
> > * actions required for resuming the device that need interrupts to be
> > * disabled
> > *
> > + * @suspend_again: Tell the system whether the device wants to either
> > + * suspend again (return > 0), wake up (return < 0), or not-care
> > + * (return = 0). If a device wants to poll sensors or execute some code
> > + * during suspended, suspend_again callback is the place assuming that
> > + * periodic-wakeup or alarm-wakeup is already setup. Note that if a
> > + * device returns "wakeup" with an under-zero value, it overrides
> > + * other devices' "suspend again" return values. This allows to
> > + * execute some codes while being kept suspended in the view of userland.
> > + *
>
> Since, as I said, I think that should cover hibernation too, I'd change the
> name to, say, sleep_again().
I'm not sure if we need to cover hibernation. Do you know any machine
that needs this for hibernation case?
> > @@ -291,7 +291,9 @@ int enter_state(suspend_state_t state)
> > goto Finish;
> >
> > pr_debug("PM: Entering %s sleep\n", pm_states[state]);
> > - error = suspend_devices_and_enter(state);
> > + do {
> > + error = suspend_devices_and_enter(state);
> > + } while (!error && dpm_suspend_again());
> >
> > Finish:
> > pr_debug("PM: Finishing wakeup.\n");
> >
>
> 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..
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > 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.
> >
> > Greg, what do you think?
>
> I agree with you in that I don't think that this type of feature is
> valid at the moment.
Our current "solution" is low level suspend code on Zaurus directly
looking at charger state and doing the "wakeup or not" decision by hand.
> I don't understand why our current situation doesn't work, what are we
> lacking that is needed for these systems that we have not seen
> before?
It works, but it is ugly; and it seems samsung now needs similar
hacks.
> What is the root problem that this is trying to solve?
It is trying to fix machines that need to run periodic kernel tasks
even when user asked them to sleep. Zaurus needs to periodicaly wake
up to be able to charge battery in s2ram state, for example.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tuesday, April 26, 2011, Pavel Machek wrote:
> Hi!
>
> > If I saw it correctly, patches [2-3/3] only added the generic routine to
> > the platform and i2c bus types, right?
> >
> > Now, this one is better than the previous one IMO, but (1) it also should
> > cover hibernation (I'd don't see a reason why suspend-to-RAM should be a
> > special case here) and (2) I don't really think that thawing user
> > space is
>
> Suspend-to-RAM really is special here... at least for zaurus-like
> machine.
>
> In hibernation, you are "powered down"; that means suspend to RAM with
> bootloader active (operating system is off). You do not need to do any
> kind of maintenance -- bootloader takes care of battery charging and
> protection.
>
> > "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.
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".
> > > @@ -145,6 +145,15 @@ typedef struct pm_message {
> > > * actions required for resuming the device that need interrupts to be
> > > * disabled
> > > *
> > > + * @suspend_again: Tell the system whether the device wants to either
> > > + * suspend again (return > 0), wake up (return < 0), or not-care
> > > + * (return = 0). If a device wants to poll sensors or execute some code
> > > + * during suspended, suspend_again callback is the place assuming that
> > > + * periodic-wakeup or alarm-wakeup is already setup. Note that if a
> > > + * device returns "wakeup" with an under-zero value, it overrides
> > > + * other devices' "suspend again" return values. This allows to
> > > + * execute some codes while being kept suspended in the view of userland.
> > > + *
> >
> > Since, as I said, I think that should cover hibernation too, I'd change the
> > name to, say, sleep_again().
>
> 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?
> > > @@ -291,7 +291,9 @@ int enter_state(suspend_state_t state)
> > > goto Finish;
> > >
> > > pr_debug("PM: Entering %s sleep\n", pm_states[state]);
> > > - error = suspend_devices_and_enter(state);
> > > + do {
> > > + error = suspend_devices_and_enter(state);
> > > + } while (!error && dpm_suspend_again());
> > >
> > > Finish:
> > > pr_debug("PM: Finishing wakeup.\n");
> > >
> >
> > 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.
Thanks,
Rafael
On Tuesday, April 26, 2011, Pavel Machek wrote:
> Hi!
>
> > > 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.
> > >
> > > Greg, what do you think?
> >
> > I agree with you in that I don't think that this type of feature is
> > valid at the moment.
>
> Our current "solution" is low level suspend code on Zaurus directly
> looking at charger state and doing the "wakeup or not" decision by hand.
>
> > I don't understand why our current situation doesn't work, what are we
> > lacking that is needed for these systems that we have not seen
> > before?
>
> It works, but it is ugly; and it seems samsung now needs similar
> hacks.
>
> > What is the root problem that this is trying to solve?
>
> It is trying to fix machines that need to run periodic kernel tasks
> even when user asked them to sleep. Zaurus needs to periodicaly wake
> up to be able to charge battery in s2ram state, for example.
Well, if you wake up periodically, it's not S2RAM any more, I'd say.
Thanks,
Rafael
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
Hi!
> > > What is the root problem that this is trying to solve?
> >
> > It is trying to fix machines that need to run periodic kernel tasks
> > even when user asked them to sleep. Zaurus needs to periodicaly wake
> > up to be able to charge battery in s2ram state, for example.
>
> Well, if you wake up periodically, it's not S2RAM any more, I'd say.
It certainly looks a lot like s2ram to the user.
Now, your PC wakes up periodically in s2ram, too; but it is keyboard
controller that wakes up, not CPU, so you do not know.
On Zaurus, you do not know, either; nothing visible happens and
userspace is shielded from these details.
Should user really have to know if battery charger is implemented on
keyboard controller or on main cpu before he selects which interface
to use?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Apr 26, 2011 at 10:38:53PM +0200, Pavel Machek wrote:
> Hi!
>
> > > 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.
> > >
> > > Greg, what do you think?
> >
> > I agree with you in that I don't think that this type of feature is
> > valid at the moment.
>
> Our current "solution" is low level suspend code on Zaurus directly
> looking at charger state and doing the "wakeup or not" decision by hand.
>
> > I don't understand why our current situation doesn't work, what are we
> > lacking that is needed for these systems that we have not seen
> > before?
>
> It works, but it is ugly; and it seems samsung now needs similar
> hacks.
>
> > What is the root problem that this is trying to solve?
>
> It is trying to fix machines that need to run periodic kernel tasks
> even when user asked them to sleep. Zaurus needs to periodicaly wake
> up to be able to charge battery in s2ram state, for example.
That sounds like a bug in userspace you are trying to work around. Why
not solve it there first?
thanks,
greg k-h
On Tuesday, April 26, 2011, Pavel Machek wrote:
> Hi!
>
> > > > What is the root problem that this is trying to solve?
> > >
> > > It is trying to fix machines that need to run periodic kernel tasks
> > > even when user asked them to sleep. Zaurus needs to periodicaly wake
> > > up to be able to charge battery in s2ram state, for example.
> >
> > Well, if you wake up periodically, it's not S2RAM any more, I'd say.
>
> It certainly looks a lot like s2ram to the user.
>
> Now, your PC wakes up periodically in s2ram, too; but it is keyboard
> controller that wakes up, not CPU, so you do not know.
I seriously doubt it's a keyboard controller in contemporary machines.
Yes, there are parts of them that don't actually sleep, but that's not
under OS control.
> On Zaurus, you do not know, either; nothing visible happens and
> userspace is shielded from these details.
If you wake up device drivers, it _is_ visible. Maybe not to user
space, but that's a semantic difference.
> Should user really have to know if battery charger is implemented on
> keyboard controller or on main cpu before he selects which interface
> to use?
Please don't confuse user space with the user. You can resume all the
way to user space without turning the backlight on and the user will not
notice, right?
Rafael
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
Hi!
> > It certainly looks a lot like s2ram to the user.
> >
> > Now, your PC wakes up periodically in s2ram, too; but it is keyboard
> > controller that wakes up, not CPU, so you do not know.
>
> I seriously doubt it's a keyboard controller in contemporary machines.
> Yes, there are parts of them that don't actually sleep, but that's not
> under OS control.
Ok, so it is called "embedded controller" these days. But you got the
idea.
> > On Zaurus, you do not know, either; nothing visible happens and
> > userspace is shielded from these details.
>
> If you wake up device drivers, it _is_ visible. Maybe not to user
> space, but that's a semantic difference.
Just to clarify; not _all_ device drivers need to be woken up. Just
SPI + charger code is enough... and I guess it would be prefered
because spinning up disk just to toggle charger would be nasty.
> > Should user really have to know if battery charger is implemented on
> > keyboard controller or on main cpu before he selects which interface
> > to use?
>
> Please don't confuse user space with the user. You can resume all the
> way to user space without turning the backlight on and the user will not
> notice, right?
The user has to type "echo mem > /sys/power/state" :-). And you better
turn him the backlight on, typing blind is bad.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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
Hi!
> > > What is the root problem that this is trying to solve?
> >
> > It is trying to fix machines that need to run periodic kernel tasks
> > even when user asked them to sleep. Zaurus needs to periodicaly wake
> > up to be able to charge battery in s2ram state, for example.
>
> That sounds like a bug in userspace you are trying to work around. Why
> not solve it there first?
Can you be more specific?
The userspace is standard Debian; or maybe even init=/bin/bash. I want
to sleep the machine before I move, so I do echo mem >
/sys/power/state, as I do on PC.
Then I do plug the machine into AC when I move near outlet. In PC,
embedded controller wakes up and starts charging. In Zaurus, it is
kernel (SPI+charging drivers) that wake up, start charging, and go
back to sleep.
Then I (the user) press the power button and get "bash#" back. Where
is the userspace bug?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
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
On Wed, Apr 27, 2011 at 7:32 AM, Rafael J. Wysocki <[email protected]> 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
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: [email protected]
Lihovarská 1060/12 tel: +49 911 7405384547
190 00 Praha 9 fax: +420 284 028 951
Czech Republic http://www.suse.cz/
On Wed, Apr 27, 2011 at 6:46 PM, Stanislav Brabec <[email protected]> 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: [email protected]
> 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
> [email protected]
> 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