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

[Rafael J. Wysocki]

On Friday, October 31, 2014 11:04:52 PM Russell King - ARM Linux wrote:
> On Sat, Nov 01, 2014 at 12:11:05AM +0100, Rafael J. Wysocki wrote:
> > [CC list trimmed + added Kevin Hilman]
> > 
> > On Monday, October 20, 2014 11:04:44 AM 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@xxxxxxxxxxx>
> > > Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > 
> > So why do we need to add the wrapper?
> > 
> > And it goes kind of against the intention which was to set irq_safe when
> > we knew that the callbacks were safe to be executed from interrupt context
> > and not when we wished that to be the case.
> 
> This was provided in the covering email - I quote:
> 
> This patchset adds runtime and system PM to the pl330 driver.
> 
> The runtime PM of pl330 driver requires interrupt safe suspend/resume
> callbacks which is in conflict with current amba bus driver.
> The latter also unprepares and prepares the AMBA bus clock which
> is not safe for atomic context.
> 
> The patchset solves this in patch 3/5 by handling clocks in different
> way if device driver set interrupt safe runtime PM.

So I'm still unsure why we need the wrapper.  IMHO this check in particular:

	WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));

(and should that be WARN_ON_ONCE(), for that matter?), looks better this way:

	WARN_ON(pcdev->irq_safe != dev->power.irq_safe);

and so on, pretty much.

Besides, these special "irq safe" code paths in the bus type look
considerably ugly to me.  I'd probably use an "irq safe" PM domain for
that device and put it in there instead of doing the

	pcdev->irq_safe = pm_runtime_is_irq_safe(dev);

thing in amba_probe().  But that's just me. :-)

There's one weak point in [3/5], but let me comment it in there.

Rafael

--
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/