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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
