| Message ID | 1458224359-32665-6-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jon Hunter |
| Headers | show |
On Thu, 17 Mar 2016 14:19:09 +0000 Jon Hunter <jonathanh@nvidia.com> wrote: > The firmware parameter that contains the IRQ sense bits may also contain > other data. When return the IRQ type, bits outside of these sense bits > should be masked. If these bits are not masked and > irq_create_fwspec_mapping() is called to map an IRQ, then the comparison > of the type returned from irq_domain_translate() will never match > that returned by irq_get_trigger_type() (because this function masks the > none sense bits) and so we will always call irq_set_irq_type() to program > the type even if it was not really necessary. > > Currently, the downside to this is unnecessarily re-programmming the type > but nevertheless this should be avoided. > > The Tegra LIC, TI Crossbar and GIC-V3 irqchips all have client instances > (from reviewing the device-tree sources) where bits outside the IRQ sense > bits are set, but do not mask these bits. Therefore, ensure these bits > are masked for these irqchips. For GICv3, this shouldn't be the case. The DT clearly says that the 3rd field should only contain the trigger description. It looks like people have been copying their GICv2 DT and simply slapped the v3 description in the middle, without changing their interrupt specifiers. Duh. I guess this patch doesn't hurt though. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> Acked-by: Marc Zyngier <marc.zyngier@arm.com> M.
On 09/04/16 12:03, Marc Zyngier wrote: > On Thu, 17 Mar 2016 14:19:09 +0000 > Jon Hunter <jonathanh@nvidia.com> wrote: > >> The firmware parameter that contains the IRQ sense bits may also contain >> other data. When return the IRQ type, bits outside of these sense bits >> should be masked. If these bits are not masked and >> irq_create_fwspec_mapping() is called to map an IRQ, then the comparison >> of the type returned from irq_domain_translate() will never match >> that returned by irq_get_trigger_type() (because this function masks the >> none sense bits) and so we will always call irq_set_irq_type() to program >> the type even if it was not really necessary. >> >> Currently, the downside to this is unnecessarily re-programmming the type >> but nevertheless this should be avoided. >> >> The Tegra LIC, TI Crossbar and GIC-V3 irqchips all have client instances >> (from reviewing the device-tree sources) where bits outside the IRQ sense >> bits are set, but do not mask these bits. Therefore, ensure these bits >> are masked for these irqchips. > > For GICv3, this shouldn't be the case. The DT clearly says that the 3rd > field should only contain the trigger description. It looks like people > have been copying their GICv2 DT and simply slapped the v3 description > in the middle, without changing their interrupt specifiers. Duh. Hmmm ... I was just double checking on this for the gic-v3 by wading through the DTS files, and may be there is no issue here. However, looking at the current code it is a bit inconsistent between firmware types ... static int gic_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, unsigned long *hwirq, unsigned int *type) { if (is_of_node(fwspec->fwnode)) { if (fwspec->param_count < 3) return -EINVAL; switch (fwspec->param[0]) { case 0: /* SPI */ *hwirq = fwspec->param[1] + 32; break; case 1: /* PPI */ *hwirq = fwspec->param[1] + 16; break; case GIC_IRQ_TYPE_LPI: /* LPI */ *hwirq = fwspec->param[1]; break; default: return -EINVAL; } *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; } if (is_fwnode_irqchip(fwspec->fwnode)) { if(fwspec->param_count != 2) return -EINVAL; *hwirq = fwspec->param[0]; *type = fwspec->param[1]; return 0; } return -EINVAL; > I guess this patch doesn't hurt though. Yes, it doesn't but I think I will leave this alone for now, given that I can't find a case where this would be a problem. 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
Hi Jon, On 19/04/16 15:14, Jon Hunter wrote: > > On 09/04/16 12:03, Marc Zyngier wrote: >> On Thu, 17 Mar 2016 14:19:09 +0000 >> Jon Hunter <jonathanh@nvidia.com> wrote: >> >>> The firmware parameter that contains the IRQ sense bits may also contain >>> other data. When return the IRQ type, bits outside of these sense bits >>> should be masked. If these bits are not masked and >>> irq_create_fwspec_mapping() is called to map an IRQ, then the comparison >>> of the type returned from irq_domain_translate() will never match >>> that returned by irq_get_trigger_type() (because this function masks the >>> none sense bits) and so we will always call irq_set_irq_type() to program >>> the type even if it was not really necessary. >>> >>> Currently, the downside to this is unnecessarily re-programmming the type >>> but nevertheless this should be avoided. >>> >>> The Tegra LIC, TI Crossbar and GIC-V3 irqchips all have client instances >>> (from reviewing the device-tree sources) where bits outside the IRQ sense >>> bits are set, but do not mask these bits. Therefore, ensure these bits >>> are masked for these irqchips. >> >> For GICv3, this shouldn't be the case. The DT clearly says that the 3rd >> field should only contain the trigger description. It looks like people >> have been copying their GICv2 DT and simply slapped the v3 description >> in the middle, without changing their interrupt specifiers. Duh. > > Hmmm ... I was just double checking on this for the gic-v3 by wading > through the DTS files, and may be there is no issue here. However, > looking at the current code it is a bit inconsistent between firmware > types ... > > static int gic_irq_domain_translate(struct irq_domain *d, > struct irq_fwspec *fwspec, > unsigned long *hwirq, > unsigned int *type) > { > if (is_of_node(fwspec->fwnode)) { > if (fwspec->param_count < 3) > return -EINVAL; > > switch (fwspec->param[0]) { > case 0: /* SPI */ > *hwirq = fwspec->param[1] + 32; > break; > case 1: /* PPI */ > *hwirq = fwspec->param[1] + 16; > break; > case GIC_IRQ_TYPE_LPI: /* LPI */ > *hwirq = fwspec->param[1]; > break; > default: > return -EINVAL; > } > > *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > return 0; > } > > if (is_fwnode_irqchip(fwspec->fwnode)) { > if(fwspec->param_count != 2) > return -EINVAL; > > *hwirq = fwspec->param[0]; > *type = fwspec->param[1]; That's because param[1] doesn't really come from the firmware. It is actually provided directly by acpi_dev_get_irq_type, so more or less guaranteed to be a valid value. Thanks, M.
diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c index 75573fa431ba..1eef56a89b1f 100644 --- a/drivers/irqchip/irq-crossbar.c +++ b/drivers/irqchip/irq-crossbar.c @@ -183,7 +183,7 @@ static int crossbar_domain_translate(struct irq_domain *d, return -EINVAL; *hwirq = fwspec->param[1]; - *type = fwspec->param[2]; + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; } diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index c569c466fa31..972335b32eae 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -777,7 +777,7 @@ static int gic_irq_domain_translate(struct irq_domain *d, return -EINVAL; *hwirq = fwspec->param[0]; - *type = fwspec->param[1]; + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; return 0; } diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c index 121ec301372e..7ceaff099072 100644 --- a/drivers/irqchip/irq-tegra.c +++ b/drivers/irqchip/irq-tegra.c @@ -235,7 +235,7 @@ static int tegra_ictlr_domain_translate(struct irq_domain *d, return -EINVAL; *hwirq = fwspec->param[1]; - *type = fwspec->param[2]; + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; return 0; }
The firmware parameter that contains the IRQ sense bits may also contain other data. When return the IRQ type, bits outside of these sense bits should be masked. If these bits are not masked and irq_create_fwspec_mapping() is called to map an IRQ, then the comparison of the type returned from irq_domain_translate() will never match that returned by irq_get_trigger_type() (because this function masks the none sense bits) and so we will always call irq_set_irq_type() to program the type even if it was not really necessary. Currently, the downside to this is unnecessarily re-programmming the type but nevertheless this should be avoided. The Tegra LIC, TI Crossbar and GIC-V3 irqchips all have client instances (from reviewing the device-tree sources) where bits outside the IRQ sense bits are set, but do not mask these bits. Therefore, ensure these bits are masked for these irqchips. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/irqchip/irq-crossbar.c | 2 +- drivers/irqchip/irq-gic-v3.c | 2 +- drivers/irqchip/irq-tegra.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)