Some platforms need to talk via I2C to power control devices during
the suspend method. Currently, they do this via the platform PM ops
prepare callback, relying on the I2C driver being hooked into the
'late' suspend method, and hence being shut down _after_ the prepare
callback.
However, as of the above commit, the ordering is changed such that
platforms don't get notified of suspends until after all devices are
well and truely shut down.
This can't work, and actively breaks some platforms.
Please come up with another solution for your PCI problems, or provide
alternative equivalent functionality where the platform code is notified
of the PM event prior to the late suspend callback being issued.
--
Russell King
On Saturday 18 April 2009, Russell King wrote:
> Some platforms need to talk via I2C to power control devices during
> the suspend method. Currently, they do this via the platform PM ops
> prepare callback, relying on the I2C driver being hooked into the
> 'late' suspend method, and hence being shut down _after_ the prepare
> callback.
Well, I was not aware of this dependency and it seems quite unusual for a
platform to depend on a driver like this.
> However, as of the above commit, the ordering is changed such that
> platforms don't get notified of suspends until after all devices are
> well and truely shut down.
>
> This can't work, and actively breaks some platforms.
Sorry for that.
The patchset in question had been discussed quite extensively before it was
merged and it's a pity none of the people caring for the affected platforms
took part in those discussions. That would allow us to avoid the breakage.
> Please come up with another solution for your PCI problems,
I don't think this is possible, sorry.
> or provide alternative equivalent functionality where the platform code is
> notified of the PM event prior to the late suspend callback being issued.
There is the .begin() callback that could be used, but if you need the
platform to be notified _just_ prior to the late suspend callback, then the
only thing I can think of at the moment is the appended patch.
It shouldn't break anything in theory, because the majority of drivers put
their devices into low power states in the "regular" suspend callbacks anyway.
Len, what do you think?
Thanks,
Rafael
---
kernel/power/disk.c | 43 +++++++++++++++++++++++--------------------
kernel/power/main.c | 19 +++++++++----------
2 files changed, 32 insertions(+), 30 deletions(-)
Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -291,21 +291,21 @@ static int suspend_enter(suspend_state_t
device_pm_lock();
- error = device_power_down(PMSG_SUSPEND);
- if (error) {
- printk(KERN_ERR "PM: Some devices failed to power down\n");
- goto Done;
- }
-
if (suspend_ops->prepare) {
error = suspend_ops->prepare();
if (error)
- goto Power_up_devices;
+ goto Done;
}
if (suspend_test(TEST_PLATFORM))
goto Platfrom_finish;
+ error = device_power_down(PMSG_SUSPEND);
+ if (error) {
+ printk(KERN_ERR "PM: Some devices failed to power down\n");
+ goto Platfrom_finish;
+ }
+
error = disable_nonboot_cpus();
if (error || suspend_test(TEST_CPUS))
goto Enable_cpus;
@@ -326,13 +326,12 @@ static int suspend_enter(suspend_state_t
Enable_cpus:
enable_nonboot_cpus();
+ device_power_up(PMSG_RESUME);
+
Platfrom_finish:
if (suspend_ops->finish)
suspend_ops->finish();
- Power_up_devices:
- device_power_up(PMSG_RESUME);
-
Done:
device_pm_unlock();
Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -217,6 +217,13 @@ static int create_image(int platform_mod
device_pm_lock();
+ error = platform_pre_snapshot(platform_mode);
+ if (error)
+ goto Unlock;
+
+ if (hibernation_test(TEST_PLATFORM))
+ goto Platform_finish;
+
/* At this point, device_suspend() has been called, but *not*
* device_power_down(). We *must* call device_power_down() now.
* Otherwise, drivers for some devices (e.g. interrupt controllers)
@@ -227,12 +234,8 @@ static int create_image(int platform_mod
if (error) {
printk(KERN_ERR "PM: Some devices failed to power down, "
"aborting hibernation\n");
- goto Unlock;
- }
-
- error = platform_pre_snapshot(platform_mode);
- if (error || hibernation_test(TEST_PLATFORM))
goto Platform_finish;
+ }
error = disable_nonboot_cpus();
if (error || hibernation_test(TEST_CPUS)
@@ -274,12 +277,12 @@ static int create_image(int platform_mod
Enable_cpus:
enable_nonboot_cpus();
- Platform_finish:
- platform_finish(platform_mode);
-
device_power_up(in_suspend ?
(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
+ Platform_finish:
+ platform_finish(platform_mode);
+
Unlock:
device_pm_unlock();
@@ -346,16 +349,16 @@ static int resume_target_kernel(bool pla
device_pm_lock();
+ error = platform_pre_restore(platform_mode);
+ if (error)
+ goto Unlock;
+
error = device_power_down(PMSG_QUIESCE);
if (error) {
printk(KERN_ERR "PM: Some devices failed to power down, "
"aborting resume\n");
- goto Unlock;
- }
-
- error = platform_pre_restore(platform_mode);
- if (error)
goto Cleanup;
+ }
error = disable_nonboot_cpus();
if (error)
@@ -398,11 +401,11 @@ static int resume_target_kernel(bool pla
Enable_cpus:
enable_nonboot_cpus();
+ device_power_up(PMSG_RECOVER);
+
Cleanup:
platform_restore_cleanup(platform_mode);
- device_power_up(PMSG_RECOVER);
-
Unlock:
device_pm_unlock();
@@ -466,17 +469,17 @@ int hibernation_platform_enter(void)
device_pm_lock();
- error = device_power_down(PMSG_HIBERNATE);
+ error = hibernation_ops->prepare();
if (error)
goto Unlock;
- error = hibernation_ops->prepare();
+ error = device_power_down(PMSG_HIBERNATE);
if (error)
goto Platofrm_finish;
error = disable_nonboot_cpus();
if (error)
- goto Platofrm_finish;
+ goto Power_up_devices;
local_irq_disable();
sysdev_suspend(PMSG_HIBERNATE);
@@ -484,6 +487,8 @@ int hibernation_platform_enter(void)
/* We should never get here */
while (1);
+ Power_up_devices:
+ device_power_up(PMSG_RESTORE);
/*
* We don't need to reenable the nonboot CPUs or resume consoles, since
* the system is going to be halted anyway.
@@ -491,8 +496,6 @@ int hibernation_platform_enter(void)
Platofrm_finish:
hibernation_ops->finish();
- device_power_up(PMSG_RESTORE);
-
Unlock:
device_pm_unlock();
On Sat, Apr 18, 2009 at 03:23:35PM +0200, Rafael J. Wysocki wrote:
> The patchset in question had been discussed quite extensively before it was
> merged and it's a pity none of the people caring for the affected platforms
> took part in those discussions. That would allow us to avoid the breakage.
Maybe on some list, but not everyone is subscribed to a million and one
mailing lists. I don't have enough time to read those which I'm currently
subscribed to, so I definitely don't want any more.
> > or provide alternative equivalent functionality where the platform code is
> > notified of the PM event prior to the late suspend callback being issued.
>
> There is the .begin() callback that could be used, but if you need the
> platform to be notified _just_ prior to the late suspend callback, then the
> only thing I can think of at the moment is the appended patch.
>
> It shouldn't break anything in theory, because the majority of drivers put
> their devices into low power states in the "regular" suspend callbacks anyway.
Okay, my requirement is:
What I need to be able to do is to suspend most devices on the host side
which may involve talking to a separate microcontroller via I2C to shut
down power to peripherals.
Once that's complete, I then need to inform this microcontroller via I2C
that we're going to be entering suspend mode, and wait for it to acknowledge
that (after it's done its own suspend preparations). At that point I can
shutdown the I2C controller, and enter suspend mode.
Upon resume (which is activated by this microcontroller, including jtagging
the boot code across to the host CPU), we need to tell this microcontroller
that we're going back to 'run' mode again via I2C, and then resume the
devices.
This is why we hooked the PXA I2C driver into the late suspend and
early resume methods, so the I2C driver would be the last to suspend and
the first to resume, thus allowing it to be used to talk to this micro-
controller when required. This worked out nicely because the late suspend
used to before the platform prepare and platform finish used to happen
after the early resume methods were called.
So, it looks like your patch to kernel/power/main.c will allow me to
continue to achieve this. (Don't know about the disk.c bits, I've
never used suspend-to-disk.)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Saturday 18 April 2009, Rafael J. Wysocki wrote:
> On Saturday 18 April 2009, Russell King wrote:
> > Some platforms need to talk via I2C to power control devices during
> > the suspend method. Currently, they do this via the platform PM ops
> > prepare callback, relying on the I2C driver being hooked into the
> > 'late' suspend method, and hence being shut down _after_ the prepare
> > callback.
>
> Well, I was not aware of this dependency and it seems quite unusual for a
> platform to depend on a driver like this.
>
> > However, as of the above commit, the ordering is changed such that
> > platforms don't get notified of suspends until after all devices are
> > well and truely shut down.
> >
> > This can't work, and actively breaks some platforms.
>
> Sorry for that.
>
> The patchset in question had been discussed quite extensively before it was
> merged and it's a pity none of the people caring for the affected platforms
> took part in those discussions. That would allow us to avoid the breakage.
>
> > Please come up with another solution for your PCI problems,
>
> I don't think this is possible, sorry.
>
> > or provide alternative equivalent functionality where the platform code is
> > notified of the PM event prior to the late suspend callback being issued.
>
> There is the .begin() callback that could be used, but if you need the
> platform to be notified _just_ prior to the late suspend callback, then the
> only thing I can think of at the moment is the appended patch.
>
> It shouldn't break anything in theory, because the majority of drivers put
> their devices into low power states in the "regular" suspend callbacks anyway.
Still, if it turns out to break anything, we'll have to think of an alternative
approach.
Which platforms exactly are affected?
Rafael
On Saturday 18 April 2009, Russell King wrote:
> On Sat, Apr 18, 2009 at 03:23:35PM +0200, Rafael J. Wysocki wrote:
> > The patchset in question had been discussed quite extensively before it was
> > merged and it's a pity none of the people caring for the affected platforms
> > took part in those discussions. That would allow us to avoid the breakage.
>
> Maybe on some list, but not everyone is subscribed to a million and one
> mailing lists. I don't have enough time to read those which I'm currently
> subscribed to, so I definitely don't want any more.
>
> > > or provide alternative equivalent functionality where the platform code is
> > > notified of the PM event prior to the late suspend callback being issued.
> >
> > There is the .begin() callback that could be used, but if you need the
> > platform to be notified _just_ prior to the late suspend callback, then the
> > only thing I can think of at the moment is the appended patch.
> >
> > It shouldn't break anything in theory, because the majority of drivers put
> > their devices into low power states in the "regular" suspend callbacks anyway.
>
> Okay, my requirement is:
>
> What I need to be able to do is to suspend most devices on the host side
> which may involve talking to a separate microcontroller via I2C to shut
> down power to peripherals.
>
> Once that's complete, I then need to inform this microcontroller via I2C
> that we're going to be entering suspend mode, and wait for it to acknowledge
> that (after it's done its own suspend preparations). At that point I can
> shutdown the I2C controller, and enter suspend mode.
Would it be an option to use a sysdev for that?
> Upon resume (which is activated by this microcontroller, including jtagging
> the boot code across to the host CPU), we need to tell this microcontroller
> that we're going back to 'run' mode again via I2C, and then resume the
> devices.
>
> This is why we hooked the PXA I2C driver into the late suspend and
> early resume methods, so the I2C driver would be the last to suspend and
> the first to resume, thus allowing it to be used to talk to this micro-
> controller when required. This worked out nicely because the late suspend
> used to before the platform prepare and platform finish used to happen
> after the early resume methods were called.
Unfortunately I'm not familiar with I2C, so I'm not sure whether or not this
would work, but it looks like sysdev could be used instead of platform_driver
for i2c_pxa_driver.
If using sysdev here (and analogously in i2c-s3c2410.c) is an option, I'd
prefer to do that instead of reordering suspend and resume code once again.
> So, it looks like your patch to kernel/power/main.c will allow me to
> continue to achieve this. (Don't know about the disk.c bits, I've
> never used suspend-to-disk.)
This patch undoes some previous changes, but I'm not too comfortable with it,
because I think that putting devices into low power states after .prepare() may
not work on some ACPI systems. Since devices should generally be put into low
power states during the "late" suspend (ie. when their interrupt handlers are
not invoked), it may be quite inconvenient.
Thanks,
Rafael
On Sat, Apr 18, 2009 at 03:59:18PM +0200, Rafael J. Wysocki wrote:
> On Saturday 18 April 2009, Rafael J. Wysocki wrote:
> > On Saturday 18 April 2009, Russell King wrote:
> > > Some platforms need to talk via I2C to power control devices during
> > > the suspend method. Currently, they do this via the platform PM ops
> > > prepare callback, relying on the I2C driver being hooked into the
> > > 'late' suspend method, and hence being shut down _after_ the prepare
> > > callback.
> >
> > Well, I was not aware of this dependency and it seems quite unusual for a
> > platform to depend on a driver like this.
> >
> > > However, as of the above commit, the ordering is changed such that
> > > platforms don't get notified of suspends until after all devices are
> > > well and truely shut down.
> > >
> > > This can't work, and actively breaks some platforms.
> >
> > Sorry for that.
> >
> > The patchset in question had been discussed quite extensively before it was
> > merged and it's a pity none of the people caring for the affected platforms
> > took part in those discussions. That would allow us to avoid the breakage.
> >
> > > Please come up with another solution for your PCI problems,
> >
> > I don't think this is possible, sorry.
> >
> > > or provide alternative equivalent functionality where the platform code is
> > > notified of the PM event prior to the late suspend callback being issued.
> >
> > There is the .begin() callback that could be used, but if you need the
> > platform to be notified _just_ prior to the late suspend callback, then the
> > only thing I can think of at the moment is the appended patch.
> >
> > It shouldn't break anything in theory, because the majority of drivers put
> > their devices into low power states in the "regular" suspend callbacks anyway.
>
> Still, if it turns out to break anything, we'll have to think of an alternative
> approach.
>
> Which platforms exactly are affected?
The one I've maintained since 2.6.9 and am just sending out the initial
bits for final review.
(It's taking a long time to get it out there because I can only spend
very little time on it, and it keeps getting broken - ALSA, SCSI and
now PM have all broken it in the last couple of months.)
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Sat, Apr 18, 2009 at 04:26:09PM +0200, Rafael J. Wysocki wrote:
> On Saturday 18 April 2009, Russell King wrote:
> > On Sat, Apr 18, 2009 at 03:23:35PM +0200, Rafael J. Wysocki wrote:
> > > The patchset in question had been discussed quite extensively before it was
> > > merged and it's a pity none of the people caring for the affected platforms
> > > took part in those discussions. That would allow us to avoid the breakage.
> >
> > Maybe on some list, but not everyone is subscribed to a million and one
> > mailing lists. I don't have enough time to read those which I'm currently
> > subscribed to, so I definitely don't want any more.
> >
> > > > or provide alternative equivalent functionality where the platform code is
> > > > notified of the PM event prior to the late suspend callback being issued.
> > >
> > > There is the .begin() callback that could be used, but if you need the
> > > platform to be notified _just_ prior to the late suspend callback, then the
> > > only thing I can think of at the moment is the appended patch.
> > >
> > > It shouldn't break anything in theory, because the majority of drivers put
> > > their devices into low power states in the "regular" suspend callbacks anyway.
> >
> > Okay, my requirement is:
> >
> > What I need to be able to do is to suspend most devices on the host side
> > which may involve talking to a separate microcontroller via I2C to shut
> > down power to peripherals.
> >
> > Once that's complete, I then need to inform this microcontroller via I2C
> > that we're going to be entering suspend mode, and wait for it to acknowledge
> > that (after it's done its own suspend preparations). At that point I can
> > shutdown the I2C controller, and enter suspend mode.
>
> Would it be an option to use a sysdev for that?
No, sysdevs are shut down after IRQs are turned off and the I2C driver
has been shut down. The I2C driver needs IRQs to work in slave mode,
and we need slave mode to work.
> This patch undoes some previous changes, but I'm not too comfortable with
> it, because I think that putting devices into low power states after
> .prepare() may not work on some ACPI systems. Since devices should
> generally be put into low power states during the "late" suspend (ie.
> when their interrupt handlers are not invoked), it may be quite
> inconvenient.
Maybe we need yet more levels of callbacks? :P
Realistically, not all platforms are going to have the same requirements,
so I don't think imposing ACPI requirements (by changing what is a
currently working suspend ordering) on everyone else is not the way
to go.
Maybe we need a way to allow hooks to be placed into the suspend/resume
mechanism in a dynamic way. Eg, define the suspend order as:
- early device suspend
- normal device suspend
- irqs off
- late device suspend
- sysdev suspend
and allow hooks into that order to be added. Maybe something like:
struct pm_hook {
struct list_head node;
unsigned int priority;
int (*suspend)(suspend_state_t);
int (*resume)(suspend_state_t);
};
static int early_device_suspend(suspend_state_t state)
{
return device_suspend(PMSG_SUSPEND);
}
static int early_device_resume(suspend_state_t state)
{
return device_resume(PMSG_RESUME);
}
static struct pm_hook early_device_hook = {
.priority = SUSP_EARLY_DEVICE,
.suspend = early_device_suspend,
.resume = early_device_resume,
};
int suspend(suspend_state_t state)
{
struct pm_hook *hook;
int err;
list_for_each(hook, &suspend_hooks, node) {
err = hook->suspend(state);
if (err) {
list_for_each_entry_continue_reverse(hook, &suspend_hooks, node)
hook->resume(state);
break;
}
}
return err;
}
where suspend_hooks is an ordered list according to 'priority'.
This would allow dynamic insertion of platforms suspend/resume requirements
into the PM system.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Saturday 18 April 2009, Rafael J. Wysocki wrote:
> On Saturday 18 April 2009, Russell King wrote:
> > On Sat, Apr 18, 2009 at 03:23:35PM +0200, Rafael J. Wysocki wrote:
> > > The patchset in question had been discussed quite extensively before it was
> > > merged and it's a pity none of the people caring for the affected platforms
> > > took part in those discussions. That would allow us to avoid the breakage.
> >
> > Maybe on some list, but not everyone is subscribed to a million and one
> > mailing lists. I don't have enough time to read those which I'm currently
> > subscribed to, so I definitely don't want any more.
> >
> > > > or provide alternative equivalent functionality where the platform code is
> > > > notified of the PM event prior to the late suspend callback being issued.
> > >
> > > There is the .begin() callback that could be used, but if you need the
> > > platform to be notified _just_ prior to the late suspend callback, then the
> > > only thing I can think of at the moment is the appended patch.
> > >
> > > It shouldn't break anything in theory, because the majority of drivers put
> > > their devices into low power states in the "regular" suspend callbacks anyway.
> >
> > Okay, my requirement is:
> >
> > What I need to be able to do is to suspend most devices on the host side
> > which may involve talking to a separate microcontroller via I2C to shut
> > down power to peripherals.
> >
> > Once that's complete, I then need to inform this microcontroller via I2C
> > that we're going to be entering suspend mode, and wait for it to acknowledge
> > that (after it's done its own suspend preparations). At that point I can
> > shutdown the I2C controller, and enter suspend mode.
>
> Would it be an option to use a sysdev for that?
>
> > Upon resume (which is activated by this microcontroller, including jtagging
> > the boot code across to the host CPU), we need to tell this microcontroller
> > that we're going back to 'run' mode again via I2C, and then resume the
> > devices.
> >
> > This is why we hooked the PXA I2C driver into the late suspend and
> > early resume methods, so the I2C driver would be the last to suspend and
> > the first to resume, thus allowing it to be used to talk to this micro-
> > controller when required. This worked out nicely because the late suspend
> > used to before the platform prepare and platform finish used to happen
> > after the early resume methods were called.
>
> Unfortunately I'm not familiar with I2C, so I'm not sure whether or not this
> would work, but it looks like sysdev could be used instead of platform_driver
> for i2c_pxa_driver.
>
> If using sysdev here (and analogously in i2c-s3c2410.c) is an option, I'd
> prefer to do that instead of reordering suspend and resume code once again.
Well, that wouldn't be straightforward, so I think I'll push my patch for
2.6.30 if Len doesn't object to it.
Thanks,
Rafael
On Saturday 18 April 2009, Russell King wrote:
> On Sat, Apr 18, 2009 at 04:26:09PM +0200, Rafael J. Wysocki wrote:
> > On Saturday 18 April 2009, Russell King wrote:
> > > On Sat, Apr 18, 2009 at 03:23:35PM +0200, Rafael J. Wysocki wrote:
> > > > The patchset in question had been discussed quite extensively before it was
> > > > merged and it's a pity none of the people caring for the affected platforms
> > > > took part in those discussions. That would allow us to avoid the breakage.
> > >
> > > Maybe on some list, but not everyone is subscribed to a million and one
> > > mailing lists. I don't have enough time to read those which I'm currently
> > > subscribed to, so I definitely don't want any more.
> > >
> > > > > or provide alternative equivalent functionality where the platform code is
> > > > > notified of the PM event prior to the late suspend callback being issued.
> > > >
> > > > There is the .begin() callback that could be used, but if you need the
> > > > platform to be notified _just_ prior to the late suspend callback, then the
> > > > only thing I can think of at the moment is the appended patch.
> > > >
> > > > It shouldn't break anything in theory, because the majority of drivers put
> > > > their devices into low power states in the "regular" suspend callbacks anyway.
> > >
> > > Okay, my requirement is:
> > >
> > > What I need to be able to do is to suspend most devices on the host side
> > > which may involve talking to a separate microcontroller via I2C to shut
> > > down power to peripherals.
> > >
> > > Once that's complete, I then need to inform this microcontroller via I2C
> > > that we're going to be entering suspend mode, and wait for it to acknowledge
> > > that (after it's done its own suspend preparations). At that point I can
> > > shutdown the I2C controller, and enter suspend mode.
> >
> > Would it be an option to use a sysdev for that?
>
> No, sysdevs are shut down after IRQs are turned off and the I2C driver
> has been shut down. The I2C driver needs IRQs to work in slave mode,
> and we need slave mode to work.
Hm, but up to and including 2.6.29 we called the late suspend callbacks with
interrupts off. Not any more, though. :-)
OK
> > This patch undoes some previous changes, but I'm not too comfortable with
> > it, because I think that putting devices into low power states after
> > .prepare() may not work on some ACPI systems. Since devices should
> > generally be put into low power states during the "late" suspend (ie.
> > when their interrupt handlers are not invoked), it may be quite
> > inconvenient.
>
> Maybe we need yet more levels of callbacks? :P
>
> Realistically, not all platforms are going to have the same requirements,
> so I don't think imposing ACPI requirements (by changing what is a
> currently working suspend ordering) on everyone else is not the way
> to go.
Well, we didn't have the problem before, because the platforms apparently
agreed with each other in that respect previously. :-)
I generally agree nevertheless.
> Maybe we need a way to allow hooks to be placed into the suspend/resume
> mechanism in a dynamic way. Eg, define the suspend order as:
>
> - early device suspend
> - normal device suspend
> - irqs off
> - late device suspend
> - sysdev suspend
That currently is
- normal device suspend
- suspend_device_irqs() (in kernel/irq/pm.c)
- late device suspend
- irqs off
- sysdev suspend
> and allow hooks into that order to be added. Maybe something like:
>
> struct pm_hook {
> struct list_head node;
> unsigned int priority;
> int (*suspend)(suspend_state_t);
> int (*resume)(suspend_state_t);
> };
>
> static int early_device_suspend(suspend_state_t state)
> {
> return device_suspend(PMSG_SUSPEND);
> }
>
> static int early_device_resume(suspend_state_t state)
> {
> return device_resume(PMSG_RESUME);
> }
>
> static struct pm_hook early_device_hook = {
> .priority = SUSP_EARLY_DEVICE,
> .suspend = early_device_suspend,
> .resume = early_device_resume,
> };
>
>
> int suspend(suspend_state_t state)
> {
> struct pm_hook *hook;
> int err;
>
> list_for_each(hook, &suspend_hooks, node) {
> err = hook->suspend(state);
> if (err) {
> list_for_each_entry_continue_reverse(hook, &suspend_hooks, node)
> hook->resume(state);
> break;
> }
> }
>
> return err;
> }
>
> where suspend_hooks is an ordered list according to 'priority'.
>
> This would allow dynamic insertion of platforms suspend/resume requirements
> into the PM system.
Hmm, what about redefining platform_suspend_ops in the following way:
struct platform_suspend_ops {
int (*valid)(suspend_state_t state);
int (*begin)(suspend_state_t state);
int (*devices_suspended)(void);
int (*prepare)(void);
int (*enter)(suspend_state_t state);
void (*wake)(void);
int (*resume_devices)(void);
void (*end)(void);
void (*recover)(void);
};
where:
* begin() will be called before suspending devices (no change)
* devices_suspended() will be called after suspending devices (before
suspend_device_irqs())
* prepare() will be callled after the late suspend callbacks
* enter() will be called to enter the sleep state (no change)
* wake() will be called before the early resume callbacks
* resume_devices() will be called before resuming devices (after
resume_device_irqs()
* end() will be called after resuming devices (no change)
I don't think we'll need more platform hooks than that.
Thanks,
Rafael
On Saturday 18 April 2009, Russell King wrote:
> On Sat, Apr 18, 2009 at 04:26:09PM +0200, Rafael J. Wysocki wrote:
> > On Saturday 18 April 2009, Russell King wrote:
> > > On Sat, Apr 18, 2009 at 03:23:35PM +0200, Rafael J. Wysocki wrote:
> > > > The patchset in question had been discussed quite extensively before it was
> > > > merged and it's a pity none of the people caring for the affected platforms
> > > > took part in those discussions. That would allow us to avoid the breakage.
> > >
> > > Maybe on some list, but not everyone is subscribed to a million and one
> > > mailing lists. I don't have enough time to read those which I'm currently
> > > subscribed to, so I definitely don't want any more.
> > >
> > > > > or provide alternative equivalent functionality where the platform code is
> > > > > notified of the PM event prior to the late suspend callback being issued.
> > > >
> > > > There is the .begin() callback that could be used, but if you need the
> > > > platform to be notified _just_ prior to the late suspend callback, then the
> > > > only thing I can think of at the moment is the appended patch.
> > > >
> > > > It shouldn't break anything in theory, because the majority of drivers put
> > > > their devices into low power states in the "regular" suspend callbacks anyway.
> > >
> > > Okay, my requirement is:
> > >
> > > What I need to be able to do is to suspend most devices on the host side
> > > which may involve talking to a separate microcontroller via I2C to shut
> > > down power to peripherals.
> > >
> > > Once that's complete, I then need to inform this microcontroller via I2C
> > > that we're going to be entering suspend mode, and wait for it to acknowledge
> > > that (after it's done its own suspend preparations). At that point I can
> > > shutdown the I2C controller, and enter suspend mode.
> >
> > Would it be an option to use a sysdev for that?
>
> No, sysdevs are shut down after IRQs are turned off and the I2C driver
> has been shut down. The I2C driver needs IRQs to work in slave mode,
> and we need slave mode to work.
>
> > This patch undoes some previous changes, but I'm not too comfortable with
> > it, because I think that putting devices into low power states after
> > .prepare() may not work on some ACPI systems. Since devices should
> > generally be put into low power states during the "late" suspend (ie.
> > when their interrupt handlers are not invoked), it may be quite
> > inconvenient.
>
> Maybe we need yet more levels of callbacks? :P
In fact, as a minimal fix that doesn't invalidate the testing of current Linus'
tree already done in this cycle, I'd like to apply the appended patch.
It adds two new platform suspend callbacks so that we can avoid the apparent
conflict between ACPI and the other platforms. Details are in the changelog.
If Len doesn't object, I'd like this one to go to 2.6.30, but I'm going to
clean up the platform suspend and hibernation callbacks eventually.
Thanks,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
Subject: PM/Suspend: Introduce two new platform callbacks to avoid breakage
Commit 900af0d973856d6feb6fc088c2d0d3fde57707d3 (PM: Change suspend
code ordering) changed the ordering of suspend code in such a way
that the platform .prepare() callback is now executed after the
device drivers' late suspend callbacks have run. Unfortunately, this
turns out to break ARM platforms that need to talk via I2C to power
control devices during the .prepare() callback.
For this reason introduce two new platform suspend callbacks,
.prepare_late() and .wake(), that will be called just prior to
disabling non-boot CPUs and right after bringing them back on line,
respectively, and use them instead of .prepare() and .finish() for
ACPI suspend. Make the PM core execute the .prepare() and .finish()
platform suspend callbacks where they were executed previously (that
is, right after calling the regular suspend methods provided by
device drivers and right before executing their regular resume
methods, respectively).
It is not necessary to make analogous changes to the hibernation
code and data structures at the moment, because they are only used
by ACPI platforms.
Signed-off-by: Rafael J. Wysocki <[email protected]>
Reported-by: Russell King <[email protected]>
---
drivers/acpi/sleep.c | 8 ++++----
include/linux/suspend.h | 36 ++++++++++++++++++++++++++----------
kernel/power/main.c | 24 +++++++++++++++++-------
3 files changed, 47 insertions(+), 21 deletions(-)
Index: linux-2.6/kernel/power/main.c
===================================================================
--- linux-2.6.orig/kernel/power/main.c
+++ linux-2.6/kernel/power/main.c
@@ -291,20 +291,26 @@ static int suspend_enter(suspend_state_t
device_pm_lock();
+ if (suspend_ops->prepare) {
+ error = suspend_ops->prepare();
+ if (error)
+ goto Done;
+ }
+
error = device_power_down(PMSG_SUSPEND);
if (error) {
printk(KERN_ERR "PM: Some devices failed to power down\n");
- goto Done;
+ goto Platfrom_finish;
}
- if (suspend_ops->prepare) {
- error = suspend_ops->prepare();
+ if (suspend_ops->prepare_late) {
+ error = suspend_ops->prepare_late();
if (error)
goto Power_up_devices;
}
if (suspend_test(TEST_PLATFORM))
- goto Platfrom_finish;
+ goto Platform_wake;
error = disable_nonboot_cpus();
if (error || suspend_test(TEST_CPUS))
@@ -326,13 +332,17 @@ static int suspend_enter(suspend_state_t
Enable_cpus:
enable_nonboot_cpus();
- Platfrom_finish:
- if (suspend_ops->finish)
- suspend_ops->finish();
+ Platform_wake:
+ if (suspend_ops->wake)
+ suspend_ops->wake();
Power_up_devices:
device_power_up(PMSG_RESUME);
+ Platfrom_finish:
+ if (suspend_ops->finish)
+ suspend_ops->finish();
+
Done:
device_pm_unlock();
Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -58,10 +58,17 @@ typedef int __bitwise suspend_state_t;
* by @begin().
* @prepare() is called right after devices have been suspended (ie. the
* appropriate .suspend() method has been executed for each device) and
- * before the nonboot CPUs are disabled (it is executed with IRQs enabled).
- * This callback is optional. It returns 0 on success or a negative
- * error code otherwise, in which case the system cannot enter the desired
- * sleep state (@enter() and @finish() will not be called in that case).
+ * before device drivers' late suspend callbacks are executed. It returns
+ * 0 on success or a negative error code otherwise, in which case the
+ * system cannot enter the desired sleep state (@prepare_late(), @enter(),
+ * @wake(), and @finish() will not be called in that case).
+ *
+ * @prepare_late: Finish preparing the platform for entering the system sleep
+ * state indicated by @begin().
+ * @prepare_late is called before disabling nonboot CPUs and after
+ * device drivers' late suspend callbacks have been executed. It returns
+ * 0 on success or a negative error code otherwise, in which case the
+ * system cannot enter the desired sleep state (@enter() and @wake()).
*
* @enter: Enter the system sleep state indicated by @begin() or represented by
* the argument if @begin() is not implemented.
@@ -69,19 +76,26 @@ typedef int __bitwise suspend_state_t;
* error code otherwise, in which case the system cannot enter the desired
* sleep state.
*
- * @finish: Called when the system has just left a sleep state, right after
- * the nonboot CPUs have been enabled and before devices are resumed (it is
- * executed with IRQs enabled).
+ * @wake: Called when the system has just left a sleep state, right after
+ * the nonboot CPUs have been enabled and before device drivers' early
+ * resume callbacks are executed.
+ * This callback is optional, but should be implemented by the platforms
+ * that implement @prepare_late(). If implemented, it is always called
+ * after @enter(), even if @enter() fails.
+ *
+ * @finish: Finish wake-up of the platform.
+ * @finish is called right prior to calling device drivers' regular suspend
+ * callbacks.
* This callback is optional, but should be implemented by the platforms
* that implement @prepare(). If implemented, it is always called after
- * @enter() (even if @enter() fails).
+ * @enter() and @wake(), if implemented, even if any of them fails.
*
* @end: Called by the PM core right after resuming devices, to indicate to
* the platform that the system has returned to the working state or
* the transition to the sleep state has been aborted.
* This callback is optional, but should be implemented by the platforms
- * that implement @begin(), but platforms implementing @begin() should
- * also provide a @end() which cleans up transitions aborted before
+ * that implement @begin(). Accordingly, platforms implementing @begin()
+ * should also provide a @end() which cleans up transitions aborted before
* @enter().
*
* @recover: Recover the platform from a suspend failure.
@@ -93,7 +107,9 @@ struct platform_suspend_ops {
int (*valid)(suspend_state_t state);
int (*begin)(suspend_state_t state);
int (*prepare)(void);
+ int (*prepare_late)(void);
int (*enter)(suspend_state_t state);
+ void (*wake)(void);
void (*finish)(void);
void (*end)(void);
void (*recover)(void);
Index: linux-2.6/drivers/acpi/sleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep.c
+++ linux-2.6/drivers/acpi/sleep.c
@@ -300,9 +300,9 @@ static int acpi_suspend_state_valid(susp
static struct platform_suspend_ops acpi_suspend_ops = {
.valid = acpi_suspend_state_valid,
.begin = acpi_suspend_begin,
- .prepare = acpi_pm_prepare,
+ .prepare_late = acpi_pm_prepare,
.enter = acpi_suspend_enter,
- .finish = acpi_pm_finish,
+ .wake = acpi_pm_finish,
.end = acpi_pm_end,
};
@@ -328,9 +328,9 @@ static int acpi_suspend_begin_old(suspen
static struct platform_suspend_ops acpi_suspend_ops_old = {
.valid = acpi_suspend_state_valid,
.begin = acpi_suspend_begin_old,
- .prepare = acpi_pm_disable_gpes,
+ .prepare_late = acpi_pm_disable_gpes,
.enter = acpi_suspend_enter,
- .finish = acpi_pm_finish,
+ .wake = acpi_pm_finish,
.end = acpi_pm_end,
.recover = acpi_pm_finish,
};
On Sat, 18 Apr 2009, Russell King wrote:
>
> What I need to be able to do is to suspend most devices on the host side
> which may involve talking to a separate microcontroller via I2C to shut
> down power to peripherals.
I suspect that for cases like this, the simplest thing to do is to just
add a marker for "don't mess with my power management, I'm doing
everything through sysdev" for the specified devices.
Then those i2c controllers (and perhaps some PCI bridges etc) can just set
that bit, and the device would basically turn invisible as far as the PM
layer is concerned.
Not that different from the IRQF_TIMER bit for timer interrupts.
Linus
On Saturday 18 April 2009, Linus Torvalds wrote:
>
> On Sat, 18 Apr 2009, Russell King wrote:
> >
> > What I need to be able to do is to suspend most devices on the host side
> > which may involve talking to a separate microcontroller via I2C to shut
> > down power to peripherals.
>
> I suspect that for cases like this, the simplest thing to do is to just
> add a marker for "don't mess with my power management, I'm doing
> everything through sysdev" for the specified devices.
In this particular case, if sysdev was used, there would be no problem, but the
platform uses a platform device to suspend-resume the i2c controller.
In principle we could convert it to use a sysdev, but that would be more
difficult than the last patch I sent IMO (at least to me).
Also, apparently it is not the only platform doing it.
> Then those i2c controllers (and perhaps some PCI bridges etc) can just set
> that bit, and the device would basically turn invisible as far as the PM
> layer is concerned.
>
> Not that different from the IRQF_TIMER bit for timer interrupts.
I generally agree and a patch to implemet such a flag has been submitted
recently.
Thanks,
Rafael
On Sat, Apr 18, 2009 at 05:09:46PM +0200, Rafael J. Wysocki wrote:
> On Saturday 18 April 2009, Russell King wrote:
> > No, sysdevs are shut down after IRQs are turned off and the I2C driver
> > has been shut down. The I2C driver needs IRQs to work in slave mode,
> > and we need slave mode to work.
>
> Hm, but up to and including 2.6.29 we called the late suspend callbacks with
> interrupts off.
That's not related to the problem - since all that the I2C drivers
late suspend callback does is shut the I2C driver down.
It's what happens between the normal suspend callback (which occurs
with IRQs on) and the late suspend callback which I'm interested in,
which is when the platform's 'prepare' method is called.
> Hmm, what about redefining platform_suspend_ops in the following way:
>
> struct platform_suspend_ops {
> int (*valid)(suspend_state_t state);
> int (*begin)(suspend_state_t state);
> int (*devices_suspended)(void);
> int (*prepare)(void);
> int (*enter)(suspend_state_t state);
> void (*wake)(void);
> int (*resume_devices)(void);
> void (*end)(void);
> void (*recover)(void);
> };
>
> where:
>
> * begin() will be called before suspending devices (no change)
> * devices_suspended() will be called after suspending devices (before
> suspend_device_irqs())
> * prepare() will be callled after the late suspend callbacks
> * enter() will be called to enter the sleep state (no change)
> * wake() will be called before the early resume callbacks
> * resume_devices() will be called before resuming devices (after
> resume_device_irqs()
> * end() will be called after resuming devices (no change)
>
> I don't think we'll need more platform hooks than that.
I'd be happy with that.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Sun, Apr 19, 2009 at 01:57:44PM -0400, Len Brown wrote:
> Acked-by: Len Brown <[email protected]>
Thanks. Since there's no objections yet, can we get this merged?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
Hi.
On Sat, 2009-04-18 at 20:47 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> Subject: PM/Suspend: Introduce two new platform callbacks to avoid breakage
>
> Commit 900af0d973856d6feb6fc088c2d0d3fde57707d3 (PM: Change suspend
> code ordering) changed the ordering of suspend code in such a way
> that the platform .prepare() callback is now executed after the
> device drivers' late suspend callbacks have run. Unfortunately, this
> turns out to break ARM platforms that need to talk via I2C to power
> control devices during the .prepare() callback.
>
> For this reason introduce two new platform suspend callbacks,
> .prepare_late() and .wake(), that will be called just prior to
> disabling non-boot CPUs and right after bringing them back on line,
> respectively, and use them instead of .prepare() and .finish() for
> ACPI suspend. Make the PM core execute the .prepare() and .finish()
> platform suspend callbacks where they were executed previously (that
> is, right after calling the regular suspend methods provided by
> device drivers and right before executing their regular resume
> methods, respectively).
>
> It is not necessary to make analogous changes to the hibernation
> code and data structures at the moment, because they are only used
> by ACPI platforms.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> Reported-by: Russell King <[email protected]>
> ---
> drivers/acpi/sleep.c | 8 ++++----
> include/linux/suspend.h | 36 ++++++++++++++++++++++++++----------
> kernel/power/main.c | 24 +++++++++++++++++-------
> 3 files changed, 47 insertions(+), 21 deletions(-)
>
> Index: linux-2.6/kernel/power/main.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/main.c
> +++ linux-2.6/kernel/power/main.c
> @@ -291,20 +291,26 @@ static int suspend_enter(suspend_state_t
>
> device_pm_lock();
>
> + if (suspend_ops->prepare) {
> + error = suspend_ops->prepare();
> + if (error)
> + goto Done;
> + }
> +
> error = device_power_down(PMSG_SUSPEND);
> if (error) {
> printk(KERN_ERR "PM: Some devices failed to power down\n");
> - goto Done;
> + goto Platfrom_finish;
s/Platfrom/Platform
Why retain the typo in multiple places?
> }
>
> - if (suspend_ops->prepare) {
> - error = suspend_ops->prepare();
> + if (suspend_ops->prepare_late) {
> + error = suspend_ops->prepare_late();
> if (error)
> goto Power_up_devices;
> }
Doesn't this invalidate testing that's already been done? Drivers
implementing prepare() (arm omap1, pxa, omap2, s3c and powerpc mpc52xx
and lite5200) are now going to have it called pre device_power_down. Why
not call the new prepare() "prepare_early" and leave the current prepare
as it is in the place where it's already called?
The name is also very confusing. Prepare matches with Finish and
Prepare_late with wake. How about prepare and unprepare?
Reviewed-by: Nigel Cunningham <[email protected]>
Regards,
Nigel
On Monday 20 April 2009, Nigel Cunningham wrote:
> Hi.
>
> On Sat, 2009-04-18 at 20:47 +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> > Subject: PM/Suspend: Introduce two new platform callbacks to avoid breakage
> >
> > Commit 900af0d973856d6feb6fc088c2d0d3fde57707d3 (PM: Change suspend
> > code ordering) changed the ordering of suspend code in such a way
> > that the platform .prepare() callback is now executed after the
> > device drivers' late suspend callbacks have run. Unfortunately, this
> > turns out to break ARM platforms that need to talk via I2C to power
> > control devices during the .prepare() callback.
> >
> > For this reason introduce two new platform suspend callbacks,
> > .prepare_late() and .wake(), that will be called just prior to
> > disabling non-boot CPUs and right after bringing them back on line,
> > respectively, and use them instead of .prepare() and .finish() for
> > ACPI suspend. Make the PM core execute the .prepare() and .finish()
> > platform suspend callbacks where they were executed previously (that
> > is, right after calling the regular suspend methods provided by
> > device drivers and right before executing their regular resume
> > methods, respectively).
> >
> > It is not necessary to make analogous changes to the hibernation
> > code and data structures at the moment, because they are only used
> > by ACPI platforms.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > Reported-by: Russell King <[email protected]>
> > ---
> > drivers/acpi/sleep.c | 8 ++++----
> > include/linux/suspend.h | 36 ++++++++++++++++++++++++++----------
> > kernel/power/main.c | 24 +++++++++++++++++-------
> > 3 files changed, 47 insertions(+), 21 deletions(-)
> >
> > Index: linux-2.6/kernel/power/main.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/power/main.c
> > +++ linux-2.6/kernel/power/main.c
> > @@ -291,20 +291,26 @@ static int suspend_enter(suspend_state_t
> >
> > device_pm_lock();
> >
> > + if (suspend_ops->prepare) {
> > + error = suspend_ops->prepare();
> > + if (error)
> > + goto Done;
> > + }
> > +
> > error = device_power_down(PMSG_SUSPEND);
> > if (error) {
> > printk(KERN_ERR "PM: Some devices failed to power down\n");
> > - goto Done;
> > + goto Platfrom_finish;
>
> s/Platfrom/Platform
>
> Why retain the typo in multiple places?
>
> > }
> >
> > - if (suspend_ops->prepare) {
> > - error = suspend_ops->prepare();
> > + if (suspend_ops->prepare_late) {
> > + error = suspend_ops->prepare_late();
> > if (error)
> > goto Power_up_devices;
> > }
>
> Doesn't this invalidate testing that's already been done? Drivers
> implementing prepare() (arm omap1, pxa, omap2, s3c and powerpc mpc52xx
> and lite5200) are now going to have it called pre device_power_down. Why
> not call the new prepare() "prepare_early" and leave the current prepare
> as it is in the place where it's already called?
Well, I wanted to avoid modifying platforms that were broken by the ordering
change.
> The name is also very confusing. Prepare matches with Finish and
> Prepare_late with wake. How about prepare and unprepare?
The names could be better as usual, but I'm going to revisit that shortly.
> Reviewed-by: Nigel Cunningham <[email protected]>
Thanks!
Best,
Rafael
On Sunday 19 April 2009, Russell King wrote:
> On Sun, Apr 19, 2009 at 01:57:44PM -0400, Len Brown wrote:
> > Acked-by: Len Brown <[email protected]>
>
> Thanks. Since there's no objections yet, can we get this merged?
I'm going to send a pull request in a minute.
Rafael
Hi Linus,
Please pull a power management update for 2.6.30 from:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git for-linus
This fixes the ARM breakage caused by the recent reordering of suspend code.
drivers/acpi/sleep.c | 8 ++++----
include/linux/suspend.h | 36 ++++++++++++++++++++++++++----------
kernel/power/main.c | 24 +++++++++++++++++-------
3 files changed, 47 insertions(+), 21 deletions(-)
---------------
Rafael J. Wysocki (1):
PM/Suspend: Introduce two new platform callbacks to avoid breakage
Thanks,
Rafael
On Mon, Apr 20, 2009 at 09:31:47AM +1000, Nigel Cunningham wrote:
> > }
> >
> > - if (suspend_ops->prepare) {
> > - error = suspend_ops->prepare();
> > + if (suspend_ops->prepare_late) {
> > + error = suspend_ops->prepare_late();
> > if (error)
> > goto Power_up_devices;
> > }
>
> Doesn't this invalidate testing that's already been done? Drivers
> implementing prepare() (arm omap1, pxa, omap2, s3c and powerpc mpc52xx
> and lite5200) are now going to have it called pre device_power_down.
... which is the way it has always been done prior to the change which
broke stuff. So this patch is putting the ordering back the way it was
which has been well proven to work on these platforms.
So we're not invalidating any testing.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
Hi.
On Mon, 2009-04-20 at 09:35 +0100, Russell King wrote:
> On Mon, Apr 20, 2009 at 09:31:47AM +1000, Nigel Cunningham wrote:
> > > }
> > >
> > > - if (suspend_ops->prepare) {
> > > - error = suspend_ops->prepare();
> > > + if (suspend_ops->prepare_late) {
> > > + error = suspend_ops->prepare_late();
> > > if (error)
> > > goto Power_up_devices;
> > > }
> >
> > Doesn't this invalidate testing that's already been done? Drivers
> > implementing prepare() (arm omap1, pxa, omap2, s3c and powerpc mpc52xx
> > and lite5200) are now going to have it called pre device_power_down.
>
> ... which is the way it has always been done prior to the change which
> broke stuff. So this patch is putting the ordering back the way it was
> which has been well proven to work on these platforms.
>
> So we're not invalidating any testing.
Ah, okay. I didn't look at anything but current HEAD. :)
Nigel
On Sun 2009-04-19 00:44:57, Rafael J. Wysocki wrote:
> On Saturday 18 April 2009, Linus Torvalds wrote:
> >
> > On Sat, 18 Apr 2009, Russell King wrote:
> > >
> > > What I need to be able to do is to suspend most devices on the host side
> > > which may involve talking to a separate microcontroller via I2C to shut
> > > down power to peripherals.
> >
> > I suspect that for cases like this, the simplest thing to do is to just
> > add a marker for "don't mess with my power management, I'm doing
> > everything through sysdev" for the specified devices.
>
> In this particular case, if sysdev was used, there would be no problem, but the
> platform uses a platform device to suspend-resume the i2c controller.
> In principle we could convert it to use a sysdev, but that would be more
> difficult than the last patch I sent IMO (at least to me).
>
> Also, apparently it is not the only platform doing it.
OTOH... if the device is needed in late phases of sleep, sysdev *does*
sound like a way to go. So it would be nice to convert those devices
to sysdevs one day.....
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html