Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 7 November 2014 09:06, Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> wrote:
> On czw, 2014-11-06 at 23:51 +0100, Rafael J. Wysocki wrote:
>> On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote:
>> > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was
>> > called by child driver, regardless of CONFIG_PM_RUNTIME.
>> >
>> > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime
>> > suspend/resume callbacks in amba bus driver act differently if irq_safe
>> > was set by child driver (in irq_safe mode bus clock is only disabled).
>> >
>> > The pl330 driver sets irq_safe and assumes that amba bus driver will
>> > only disable the clock in runtime PM. So in system sleep suspend
>> > callback the pl330 driver unprepares the clock after calling
>> > pm_runtime_force_suspend().
>> >
>> > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and
>> > child drivers do not want the irq_safe runtime PM. In such case amba bus
>> > driver still has to know whether child driver wanted irq_safe - by
>> > looking at dev->power.irq_safe field.
>> >
>> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
>> > ---
>> >  include/linux/pm.h         | 2 +-
>> >  include/linux/pm_runtime.h | 5 ++++-
>> >  2 files changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/linux/pm.h b/include/linux/pm.h
>> > index 383fd68aaee1..b05fa954f50d 100644
>> > --- a/include/linux/pm.h
>> > +++ b/include/linux/pm.h
>> > @@ -566,6 +566,7 @@ struct dev_pm_info {
>> >     bool                    ignore_children:1;
>> >     bool                    early_init:1;   /* Owned by the PM core */
>> >     bool                    direct_complete:1;      /* Owned by the PM core */
>> > +   unsigned int            irq_safe:1;     /* PM runtime */
>> >     spinlock_t              lock;
>> >  #ifdef CONFIG_PM_SLEEP
>> >     struct list_head        entry;
>> > @@ -590,7 +591,6 @@ struct dev_pm_info {
>> >     unsigned int            run_wake:1;
>> >     unsigned int            runtime_auto:1;
>> >     unsigned int            no_callbacks:1;
>> > -   unsigned int            irq_safe:1;
>> >     unsigned int            use_autosuspend:1;
>> >     unsigned int            timer_autosuspends:1;
>> >     unsigned int            memalloc_noio:1;
>>
>> Well, that is a good reason to introduce a wrapper around power.irq_safe in my
>> view.
>>
>> And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
>> unset.
>>
>> This way not only you wouldn't need to move the flag from under the #ifdef,
>> but also you would make the compiler skip the relevant pieces of code
>> entiretly for CONFIG_PM_RUNTIME unset.
>
> Few days ago I would be happy with your opinion :), but know I think
> this is better solution than wrapper. Consider case:
> 1. PM_RUNTIME unset.
> 2. System suspends.
> 3. The pl330 in its suspend callback calls force_runtime_suspend which
> leads us to amba/bus.
> 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE),
> so it disables and unprepares the clock.
> 5. The pl330 in probe requested irq_safe so it assumes amba/bus will
> only disable the clock. So the pl330 unprepares the clock. Again.

This is easy to solve, still by using Rafael's approach.

In the pl330 system PM callbacks, you need to check
"pm_runtime_is_irqsafe()" or whatever the wrapper would be called.
When it returns true, that's when you should do
clk_prepare|unprepare().

I think that would be quite nice.

Kind regards
Uffe
--
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/