Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754330AbcDVJe4 (ORCPT ); Fri, 22 Apr 2016 05:34:56 -0400 Received: from foss.arm.com ([217.140.101.70]:60321 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752937AbcDVJex (ORCPT ); Fri, 22 Apr 2016 05:34:53 -0400 Subject: Re: [PATCH V2 06/14] irqdomain: Don't set type when mapping an IRQ To: Jon Hunter , Thomas Gleixner , Jason Cooper , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Stephen Warren , Thierry Reding References: <1461150237-15580-1-git-send-email-jonathanh@nvidia.com> <1461150237-15580-7-git-send-email-jonathanh@nvidia.com> <5718F593.40605@nvidia.com> <5719DF5B.9010304@arm.com> <5719E563.3010303@nvidia.com> Cc: Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Linus Walleij , linux-tegra@vger.kernel.org, linux-omap@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org From: Marc Zyngier X-Enigmail-Draft-Status: N1110 Organization: ARM Ltd Message-ID: <5719F038.20800@arm.com> Date: Fri, 22 Apr 2016 10:34:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 MIME-Version: 1.0 In-Reply-To: <5719E563.3010303@nvidia.com> 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 Content-Length: 4797 Lines: 117 On 22/04/16 09:48, Jon Hunter wrote: > > On 22/04/16 09:22, Marc Zyngier wrote: >> Hi Jon, >> >> On 21/04/16 16:45, Jon Hunter wrote: >>> >>> On 20/04/16 12:03, Jon Hunter wrote: >>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt >>>> controllers, may require require additional runtime power management >>>> control to ensure they are accessible. For such IRQ chips, it makes sense >>>> to enable the IRQ chip when interrupts are requested and disabled them >>>> again once all interrupts have been freed. >>>> >>>> When mapping an IRQ, the IRQ type settings are read and then programmed. >>>> The mapping of the IRQ happens before the IRQ is requested and so the >>>> programming of the type settings occurs before the IRQ is requested. This >>>> is a problem for IRQ chips that require additional power management >>>> control because they may not be accessible yet. Therefore, when mapping >>>> the IRQ, don't program the type settings, just save them and then program >>>> these saved settings when the IRQ is requested (so long as if they are not >>>> overridden via the call to request the IRQ). >>>> >>>> Add a stub function for irq_domain_free_irqs() to avoid any compilation >>>> errors when CONFIG_IRQ_DOMAIN_HIERARCHY is not selected. >>>> >>>> Signed-off-by: Jon Hunter >>>> --- >>>> include/linux/irqdomain.h | 3 +++ >>>> kernel/irq/irqdomain.c | 17 +++++++++++++---- >>>> 2 files changed, 16 insertions(+), 4 deletions(-) >>> >>> [snip] >>> >>>> - /* Set type if specified and different than the current one */ >>>> - if (type != IRQ_TYPE_NONE && >>>> - type != irq_get_trigger_type(virq)) >>>> - irq_set_irq_type(virq, type); >>>> + irq_data = irq_get_irq_data(virq); >>>> + if (!irq_data) { >>>> + if (irq_domain_is_hierarchy(domain)) >>>> + irq_domain_free_irqs(virq, 1); >>>> + else >>>> + irq_dispose_mapping(virq); >>>> + return 0; >>>> + } >>>> + >>>> + /* Store trigger type */ >>>> + irqd_set_trigger_type(irq_data, type); >>>> + >>> >>> I appear to have missed another place for saving the irq type >>> which I had change in this version. Next time I will triple >>> check! Should have been ... >>> >>> >>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h >>> index 2aed04396210..fc66876d1965 100644 >>> --- a/include/linux/irqdomain.h >>> +++ b/include/linux/irqdomain.h >>> @@ -440,6 +440,9 @@ static inline int irq_domain_alloc_irqs(struct irq_domain *domain, >>> return -1; >>> } >>> >>> +static inline void irq_domain_free_irqs(unsigned int virq, >>> + unsigned int nr_irqs) { } >>> + >>> static inline bool irq_domain_is_hierarchy(struct irq_domain *domain) >>> { >>> return false; >>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c >>> index 3d6ef5527b71..46ecf5d468b2 100644 >>> --- a/kernel/irq/irqdomain.c >>> +++ b/kernel/irq/irqdomain.c >>> @@ -569,6 +569,7 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data, >>> unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >>> { >>> struct irq_domain *domain; >>> + struct irq_data *irq_data; >>> irq_hw_number_t hwirq; >>> unsigned int type = IRQ_TYPE_NONE; >>> int virq; >>> @@ -619,7 +620,11 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) >>> * it now and return the interrupt number. >>> */ >>> if (IRQ_TYPE_NONE == irq_get_trigger_type(virq)) { >>> - irq_set_irq_type(virq, type); >>> + irq_data = irq_get_irq_data(virq); >>> + if (!irq_data) >>> + return 0; >> >> Can you point out in which circumstances irq_data can be NULL? If we can >> lookup the interrupt in the domain, it'd better exist somewhere. Or am I >> missing something obvious? > > To be honest, I was not 100% sure if this could be possible if we find > an existing mapping or if there were any paths that could race. So if we > can guarantee that it is not NULL here, I can drop this and simplify the > change. No, you're right. It could very well race against irq_dispose_mapping (and the disassociate method doesn't provide enough guarantee). Unfortunately, most of the irqdomain code still relies on this race not happening. How to fix this is still a bit unclear to me. > By the way, is there any sort of reference count for mapping an IRQ so > that if irq_create_fwspec_mapping() is called more than once for an IRQ, > it is not freed on the first call to irq_dispose_mapping()? I assume > there is but could not see where this is handled. There is none, unfortunately. Thanks, M. -- Jazz is not dead. It just smells funny...