[RFC PATCH] pm: prevent suspend until power supply events are processed

[RFC PATCH] pm: prevent suspend until power supply events are processed

[Zoran Markovic]

This patch, originally authored by Arve Hjonnevag and Todd Poynor,
prevents the system from entering suspend mode until the power
supply plug, unplug, or any other change of state event is fully
processed. This guarantees that the screen lights up and displays
the battery charging state. The implementation uses the power
supply wakeup_source object.

Cc: Anton Vorontsov <anton@enomsg.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Arve Hjonnevag <arve@android.com>
Cc: Todd Poynor <toddpoynor@google.com>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
---
 drivers/power/power_supply_core.c |   37 +++++++++++++++++++++++++++++++------
 include/linux/power_supply.h      |    2 ++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 3b2d5df..e68d598 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
 
 static void power_supply_changed_work(struct work_struct *work)
 {
+	unsigned long flags;
 	struct power_supply *psy = container_of(work, struct power_supply,
 						changed_work);
 
 	dev_dbg(psy->dev, "%s\n", __func__);
 
-	class_for_each_device(power_supply_class, NULL, psy,
-			      __power_supply_changed_work);
-
-	power_supply_update_leds(psy);
-
-	kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
+	spin_lock_irqsave(&psy->changed_lock, flags);
+	if (psy->changed) {
+		psy->changed = false;
+		spin_unlock_irqrestore(&psy->changed_lock, flags);
+		class_for_each_device(power_supply_class, NULL, psy,
+				      __power_supply_changed_work);
+		power_supply_update_leds(psy);
+		kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
+		spin_lock_irqsave(&psy->changed_lock, flags);
+	}
+	/* dependent power supplies (e.g. battery) may have changed
+	 * state as a result of this event, so poll again and hold
+	 * the wakeup_source until all events are processed.
+	 */
+	if (!psy->changed)
+		pm_relax(psy->dev);
+	spin_unlock_irqrestore(&psy->changed_lock, flags);
 }
 
 void power_supply_changed(struct power_supply *psy)
 {
+	unsigned long flags;
+
 	dev_dbg(psy->dev, "%s\n", __func__);
 
+	spin_lock_irqsave(&psy->changed_lock, flags);
+	psy->changed = true;
+	pm_stay_awake(psy->dev);
+	spin_unlock_irqrestore(&psy->changed_lock, flags);
 	schedule_work(&psy->changed_work);
 }
 EXPORT_SYMBOL_GPL(power_supply_changed);
@@ -500,6 +518,11 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 		goto check_supplies_failed;
 	}
 
+	spin_lock_init(&psy->changed_lock);
+	rc = device_init_wakeup(dev, true);
+	if (rc)
+		goto wakeup_init_failed;
+
 	rc = kobject_set_name(&dev->kobj, "%s", psy->name);
 	if (rc)
 		goto kobject_set_name_failed;
@@ -529,6 +552,7 @@ create_triggers_failed:
 register_cooler_failed:
 	psy_unregister_thermal(psy);
 register_thermal_failed:
+wakeup_init_failed:
 	device_del(dev);
 kobject_set_name_failed:
 device_add_failed:
@@ -546,6 +570,7 @@ void power_supply_unregister(struct power_supply *psy)
 	power_supply_remove_triggers(psy);
 	psy_unregister_cooler(psy);
 	psy_unregister_thermal(psy);
+	device_init_wakeup(psy->dev, false);
 	device_unregister(psy->dev);
 }
 EXPORT_SYMBOL_GPL(power_supply_unregister);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 804b906..253d412 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -194,6 +194,8 @@ struct power_supply {
 	/* private */
 	struct device *dev;
 	struct work_struct changed_work;
+	spinlock_t changed_lock;
+	bool changed;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
 	struct thermal_cooling_device *tcd;
-- 
1.7.9.5

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Zoran Markovic]

Any opinions on this patch?
Regards, Zoran

On 2 August 2013 13:38, Zoran Markovic <zoran.markovic@linaro.org> wrote:
> This patch, originally authored by Arve Hjonnevag and Todd Poynor,
> prevents the system from entering suspend mode until the power
> supply plug, unplug, or any other change of state event is fully
> processed. This guarantees that the screen lights up and displays
> the battery charging state. The implementation uses the power
> supply wakeup_source object.
>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Arve Hjonnevag <arve@android.com>
> Cc: Todd Poynor <toddpoynor@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
> ---
>  drivers/power/power_supply_core.c |   37 +++++++++++++++++++++++++++++++------
>  include/linux/power_supply.h      |    2 ++
>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 3b2d5df..e68d598 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>
>  static void power_supply_changed_work(struct work_struct *work)
>  {
> +       unsigned long flags;
>         struct power_supply *psy = container_of(work, struct power_supply,
>                                                 changed_work);
>
>         dev_dbg(psy->dev, "%s\n", __func__);
>
> -       class_for_each_device(power_supply_class, NULL, psy,
> -                             __power_supply_changed_work);
> -
> -       power_supply_update_leds(psy);
> -
> -       kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> +       spin_lock_irqsave(&psy->changed_lock, flags);
> +       if (psy->changed) {
> +               psy->changed = false;
> +               spin_unlock_irqrestore(&psy->changed_lock, flags);
> +               class_for_each_device(power_supply_class, NULL, psy,
> +                                     __power_supply_changed_work);
> +               power_supply_update_leds(psy);
> +               kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> +               spin_lock_irqsave(&psy->changed_lock, flags);
> +       }
> +       /* dependent power supplies (e.g. battery) may have changed
> +        * state as a result of this event, so poll again and hold
> +        * the wakeup_source until all events are processed.
> +        */
> +       if (!psy->changed)
> +               pm_relax(psy->dev);
> +       spin_unlock_irqrestore(&psy->changed_lock, flags);
>  }
>
>  void power_supply_changed(struct power_supply *psy)
>  {
> +       unsigned long flags;
> +
>         dev_dbg(psy->dev, "%s\n", __func__);
>
> +       spin_lock_irqsave(&psy->changed_lock, flags);
> +       psy->changed = true;
> +       pm_stay_awake(psy->dev);
> +       spin_unlock_irqrestore(&psy->changed_lock, flags);
>         schedule_work(&psy->changed_work);
>  }
>  EXPORT_SYMBOL_GPL(power_supply_changed);
> @@ -500,6 +518,11 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
>                 goto check_supplies_failed;
>         }
>
> +       spin_lock_init(&psy->changed_lock);
> +       rc = device_init_wakeup(dev, true);
> +       if (rc)
> +               goto wakeup_init_failed;
> +
>         rc = kobject_set_name(&dev->kobj, "%s", psy->name);
>         if (rc)
>                 goto kobject_set_name_failed;
> @@ -529,6 +552,7 @@ create_triggers_failed:
>  register_cooler_failed:
>         psy_unregister_thermal(psy);
>  register_thermal_failed:
> +wakeup_init_failed:
>         device_del(dev);
>  kobject_set_name_failed:
>  device_add_failed:
> @@ -546,6 +570,7 @@ void power_supply_unregister(struct power_supply *psy)
>         power_supply_remove_triggers(psy);
>         psy_unregister_cooler(psy);
>         psy_unregister_thermal(psy);
> +       device_init_wakeup(psy->dev, false);
>         device_unregister(psy->dev);
>  }
>  EXPORT_SYMBOL_GPL(power_supply_unregister);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 804b906..253d412 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -194,6 +194,8 @@ struct power_supply {
>         /* private */
>         struct device *dev;
>         struct work_struct changed_work;
> +       spinlock_t changed_lock;
> +       bool changed;
>  #ifdef CONFIG_THERMAL
>         struct thermal_zone_device *tzd;
>         struct thermal_cooling_device *tcd;
> --
> 1.7.9.5
>

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Anton Vorontsov]

On Fri, Aug 02, 2013 at 01:38:02PM -0700, Zoran Markovic wrote:
> This patch, originally authored by Arve Hjonnevag and Todd Poynor,
> prevents the system from entering suspend mode until the power
> supply plug, unplug, or any other change of state event is fully
> processed. This guarantees that the screen lights up and displays
> the battery charging state. The implementation uses the power
> supply wakeup_source object.
> 
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Arve Hjonnevag <arve@android.com>
> Cc: Todd Poynor <toddpoynor@google.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
> ---
...
> +		kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> +		spin_lock_irqsave(&psy->changed_lock, flags);
> +	}
> +	/* dependent power supplies (e.g. battery) may have changed

Multi-line comments style issue...

> +	 * state as a result of this event, so poll again and hold
> +	 * the wakeup_source until all events are processed.
> +	 */
> +	if (!psy->changed)
> +		pm_relax(psy->dev);
...
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 804b906..253d412 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -194,6 +194,8 @@ struct power_supply {
>  	/* private */
>  	struct device *dev;
>  	struct work_struct changed_work;
> +	spinlock_t changed_lock;

#include <linux/spinlock.h> is needed.

I fixed it up and applied the patch, thanks a lot!

Anton

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Viresh Kumar]

Don't have Zoran's new email address, but probably other might have
answers to my queries.

I have just started with the power-supply framework a day or two back
and so my understanding might not be good enough :)

On Sat, Aug 3, 2013 at 2:08 AM, Zoran Markovic
<zoran.markovic@linaro.org> wrote:
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 3b2d5df..e68d598 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>
>  static void power_supply_changed_work(struct work_struct *work)
>  {
> +       unsigned long flags;
>         struct power_supply *psy = container_of(work, struct power_supply,
>                                                 changed_work);
>
>         dev_dbg(psy->dev, "%s\n", __func__);
>
> -       class_for_each_device(power_supply_class, NULL, psy,
> -                             __power_supply_changed_work);
> -
> -       power_supply_update_leds(psy);
> -
> -       kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> +       spin_lock_irqsave(&psy->changed_lock, flags);
> +       if (psy->changed) {

Can this be false here? We have reached here as the work was
scheduled after setting it to true..

Maybe a WARN_ON(psy->changed) is more sensible here ?

> +               psy->changed = false;
> +               spin_unlock_irqrestore(&psy->changed_lock, flags);
> +               class_for_each_device(power_supply_class, NULL, psy,
> +                                     __power_supply_changed_work);
> +               power_supply_update_leds(psy);
> +               kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
> +               spin_lock_irqsave(&psy->changed_lock, flags);
> +       }
> +       /* dependent power supplies (e.g. battery) may have changed
> +        * state as a result of this event, so poll again and hold
> +        * the wakeup_source until all events are processed.
> +        */
> +       if (!psy->changed)
> +               pm_relax(psy->dev);

I got a bit confused here. Does the above comment say this:

The supplies dependent on 'psy' may change states and that *may*
change the state of 'psy' again? And so psy->changed is set to true
again?

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Viresh Kumar]

On Wed, Sep 3, 2014 at 10:09 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Don't have Zoran's new email address, but probably other might have
> answers to my queries.

Got Zoran's email id finally :)

> I have just started with the power-supply framework a day or two back
> and so my understanding might not be good enough :)
>
> On Sat, Aug 3, 2013 at 2:08 AM, Zoran Markovic
> <zoran.markovic@linaro.org> wrote:
>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>> index 3b2d5df..e68d598 100644
>> --- a/drivers/power/power_supply_core.c
>> +++ b/drivers/power/power_supply_core.c
>> @@ -67,23 +67,41 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>>
>>  static void power_supply_changed_work(struct work_struct *work)
>>  {
>> +       unsigned long flags;
>>         struct power_supply *psy = container_of(work, struct power_supply,
>>                                                 changed_work);
>>
>>         dev_dbg(psy->dev, "%s\n", __func__);
>>
>> -       class_for_each_device(power_supply_class, NULL, psy,
>> -                             __power_supply_changed_work);
>> -
>> -       power_supply_update_leds(psy);
>> -
>> -       kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
>> +       spin_lock_irqsave(&psy->changed_lock, flags);
>> +       if (psy->changed) {
>
> Can this be false here? We have reached here as the work was
> scheduled after setting it to true..
>
> Maybe a WARN_ON(psy->changed) is more sensible here ?
>
>> +               psy->changed = false;
>> +               spin_unlock_irqrestore(&psy->changed_lock, flags);
>> +               class_for_each_device(power_supply_class, NULL, psy,
>> +                                     __power_supply_changed_work);
>> +               power_supply_update_leds(psy);
>> +               kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
>> +               spin_lock_irqsave(&psy->changed_lock, flags);
>> +       }
>> +       /* dependent power supplies (e.g. battery) may have changed
>> +        * state as a result of this event, so poll again and hold
>> +        * the wakeup_source until all events are processed.
>> +        */
>> +       if (!psy->changed)
>> +               pm_relax(psy->dev);
>
> I got a bit confused here. Does the above comment say this:
>
> The supplies dependent on 'psy' may change states and that *may*
> change the state of 'psy' again? And so psy->changed is set to true
> again?

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Viresh Kumar]

Thanks for your quick reply :)

On 4 September 2014 00:43, Zoran Markovic <zrn.markovic@gmail.com> wrote:
> Note that power_supply_changed_work() could race with
> power_supply_changed(), as well as with itself. You could theoretically run
> power_supply_changed() several times and queue several
> power_supply_changed_work() calls. The first run of
> power_supply_changed_work() would cancel all previous power_supply_changed()
> and the remaining runs would just encounter psy->changed == FALSE and fall
> through.

That's not completely true. You can't queue the same work multiple times. And
the work is queued only if its not pending. The pending bit is just
cleared before calling
the work-handler.

The worst corner case is that the work-handler, i.e. power_supply_changed_work()
is called and just before taking the lock, another work is enqueued. Now for the
first iteration of power_supply_changed_work() we will surely get
changes as TRUE,
but for second one it may be FALSE.

So, yes my theory was incorrect.

>> >> +               psy->changed = false;
>> >> +               spin_unlock_irqrestore(&psy->changed_lock, flags);
>> >> +               class_for_each_device(power_supply_class, NULL, psy,
>> >> +                                     __power_supply_changed_work);
>> >> +               power_supply_update_leds(psy);
>> >> +               kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE);
>> >> +               spin_lock_irqsave(&psy->changed_lock, flags);
>> >> +       }
>> >> +       /* dependent power supplies (e.g. battery) may have changed
>> >> +        * state as a result of this event, so poll again and hold
>> >> +        * the wakeup_source until all events are processed.
>> >> +        */
>> >> +       if (!psy->changed)
>> >> +               pm_relax(psy->dev);
>> >
>> > I got a bit confused here. Does the above comment say this:
>> >
>> > The supplies dependent on 'psy' may change states and that *may*
>> > change the state of 'psy' again? And so psy->changed is set to true
>> > again?
>
> This is where power_supply_changed_work() could race with another
> power_supply_changed(). By the time you hit the check for !psy->changed,
> another work may have been already queued. Calling pm_relax() without this
> check could defer that work until next resume.

Hmm.. Correct here as well.

Thanks for your explanation.

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Viresh Kumar]

On 4 September 2014 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> >> +       /* dependent power supplies (e.g. battery) may have changed
>>> >> +        * state as a result of this event, so poll again and hold
>>> >> +        * the wakeup_source until all events are processed.
>>> >> +        */

But isn't this comment still incorrect? Its not about dependent supplies
but the race between the work-handler and its enqueuing?

RE: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Tc, Jenny]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1159 bytes --]

If the intention is to prevent suspend while processing the power supply uevents, 
Isn't it a good option to use EPOLLWAKEUP? In Android, it's already used by
healthd to achieve the same.

-Jenny

> Subject: Re: [RFC PATCH] pm: prevent suspend until power supply events are
> processed
> 
> On 4 September 2014 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >>> >> +       /* dependent power supplies (e.g. battery) may have changed
> >>> >> +        * state as a result of this event, so poll again and hold
> >>> >> +        * the wakeup_source until all events are processed.
> >>> >> +        */
> 
> But isn't this comment still incorrect? Its not about dependent supplies but the race
> between the work-handler and its enqueuing?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a
> message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Todd Poynor]

On Thu, Sep 4, 2014 at 9:37 AM, Tc, Jenny <jenny.tc@intel.com> wrote:
> If the intention is to prevent suspend while processing the power supply uevents,
> Isn't it a good option to use EPOLLWAKEUP? In Android, it's already used by
> healthd to achieve the same.

It's a good idea for the userspace handler of the power_supply uevents
to use EPOLLWAKEUP.  This patch will help ensure the system remains
awake until uevent dispatching grabs its wakelock, and then the
userspace receiver can take over preventing suspend via EPOLLWAKEUP.


Todd

Re: [RFC PATCH] pm: prevent suspend until power supply events are processed

[Todd Poynor]

On Wed, Sep 3, 2014 at 9:53 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 4 September 2014 10:21, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>> >> +       /* dependent power supplies (e.g. battery) may have changed
>>>> >> +        * state as a result of this event, so poll again and hold
>>>> >> +        * the wakeup_source until all events are processed.
>>>> >> +        */
>
> But isn't this comment still incorrect? Its not about dependent supplies
> but the race between the work-handler and its enqueuing?

I believe you are correct: the code is to make sure we do not relax a
wakeup source that has had multiple events queued, until all are
notified to userspace.


Todd