Re: Issues with calling pm_runtime functions in platform_pm_suspend_noirq/IRQ disabled context.

[Kevin Hilman <khilman@deeprootsystems.com>]

"Basak, Partha" <p-basak2@ti.com> writes:

> Resending with the corrected mailing list
>
> Hello Kevin,
>
> I want to draw your attention to an issue the following patch
> introduces. http://dev.omapzoom.org/?p=swarch/linux-omap-adv.git;a=commitdiff;h=8041359e18e49bf8a3d41f15894db9e476f3a8fc

[...]

> I believe, it is not correct to call the pm_runtime APIs from
> interrupt locked context since the underlying functions
> __pm_runtime_suspend & __pm_runtime_resume enable interrupts and call
> schedule().
>
> Further, from a s/w layering standpoint, platform-bus is a deeper
> layer than the Runtime layer, which the runtime layer calls into. So
> it may not be correct to have this layer calling into pm_runtime(). It
> should directly call omap_device APIs.

The original version of this patch did directly call the omap_device
APIs.  However, Paul didn't like that approach and there were
use-counting problems to handle as well (e.g. if a driver was not (yet)
in use, it would already be disabled, and a suspend would trigger an
omap_device_disable() which would fail since device was already
disabled.)

Paul and I agreed that this current approach would solve the
use-counting problems, but you're correct in that it introduces
new problems as these functions can _possibly_ sleep/schedule.

That being said, have you actually seen problems here?   I would like
to see how they are triggered?

By the time the drivers _noirq hooks are called, the PM core has
shutdown all the drivers, prevented any runtime PM transitions and
disabled interrupts, so no pending runtime transitions will be queued
so the sleeping patch of the __pm_runtime_* should never be entered.

> We are facing a similar issue with a few drivers USB, UART & GPIO
> where, we need to enable/disable clocks & restore/save context in the
> CPU-Idle path with interrupts locked.

These are unrelated issues.  The above is for the static suspend/resume
case.  These are for runtime PM.

> As we are unable to use Runtime APIs, we are using omap_device APIs
> directly with the following modification in the .activate_funcion/
> deactivate_funcion (example UART driver)

[...]

The UART driver is a special case as it is managed from deep inside the
idle path.

> Can you feedback on my observations and comment on the approach taken
> for these drivers?

I cannot comment until I understand why these drivers need to
enable/disable with interrupts off.

In general, any use of the interrupts-off versions of the omap_device
APIs need to be thorougly justified.

Even the UART one will go away when the omap-serial driver is converted
to runtime PM.

Kevin