Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On Wed, Sep 3, 2014 at 9:53 PM, Viresh Kumar wrote: > On 4 September 2014 10:21, Viresh Kumar 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On Thu, Sep 4, 2014 at 9:37 AM, Tc, Jenny 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On Thu, Sep 4, 2014 at 9:37 AM, Tc, Jenny jenny...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On Wed, Sep 3, 2014 at 9:53 PM, Viresh Kumar viresh.ku...@linaro.org wrote: On 4 September 2014 10:21, Viresh Kumar viresh.ku...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH] pm: prevent suspend until power supply events are processed
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 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 majord...@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC PATCH] pm: prevent suspend until power supply events are processed
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.ku...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On 4 September 2014 10:21, Viresh Kumar 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
Thanks for your quick reply :) On 4 September 2014 00:43, Zoran Markovic 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(>changed_lock, flags); >> >> + class_for_each_device(power_supply_class, NULL, psy, >> >> + __power_supply_changed_work); >> >> + power_supply_update_leds(psy); >> >> + kobject_uevent(>dev->kobj, KOBJ_CHANGE); >> >> + spin_lock_irqsave(>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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
Thanks for your quick reply :) On 4 September 2014 00:43, Zoran Markovic zrn.marko...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On 4 September 2014 10:21, Viresh Kumar viresh.ku...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On Wed, Sep 3, 2014 at 10:09 AM, Viresh Kumar 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
> 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(>dev->kobj, KOBJ_CHANGE);
>> + spin_lock_irqsave(>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(>changed_lock, flags);
>> + class_for_each_device(power_supply_class, NULL, psy,
>> + __power_supply_changed_work);
>> + power_supply_update_leds(psy);
>> + kobject_uevent(>dev->kobj, KOBJ_CHANGE);
>> + spin_lock_irqsave(>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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
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
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(>dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(>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(>changed_lock, flags);
> + class_for_each_device(power_supply_class, NULL, psy,
> + __power_supply_changed_work);
> + power_supply_update_leds(psy);
> + kobject_uevent(>dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(>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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
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.marko...@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?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
On Wed, Sep 3, 2014 at 10:09 AM, Viresh Kumar viresh.ku...@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.marko...@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?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
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
> Cc: David Woodhouse
> Cc: Arve Hjonnevag
> Cc: Todd Poynor
> Cc: John Stultz
> Signed-off-by: Zoran Markovic
> ---
...
> + kobject_uevent(>dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(>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 is needed.
I fixed it up and applied the patch, thanks a lot!
Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
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 an...@enomsg.org
Cc: David Woodhouse dw...@infradead.org
Cc: Arve Hjonnevag a...@android.com
Cc: Todd Poynor toddpoy...@google.com
Cc: John Stultz john.stu...@linaro.org
Signed-off-by: Zoran Markovic zoran.marko...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
Any opinions on this patch?
Regards, Zoran
On 2 August 2013 13:38, 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
> Cc: David Woodhouse
> Cc: Arve Hjonnevag
> Cc: Todd Poynor
> Cc: John Stultz
> Signed-off-by: Zoran Markovic
> ---
> 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(>dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(>changed_lock, flags);
> + if (psy->changed) {
> + psy->changed = false;
> + spin_unlock_irqrestore(>changed_lock, flags);
> + class_for_each_device(power_supply_class, NULL, psy,
> + __power_supply_changed_work);
> + power_supply_update_leds(psy);
> + kobject_uevent(>dev->kobj, KOBJ_CHANGE);
> + spin_lock_irqsave(>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(>changed_lock, flags);
> }
>
> void power_supply_changed(struct power_supply *psy)
> {
> + unsigned long flags;
> +
> dev_dbg(psy->dev, "%s\n", __func__);
>
> + spin_lock_irqsave(>changed_lock, flags);
> + psy->changed = true;
> + pm_stay_awake(psy->dev);
> + spin_unlock_irqrestore(>changed_lock, flags);
> schedule_work(>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(>changed_lock);
> + rc = device_init_wakeup(dev, true);
> + if (rc)
> + goto wakeup_init_failed;
> +
> rc = kobject_set_name(>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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] pm: prevent suspend until power supply events are processed
Any opinions on this patch?
Regards, Zoran
On 2 August 2013 13:38, Zoran Markovic zoran.marko...@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 an...@enomsg.org
Cc: David Woodhouse dw...@infradead.org
Cc: Arve Hjonnevag a...@android.com
Cc: Todd Poynor toddpoy...@google.com
Cc: John Stultz john.stu...@linaro.org
Signed-off-by: Zoran Markovic zoran.marko...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] pm: prevent suspend until power supply events are processed
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
Cc: David Woodhouse
Cc: Arve Hjonnevag
Cc: Todd Poynor
Cc: John Stultz
Signed-off-by: Zoran Markovic
---
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(>dev->kobj, KOBJ_CHANGE);
+ spin_lock_irqsave(>changed_lock, flags);
+ if (psy->changed) {
+ psy->changed = false;
+ spin_unlock_irqrestore(>changed_lock, flags);
+ class_for_each_device(power_supply_class, NULL, psy,
+ __power_supply_changed_work);
+ power_supply_update_leds(psy);
+ kobject_uevent(>dev->kobj, KOBJ_CHANGE);
+ spin_lock_irqsave(>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(>changed_lock, flags);
}
void power_supply_changed(struct power_supply *psy)
{
+ unsigned long flags;
+
dev_dbg(psy->dev, "%s\n", __func__);
+ spin_lock_irqsave(>changed_lock, flags);
+ psy->changed = true;
+ pm_stay_awake(psy->dev);
+ spin_unlock_irqrestore(>changed_lock, flags);
schedule_work(>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(>changed_lock);
+ rc = device_init_wakeup(dev, true);
+ if (rc)
+ goto wakeup_init_failed;
+
rc = kobject_set_name(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH] pm: prevent suspend until power supply events are processed
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 an...@enomsg.org
Cc: David Woodhouse dw...@infradead.org
Cc: Arve Hjonnevag a...@android.com
Cc: Todd Poynor toddpoy...@google.com
Cc: John Stultz john.stu...@linaro.org
Signed-off-by: Zoran Markovic zoran.marko...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
