| Message ID | 1305608346-23800-2-git-send-email-myungjoo.ham@samsung.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Tuesday, May 17, 2011, MyungJoo Ham wrote: > The API, suspend_again, is supposed to be used by platforms when the > platforms want to execute some code while suspended (wakeup by an alarm > or periodic ticks, run code, suspend again). Because suspend_again is > not called when every device is resumed, but is called right after > suspend_enter() is called, the suspend_again callback should include > device resume and suspend features. > > This patch provides two APIs: dpm_partial_resume and > dpm_partial_suspend. They are supposed to be used in suspend_again to > actiavte required devices. > > A usage example is: > > /* Devices required to run "monitor_something()". */ > static device *devs[] = { > &console_serial, > &device_x, > &device_y, > ... > }; > > bool example_suspend_again(void) > { > int error, monitor_result; > > if (!wakeup_reason_is_polling()) > return false; > > dpm_partial_resume(PMSG_RESUME, devs, ARRAY_SIZE(devs)); I'm not sure you need the first argument here. Also, if the array is NULL terminated, you won't need the third one. > resume_console(); > > monitor_result = monitor_something(); > > suspend_console(); > error = dpm_partial_suspend(PMSG_SUSPEND, devs, ARRAY_SIZE(devs)); Same here. > if (error || monitor_result == ERROR) > return false; > > return true; > } That said, I don't like this patch. The reason is that you call suspend_enter() is a loop and it calls the _noirq() callbacks for _all_ devices and now you're going to only call .resume() and .suspend() for several selected devices. This is not too consistent. I wonder if those devices needed for .suspend_again() to work on your platform could be resumed and suspended by their drivers' _noirq() callbacks? Rafael > Tested at Exynos4-NURI. > > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > -- > No changes from v3. > --- > drivers/base/power/main.c | 154 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm.h | 4 + > 2 files changed, 158 insertions(+), 0 deletions(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 052dc53..71dc693 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -1082,3 +1082,157 @@ int device_pm_wait_for_dev(struct device *subordinate, struct device *dev) > return async_error; > } > EXPORT_SYMBOL_GPL(device_pm_wait_for_dev); > + > +/** > + * __dpm_partial_resume - Execute "resume" callbacks for the listed devices. > + * @state: PM transition being carried out. > + * @devs: Array of pointers for the devices being resumed > + * @size: The size of devs array. > + */ > +static void __dpm_partial_resume(pm_message_t state, struct device **devs, > + int size) > +{ > + int i; > + struct device *dev; > + > + for (i = 0; i < size; i++) { > + int error; > + > + dev = devs[i]; > + get_device(dev); > + > + error = device_resume(dev, state, false); > + if (error) > + pm_dev_err(dev, state, "", error); > + > + put_device(dev); > + } > +} > + > +/** > + * __dpm_partial_complete - Complete a PM transition for the listed devices. > + * @state: PM transition being carried out. > + * @devs: Array of pointers for the devices being resumed > + * @size: The size of devs array. > + */ > +static void __dpm_partial_complete(pm_message_t state, struct device **devs, > + int size) > +{ > + int i; > + struct device *dev; > + > + for (i = 0; i < size; i++) { > + dev = devs[i]; > + > + get_device(dev); > + dev->power.in_suspend = false; > + > + device_complete(dev, state); > + > + put_device(dev); > + } > +} > + > +/** > + * dpm_partial_resume - Execute "resume" callbacks and complete system > + * transaction for the chosen devices only. > + * @state: PM transition being carried out. > + * @devs: Array of pointers for the devices being resumed > + * @size: The size of devs array. > + * > + * Execute "resume" callbacks for the listed devices and complete the PM > + * transition of them. > + * > + * Because the only a part of devices will be resumed, asynchronous resume > + * is dropped. > + */ > +void dpm_partial_resume(pm_message_t state, struct device **devs, int size) > +{ > + /* Partial dpm_resume */ > + __dpm_partial_resume(state, devs, size); > + > + /* Partial dpm_complete */ > + __dpm_partial_complete(state, devs, size); > +} > +EXPORT_SYMBOL_GPL(dpm_partial_resume); > + > +/** > + * dpm_partial_suspend - Prepare the given devices for PM transition and > + * suspend them. > + * @state: PM transition being carried out. > + * @devs: Array of pointers for the devices being suspended > + * @size: The size of devs array. > + * > + * Prepare the given devices for system PM transition and execute "suspend" > + * callbacks for them. > + * > + * The devs array is iterated in the reversed order to use the same devs > + * array with dpm_partial_resume(). > + */ > +int dpm_partial_suspend(pm_message_t state, struct device **devs, int size) > +{ > + int error = 0, i; > + struct device *dev; > + > + /* Partial dpm_prepare */ > + for (i = size - 1; i >= 0; i--) { > + dev = devs[i]; > + > + get_device(dev); > + > + pm_runtime_get_noresume(dev); > + if (pm_runtime_barrier(dev) && device_may_wakeup(dev)) > + pm_wakeup_event(dev, 0); > + > + pm_runtime_put_sync(dev); > + error = pm_wakeup_pending() ? > + -EBUSY : device_prepare(dev, state); > + > + if (error) { > + if (error == -EAGAIN) { > + put_device(dev); > + error = 0; > + i++; /* Try Again */ > + continue; > + } > + printk(KERN_INFO "PM: Device %s not prepared " > + "for power transition: code %d\n", > + dev_name(dev), error); > + put_device(dev); > + break; > + } > + dev->power.in_suspend = true; > + put_device(dev); > + } > + > + if (error) > + goto err_prepare; > + > + /* Partial dpm_suspend */ > + for (i = size - 1; i >= 0; i--) { > + dev = devs[i]; > + > + get_device(dev); > + > + /* Synchronous suspend. The list shouldn't be long */ > + error = __device_suspend(dev, pm_transition, false); > + > + if (error) { > + pm_dev_err(dev, state, "", error); > + put_device(dev); > + break; > + } > + put_device(dev); > + } > + > + if (!error) > + return 0; > + > + __dpm_partial_resume(PMSG_RESUME, devs + i + 1, size - i - 1); > + i = -1; > +err_prepare: > + __dpm_partial_complete(PMSG_RESUME, devs + i + 1, size - i - 1); > + > + return error; > +} > +EXPORT_SYMBOL_GPL(dpm_partial_suspend); > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 512e091..b407762 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -540,10 +540,14 @@ static inline int sysdev_resume(void) { return 0; } > extern void device_pm_lock(void); > extern void dpm_resume_noirq(pm_message_t state); > extern void dpm_resume_end(pm_message_t state); > +extern void dpm_partial_resume(pm_message_t state, struct device **devs, > + int size); > > 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_partial_suspend(pm_message_t state, struct device **devs, > + int size); > > extern void __suspend_report_result(const char *function, void *fn, int ret); > >
On Wednesday, May 18, 2011, MyungJoo Ham wrote: > 2011/5/18 Rafael J. Wysocki <rjw@sisk.pl>: > > On Tuesday, May 17, 2011, MyungJoo Ham wrote: > >> A usage example is: > >> > >> bool example_suspend_again(void) > >> { > >> int error, monitor_result; > >> > >> if (!wakeup_reason_is_polling()) > >> return false; > >> > >> dpm_partial_resume(PMSG_RESUME, devs, ARRAY_SIZE(devs)); > > > > I'm not sure you need the first argument here. Also, if the array is > > NULL terminated, you won't need the third one. > > > > It was to pass the parameter to device_resume(), pm_dev_err(), > device_complete(), and device_prepare(), which have been using the > state value. However, as it is supposed to be used in the context of > suspend_devices_and_enter(), I'll remove it and assume PMSG_SUSPEND > and PMSG_RESUME are in effect. > > For the array style, do we generally prefer to use NULL at the end > rather than supplying the size? If so, I'll change the array style. > > >> resume_console(); > >> > >> monitor_result = monitor_something(); > >> > >> suspend_console(); > >> error = dpm_partial_suspend(PMSG_SUSPEND, devs, ARRAY_SIZE(devs)); > > > > Same here. > > > >> if (error || monitor_result == ERROR) > >> return false; > >> > >> return true; > >> } > > > > That said, I don't like this patch. The reason is that you call > > suspend_enter() is a loop and it calls the _noirq() callbacks for > > _all_ devices and now you're going to only call .resume() and > > .suspend() for several selected devices. This is not too consistent. > > > > I wonder if those devices needed for .suspend_again() to work on > > your platform could be resumed and suspended by their drivers' > > _noirq() callbacks? > > For now, we need to access PMIC, Fuel-Gauge, Charger (implemented as > regulators), RTC, ADC (w/ hwmon), and UART(drivers/tty/serial). > For PMIC, Fuel-Gauge, Charger, and RTC, they will work with no_irq > callbacks without any modification. > ADC will work if we just let it suspend and resume with no_irq callbacks. > > However, for UART/serial, although it is only used for debugging, it > wouldn't be easy and clean to enable only with no_irq callbacks. If we > can keep observing the console with suspend_console_enabled=0, it > would be much helpful. > In order to let some UART/serial drivers work with no-irq callbacks, > we'd need reconstruction in drivers/tty/serial/serial_core.c, too. > > Anyway, as the effect of not having partial suspend/resume is limited > for now (support for UART/serial can wait), I may defer this part and > wait for better and clean methods. And yes, because of the possibility > of having dependencies between devices, the inconsistency you've > mentioned could be an issue for some systems. However, because such > dependencies are not explicitly expressed to kernel while the platform > should know about them, partial_suspend/resume was supposed to be > called by the platform's suspend_again ops. This is a difficult problem in general. One solution of it I had been considering some time ago might be to use two or more "suspend lists" of devices instead of the single dpm_list we have right now. Namely, suppose we have an array of lists that are idexed by priority such that devices in the list of priority 0 are suspended first, devices in the list of priority 1 are suspended next and so on (the oredering of resume will have to be reversed). Then, your new code may request to only resume the highest pririty list and then call .suspend_again() and possiblty continue with normal resume if it returns "false". [Please note that you need to call .resume_noirq() for all devices anyway, because device interrupts cannot be enabled before calling any drivers' .resume_noirq() in case .suspend_again() returns "false".] Devices will be added to the priority 0 list during registration and there will be a special function to move a device to another list (increase its suspend pririty) that will ensure the requisite dependencies are met (i.e. the suspend priority of a device cannot be greater than the suspend priority of its parent, so that the parent would be suspended first). Thanks, Rafael
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 052dc53..71dc693 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1082,3 +1082,157 @@ int device_pm_wait_for_dev(struct device *subordinate, struct device *dev) return async_error; } EXPORT_SYMBOL_GPL(device_pm_wait_for_dev); + +/** + * __dpm_partial_resume - Execute "resume" callbacks for the listed devices. + * @state: PM transition being carried out. + * @devs: Array of pointers for the devices being resumed + * @size: The size of devs array. + */ +static void __dpm_partial_resume(pm_message_t state, struct device **devs, + int size) +{ + int i; + struct device *dev; + + for (i = 0; i < size; i++) { + int error; + + dev = devs[i]; + get_device(dev); + + error = device_resume(dev, state, false); + if (error) + pm_dev_err(dev, state, "", error); + + put_device(dev); + } +} + +/** + * __dpm_partial_complete - Complete a PM transition for the listed devices. + * @state: PM transition being carried out. + * @devs: Array of pointers for the devices being resumed + * @size: The size of devs array. + */ +static void __dpm_partial_complete(pm_message_t state, struct device **devs, + int size) +{ + int i; + struct device *dev; + + for (i = 0; i < size; i++) { + dev = devs[i]; + + get_device(dev); + dev->power.in_suspend = false; + + device_complete(dev, state); + + put_device(dev); + } +} + +/** + * dpm_partial_resume - Execute "resume" callbacks and complete system + * transaction for the chosen devices only. + * @state: PM transition being carried out. + * @devs: Array of pointers for the devices being resumed + * @size: The size of devs array. + * + * Execute "resume" callbacks for the listed devices and complete the PM + * transition of them. + * + * Because the only a part of devices will be resumed, asynchronous resume + * is dropped. + */ +void dpm_partial_resume(pm_message_t state, struct device **devs, int size) +{ + /* Partial dpm_resume */ + __dpm_partial_resume(state, devs, size); + + /* Partial dpm_complete */ + __dpm_partial_complete(state, devs, size); +} +EXPORT_SYMBOL_GPL(dpm_partial_resume); + +/** + * dpm_partial_suspend - Prepare the given devices for PM transition and + * suspend them. + * @state: PM transition being carried out. + * @devs: Array of pointers for the devices being suspended + * @size: The size of devs array. + * + * Prepare the given devices for system PM transition and execute "suspend" + * callbacks for them. + * + * The devs array is iterated in the reversed order to use the same devs + * array with dpm_partial_resume(). + */ +int dpm_partial_suspend(pm_message_t state, struct device **devs, int size) +{ + int error = 0, i; + struct device *dev; + + /* Partial dpm_prepare */ + for (i = size - 1; i >= 0; i--) { + dev = devs[i]; + + get_device(dev); + + pm_runtime_get_noresume(dev); + if (pm_runtime_barrier(dev) && device_may_wakeup(dev)) + pm_wakeup_event(dev, 0); + + pm_runtime_put_sync(dev); + error = pm_wakeup_pending() ? + -EBUSY : device_prepare(dev, state); + + if (error) { + if (error == -EAGAIN) { + put_device(dev); + error = 0; + i++; /* Try Again */ + continue; + } + printk(KERN_INFO "PM: Device %s not prepared " + "for power transition: code %d\n", + dev_name(dev), error); + put_device(dev); + break; + } + dev->power.in_suspend = true; + put_device(dev); + } + + if (error) + goto err_prepare; + + /* Partial dpm_suspend */ + for (i = size - 1; i >= 0; i--) { + dev = devs[i]; + + get_device(dev); + + /* Synchronous suspend. The list shouldn't be long */ + error = __device_suspend(dev, pm_transition, false); + + if (error) { + pm_dev_err(dev, state, "", error); + put_device(dev); + break; + } + put_device(dev); + } + + if (!error) + return 0; + + __dpm_partial_resume(PMSG_RESUME, devs + i + 1, size - i - 1); + i = -1; +err_prepare: + __dpm_partial_complete(PMSG_RESUME, devs + i + 1, size - i - 1); + + return error; +} +EXPORT_SYMBOL_GPL(dpm_partial_suspend); diff --git a/include/linux/pm.h b/include/linux/pm.h index 512e091..b407762 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -540,10 +540,14 @@ static inline int sysdev_resume(void) { return 0; } extern void device_pm_lock(void); extern void dpm_resume_noirq(pm_message_t state); extern void dpm_resume_end(pm_message_t state); +extern void dpm_partial_resume(pm_message_t state, struct device **devs, + int size); 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_partial_suspend(pm_message_t state, struct device **devs, + int size); extern void __suspend_report_result(const char *function, void *fn, int ret);
The API, suspend_again, is supposed to be used by platforms when the platforms want to execute some code while suspended (wakeup by an alarm or periodic ticks, run code, suspend again). Because suspend_again is not called when every device is resumed, but is called right after suspend_enter() is called, the suspend_again callback should include device resume and suspend features. This patch provides two APIs: dpm_partial_resume and dpm_partial_suspend. They are supposed to be used in suspend_again to actiavte required devices. A usage example is: /* Devices required to run "monitor_something()". */ static device *devs[] = { &console_serial, &device_x, &device_y, ... }; bool example_suspend_again(void) { int error, monitor_result; if (!wakeup_reason_is_polling()) return false; dpm_partial_resume(PMSG_RESUME, devs, ARRAY_SIZE(devs)); resume_console(); monitor_result = monitor_something(); suspend_console(); error = dpm_partial_suspend(PMSG_SUSPEND, devs, ARRAY_SIZE(devs)); if (error || monitor_result == ERROR) return false; return true; } Tested at Exynos4-NURI. Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> -- No changes from v3. --- drivers/base/power/main.c | 154 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/pm.h | 4 + 2 files changed, 158 insertions(+), 0 deletions(-)