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@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/