From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips Date: Thu, 21 Jan 2016 08:38:55 +0000 Message-ID: <56A0991F.4010802@nvidia.com> References: <1450349309-8107-1-git-send-email-jonathanh@nvidia.com> <1450349309-8107-4-git-send-email-jonathanh@nvidia.com> <569E1366.8070005@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Thomas Gleixner Cc: Ulf Hansson , Jason Cooper , Marc Zyngier , Jiang Liu , Stephen Warren , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Linus Walleij , Soren Brinkmann , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "Rafael J. Wysocki" List-Id: linux-tegra@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759116AbcAUIjL (ORCPT ); Thu, 21 Jan 2016 03:39:11 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:10570 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758587AbcAUIjI (ORCPT ); Thu, 21 Jan 2016 03:39:08 -0500 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Thu, 21 Jan 2016 00:40:14 -0800 Subject: Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips To: Thomas Gleixner References: <1450349309-8107-1-git-send-email-jonathanh@nvidia.com> <1450349309-8107-4-git-send-email-jonathanh@nvidia.com> <569E1366.8070005@nvidia.com> CC: Ulf Hansson , Jason Cooper , Marc Zyngier , Jiang Liu , Stephen Warren , Thierry Reding , Kevin Hilman , "Geert Uytterhoeven" , Grygorii Strashko , Lars-Peter Clausen , "Linus Walleij" , Soren Brinkmann , "linux-kernel@vger.kernel.org" , "linux-tegra@vger.kernel.org" , "linux-pm@vger.kernel.org" , "Rafael J. Wysocki" From: Jon Hunter Message-ID: <56A0991F.4010802@nvidia.com> Date: Thu, 21 Jan 2016 08:38:55 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.21.132.159] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL102.nvidia.com (10.26.138.15) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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