linux-kernel - Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56A0991F.4010802@nvidia.com>
Date:	Thu, 21 Jan 2016 08:38:55 +0000
From:	Jon Hunter <jonathanh@...dia.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Ulf Hansson <ulf.hansson@...aro.org>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>,
	Jiang Liu <jiang.liu@...ux.intel.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Kevin Hilman <khilman@...nel.org>,
	"Geert Uytterhoeven" <geert@...ux-m68k.org>,
	Grygorii Strashko <grygorii.strashko@...com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	"Linus Walleij" <linus.walleij@...aro.org>,
	Soren Brinkmann <soren.brinkmann@...inx.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support
 for IRQ chips


On 20/01/16 15:30, Thomas Gleixner wrote:
> On Tue, 19 Jan 2016, Jon Hunter wrote:
>> On 18/01/16 14:47, Ulf Hansson wrote:
>>>> +/* 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
>  
>>> 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.
>  
>>>> @@ -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.

Ok, I will fix that.

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

That's fine with me.

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

Ok, that makes sense.

Cheers
Jon