Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936416AbcCQSST (ORCPT ); Thu, 17 Mar 2016 14:18:19 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:17783 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935760AbcCQSSQ (ORCPT ); Thu, 17 Mar 2016 14:18:16 -0400 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Thu, 17 Mar 2016 11:16:45 -0700 Subject: Re: [PATCH 06/15] irqdomain: Ensure type settings match for an existing mapping To: Thomas Gleixner , Jason Cooper , Marc Zyngier , =?UTF-8?Q?Beno=c3=aet_Cousson?= , Tony Lindgren , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , "Kumar Gala" , Stephen Warren , "Thierry Reding" References: <1458224359-32665-1-git-send-email-jonathanh@nvidia.com> <1458224359-32665-7-git-send-email-jonathanh@nvidia.com> CC: Kevin Hilman , Geert Uytterhoeven , Grygorii Strashko , Lars-Peter Clausen , Linus Walleij , , , , From: Jon Hunter Message-ID: <56EAF4E1.5020201@nvidia.com> Date: Thu, 17 Mar 2016 18:18:09 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1458224359-32665-7-git-send-email-jonathanh@nvidia.com> X-Originating-IP: [10.21.132.114] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) 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: 3912 Lines: 111 On 17/03/16 14:19, Jon Hunter wrote: > When mapping an IRQ, if a mapping already exists, then we simply return > the virtual IRQ number. However, we do not check that the type settings > for the existing mapping match those for the mapping that is about to be > created. It may be unlikely that the type settings would not match, but > check for this and don't return a valid IRQ mapping if the type settings > do not match. > > WARN if the type return by irq_domain_translate() has bits outside the > sense mask set and then clear these bits. If these bits are not cleared > then this will cause the comparision of the type settings for an > existing mapping to fail with that of the new mapping even if the sense > bit themselves match. The reason being is that the existing type > settings are read by calling irq_get_trigger_type() which will clear > any bits outside the sense mask. This will allow us to detect irqchips > that are not correctly clearing these bits and fix them. > > Signed-off-by: Jon Hunter > --- > kernel/irq/irqdomain.c | 59 ++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 43 insertions(+), 16 deletions(-) > > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c > index 3a519a01118b..0ea285baa619 100644 > --- a/kernel/irq/irqdomain.c > +++ b/kernel/irq/irqdomain.c > @@ -549,6 +549,13 @@ static int irq_domain_translate(struct irq_domain *d, > fwspec->param, fwspec->param_count, > hwirq, type); > > + /* > + * WARN if the irqchip returns a type with bits > + * outside the sense mask set and clear these bits. > + */ > + if (WARN_ON(*type & ~IRQ_TYPE_SENSE_MASK)) > + *type &= IRQ_TYPE_SENSE_MASK; > + > /* If domain has no translation, then we assume interrupt line */ > *hwirq = fwspec->param[0]; > return 0; > @@ -570,6 +577,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > { > struct irq_domain *domain; > irq_hw_number_t hwirq; > + unsigned int cur_type = IRQ_TYPE_NONE; > unsigned int type = IRQ_TYPE_NONE; > int virq; > > @@ -592,23 +600,42 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec) > if (irq_domain_translate(domain, fwspec, &hwirq, &type)) > return 0; > > - if (irq_domain_is_hierarchy(domain)) { > - /* > - * If we've already configured this interrupt, > - * don't do it again, or hell will break loose. > - */ > - virq = irq_find_mapping(domain, hwirq); > - if (virq) > - return virq; > - > - virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec); > - if (virq <= 0) > - return 0; > + /* > + * If we've already configured this interrupt, > + * don't do it again, or hell will break loose. > + */ > + virq = irq_find_mapping(domain, hwirq); > + if (!virq) { > + if (irq_domain_is_hierarchy(domain)) { > + virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, > + fwspec); > + if (virq <= 0) > + return 0; > + } else { > + /* Create mapping */ > + virq = irq_create_mapping(domain, hwirq); > + if (!virq) > + return virq; > + } > } else { > - /* Create mapping */ > - virq = irq_create_mapping(domain, hwirq); > - if (!virq) > - return virq; > + cur_type = irq_get_trigger_type(virq); > + } > + > + /* > + * If the trigger type is not specified or matches the current > + * trigger type then we are done so return the interrupt number. > + */ > + if (type == IRQ_TYPE_NONE || type == cur_type) > + return virq; > + > + /* > + * If the trigger type is already set and does > + * not match this interrupt, then return 0. > + */ > + if (cur_type != IRQ_TYPE_NONE) { > + pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n", > + hwirq, of_node_full_name(to_of_node(fwspec->fwnode))); > + return 0; If we created a new mapping and return here, then this will leak the mapping. I will fix this. Sorry should have caught this before! Jon