On Tue, 19 Jan 2016, Jon Hunter wrote:

On 18/01/16 14:47, Ulf Hansson wrote:

quoted

quoted

+/* Inline functions for support of irq chips that require runtime pm */
+static inline int chip_pm_get(struct irq_desc *desc)

Why does these new get/put functions need to be inline functions and
thus defined in the header file? Perhaps move them to manage.c are
better?

They don't have to be, and so I can move them.

Yes, please make them proper functions. The proper place for them is chip.c
 

quoted

This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
would return -ENOSYS. In such cases I guess you would like to ignore
the error!?

Ok, yes good point.

So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
 

quoted

quoted

@@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
        if (!try_module_get(desc->owner))
                return -ENODEV;

+       ret = chip_pm_get(desc);
+       if (ret < 0)
+               return ret;

That leaks the module refcount.

quoted

I don't think using __free_irq() is the correct place to decrease the
runtime PM usage count. It will keep the irqchip runtime resumed even
if there are no irqs enabled for it.

Instead I would rather allow the irqchip to be runtime suspended, when
there are no irqs enabled on it.

Which is a no no, as you might lose interrupts that way. We disable interrupts
lazy, i.e. we do not mask them. So no, you cannot do that from
enable/disable_irq().
 

This may appear ugly, but for something like this, we may need to have a
separate enable/disable API, such as
enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
suspend/resume the chip and must not be used in critical sections.

enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

But before we go there I really want to see a proper use case for such
functions.

Thanks,

	tglx