Re: [PATCH v8 1/5] PM / Runtime: Add getter for querying the IRQ safe option

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Krzysztof,

On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote:
> On pią, 2014-10-31 at 15:22 +0100, Pavel Machek wrote:
> > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote:
> >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote:
> >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether
> >>> runtime PM IRQ safe was set or not.
> >>> 
> >>> Various bus drivers implementing runtime PM may use choose to suspend
> >>> differently based on IRQ safeness status of child driver (e.g. do not
> >>> unprepare the clock if IRQ safe is not set).
> >>> 
> >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> 
> >> Rafael, Len, Pavel,
> >> 
> >> Is proposed API ok? Do you have any comments?
> >> 
> >> I'll upload whole patchset to Russell's patch tracking system. However
> >> an ack from PM maintainer is probably needed.
> > 
> > I don't like the API. Having callbacks work in different context (irq
> > / noirq) based on what another function reports is ugly.
> > 
> > What is the penalty if we always decide callbacks are not IRQ safe?
> 
> Then pm_runtime_get_sync() could not be called in atomic context. The
> pl330 runtime PM would have to be completely reworked because one
> pm_runtime_get_sync() is called in device_issue_pending which cannot
> sleep (at least in non preemptible kernels). Probably this can be solved
> some way...

Many other drivers suffer from the same problem. While I won't reject your 
proposed fix, I would prefer a more generic approach.

One option that has been discussed previously was to use a work queue to delay 
starting the DMA transfer to an interruptible context where 
pm_runtime_get_sync() could be called. However, as Russell pointed out [1], 
even that won't work in all cases as the DMA slave might need the transfer to 
be started before enabling part of its hardware (OMAP audio seem to be such a 
case).

I've heard a rumor of a possible DMA engine rework to forbid calling the 
descriptor preparation API from atomic context. This could be used as a base 
to implement runtime PM, as DMA slave drivers should not prepare descriptors 
if they don't need to use them. However that's a long term plan, and we need a 
solution sooner than that.

I've been toying with the idea of adding explicit open/close (or whatever we 
would call them) operations to the DMA engine API. Those would be used by DMA 
slave drivers to signal that they will start/stop using the DMA engine.

If (1) we must start the DMA synchronously with a DMA slave call, (2) need to 
sleep to handle PM, and (3) don't want to keep the DMA engine powered for as 
long as one channel is requested, then we need to turn at least preparation as 
not callable in atomic context, or introduce a new operation.

[1] http://www.spinics.net/lists/dmaengine/msg01548.html

> >>> --- a/Documentation/power/runtime_pm.txt
> >>> +++ b/Documentation/power/runtime_pm.txt
> >>> 
> >>> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and
> >>> include/linux/pm_runtime.h:
> >>>      - set the power.irq_safe flag for the device, causing the
> >>>      runtime-PM
> >>>      
> >>>        callbacks to be invoked with interrupts off
> >>> 
> >>> +  bool pm_runtime_is_irq_safe(struct device *dev);
> >>> +    - return true if power.irq_safe flag was set for the device,
> >>> causing
> >>> +      the runtime-PM callbacks to be invoked with interrupts off
> >>> +
> >>> 
> >>>    void pm_runtime_mark_last_busy(struct device *dev);
> >>>    
> >>>      - set the power.last_busy field to the current time

-- 
Regards,

Laurent Pinchart