Re: [PATCHv2 1/1] kernel/power/autosleep.c: check for pm_suspend() return before queueing suspend again

[Nitish Ambastha]

On Wed, Jul 1, 2015 at 1:31 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>
> On Wednesday, July 01, 2015 12:52:43 AM Nitish Ambastha wrote:
> > Hi Rafael
> >
> > Thanks for your feedback
> >
> > On Tue, Jun 30, 2015 at 1:37 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > > On Monday, June 29, 2015 09:56:18 PM Rafael J. Wysocki wrote:
> > >> On Tuesday, June 30, 2015 12:24:14 AM Nitish Ambastha wrote:
> > >> > Prevent tight loop for suspend-resume when some
> > >> > devices failed to suspend
> > >> > If some devices failed to suspend, we monitor this
> > >> > error in try_to_suspend(). pm_suspend() is already
> > >> > an 'int' returning function, how about checking return
> > >> > from pm_suspend() before queueing suspend again?
> > >> >
> > >> > For devices which do not register for pending events,
> > >> > this will prevent tight loop for suspend-resume in
> > >> > suspend abort scenarios due to device suspend failures
> > >
> > > Having said the below I'm not sure why the current code doesn't cover this
> > > for you?
> > >
> > > That would be the final_count == initial_count case, no?
> > >
> > Agree, this should cover most of the cases, however there are some
> > cases where final_count may not match initial_count here
> >
> > A couple of such scenario I came across is
> > 1) when tasks are restarted again due to suspend failure, sometimes
> > battery kernel thread acquires lock for battery monitoring resulting
> > in either pm_get_wakeup_count() returning false or increment in
> > final_count from initial_count
>
> Locks should not have any effect on the return value of pm_get_wakeup_count()
> and if false is returned by it, a wakeup event was being processed when it
> was called.
>
'lock' was not a correct term used, sorry about it.
By 'lock', I actually meant battery monitoring acquiring 'wake lock' here
i.e pm_wakeup_event/pm_stay_awake().

when battery kernel thread calls pm_stay_awake() or pm_wakeup_event()
momentarily and then pm_relax(), after being restarted on suspend failure,
this affects the wakeup event count

> In turn, if pm_get_wakeup_count() returns false or final_count != initial_count,
> this means that *somebody* called pm_wakeup_event() or equivalent in the meantime
> and there *was* a valid wakeup event (regardless of or in addition to the driver
> error).
>
ok, as I understand, if some driver failed to suspend, and during resume
if *somebody* called pm_stay_awake() or pm_wakeup_event() meantime,
and then pm_relax(), final_count and initial_count will not be
same in try_to_suspend(), and it will be considered as a *valid wakeup*
event, though the actual reason of resume was suspend failure.
In this condition, it will again try to queue suspend

Will it be a reasonable idea to wait in this case, before queueing
suspend as some drivers failed to suspend in the current attempt?

> > 2) In some platforms, power transitions are carried from User space
> > (power manager), these power-manager tries to hold some wake lock
> > after being restarted on resume
>
> And what exactly is the failing scenario in that case?
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
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/