Question on 1d9174fbc55e (pm_runtime_force_suspend/resume)

Question on 1d9174fbc55e (pm_runtime_force_suspend/resume)

[Lukas Wunner]

Hi Ulf,

I'm trying (and failing) to understand your commit 1d9174fbc55e ("PM /
Runtime: Defer resuming of the device in pm_runtime_force_resume()")
and was hoping that you could give me a clue:

You're acquiring a runtime PM ref on the parent when a device is force-
suspended and releasing it when it's force-resumed.  Moreover you're only
doing this if the device was runtime active upon force-suspend.

What is the purpose of this?

I researched in the mailing list archive and it seems your commit
superseded a patch submitted by Laurent Pinchart which sought to prevent
a gratuitous runtime resume when coming out of system sleep if the device
was runtime suspended before sleep:
https://lkml.org/lkml/2016/3/3/807

The commit message of 1d9174fbc55e also cites this as motivation for the
change.  It does not explain why a runtime PM ref on the parent needs to
be held.

The commit adds a "goto out" to pm_runtime_force_resume() and this would
already seem sufficient to achieve the objective described in the commit
message.

The commit adds the following comment to pm_runtime_force_suspend():

+       /*
+        * Increase the runtime PM usage count for the device's parent, in case
+        * when we find the device being used when system suspend was invoked.
+        * This informs pm_runtime_force_resume() to resume the parent
+        * immediately, which is needed to be able to resume its children,
+        * when not deferring the resume to be managed via runtime PM.
+        */
+       if (dev->parent && atomic_read(&dev->power.usage_count) > 1)
+               pm_runtime_get_noresume(dev->parent);

The comment talks about pm_runtime_force_resume() having to "resume the
parent immediately", but there is no code to that effect in the function.

"Resuming immediately" would presumably involve pm_runtime_get_sync() to
ensure that the parent is runtime active before the force-suspended device
can be resumed.  But there is no call to pm_runtime_get_sync() in
pm_runtime_force_resume().  Instead, the function *drops* a runtime PM ref
on the parent, allowing it to runtime suspend!

So let's look at it the other way round, what is the *effect* of your
change, maybe this will explain what its purpose is:  The effect is that
you keep the parent runtime active while the child is force-suspended.

First of all, this seems entirely gratuitous because the parent will
not runtime suspend as long as its child_count is non-zero.  You're
not decreasing the parent's child_count in pm_runtime_force_suspend(),
so the parent cannot suspend and consequently increasing its usage_count
is superfluous.

(Okay the argument is moot if ignore_children is set on the parent,
but you're not checking that field in pm_runtime_force_suspend/resume().)

Secondly, why would you want to prevent the parent from suspending while
the child is force-suspended?  It seems like a waste of energy to me.

Thirdly, in pm_runtime_force_resume(), you're releasing the runtime PM
ref on the parent *before* the child is resumed.  If the purpose of the
runtime PM ref is to ensure wakefulness of the parent when the child is
force-resumed, shouldn't the PM ref be held until *after* the child is
runtime active again?

Another thing that confuses me is that you're acquiring the runtime PM
ref with "_noresume" and releasing with "_noidle".  In other words, the
effect of your change as stated above, that you keep the parent runtime
active while the child is force-suspend, is not achieved if the parent
was runtime suspended upon force-suspending the child.  (I can only see
this happening if ignore_children is set on the parent, but then why is
a runtime PM ref necessary at all?)

My confusion is complete, can you help me make sense of it?

Thanks,

Lukas

Re: Question on 1d9174fbc55e (pm_runtime_force_suspend/resume)

[Lukas Wunner]

Hi Ulf,

On Sun, May 07, 2017 at 11:13:27AM +0200, Lukas Wunner wrote:
> I'm trying (and failing) to understand your commit 1d9174fbc55e ("PM /
> Runtime: Defer resuming of the device in pm_runtime_force_resume()")
> and was hoping that you could give me a clue:
> 
> You're acquiring a runtime PM ref on the parent when a device is force-
> suspended and releasing it when it's force-resumed.  Moreover you're only
> doing this if the device was runtime active upon force-suspend.
> 
> What is the purpose of this?

After some more digging, it seems acquiring a runtime PM ref on the parent
was added in v3 of your patch to try to fix an issue reported by Geert:

https://patchwork.kernel.org/patch/9375061/

	Changes in v3:
	- Updated to take care of parent-child relations.
	  This patch has earlier been sent standalone, but also as a part
	  of series. In the end it turned out the solution needed some
	  improvement to take care of parent-child relations, as reported
	  by Geert [1].
	  Geert, I would really appreciate if you could help out testing
	  to make sure the reported issue is fixed.
	  [1] https://patches.linaro.org/patch/67940/

However Geert responded that the change in v3 does *not* fix the issue:

	  Unfortunately it doesn't help. Still fails on both
	  r8a73a4/ape6evm and sh73a0/kzm9g.

I then turned out that the issue reported by Geert required an entirely
different fix to an Ethernet driver:

	  Rafael, this one is ready to be queued. Unless there are other
	  objections.
	  The problems that was reported by Geert for this change, has
	  been taken care of. Those are fixed by the patch for the
	  smsc911x ethernet driver [1], which you queued a while ago.
	  [1] https://www.spinics.net/lists/netdev/msg401339.html

Could it be that maybe, just maybe, the code to acquire a runtime PM ref
on the parent is entirely unnecessary and ended up in mainline by mistake?

Thanks,

Lukas