| Message ID | 1458224359-32665-7-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jon Hunter |
| Headers | show |
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 <jonathanh@nvidia.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/17/2016 08:18 PM, Jon Hunter wrote: > > 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 <jonathanh@nvidia.com> >> --- >> 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; Not sure that this warn and in this place make sense. type will come unchanged here from caller irq_create_fwspec_mapping() [...]
On 18/03/16 10:03, Grygorii Strashko wrote: > On 03/17/2016 08:18 PM, Jon Hunter wrote: >> >> 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 <jonathanh@nvidia.com> >>> --- >>> 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; > > Not sure that this warn and in this place make sense. > type will come unchanged here from caller irq_create_fwspec_mapping() Yes you are right. I missed the return statement. I can move to irq_create_fwspec_mapping(). Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } /* Set type if specified and different than the current one */
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 <jonathanh@nvidia.com> --- kernel/irq/irqdomain.c | 59 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 16 deletions(-)