Skip to contentSkip to conversation

of/irq: Fix irq-mapping in of_irq_parse_raw()

PATCH New Tim Harvey 2 participants 4 messages Mar 4, 2014 – Mar 12, 2014

Subsystems (5)
Device tree bindings PCI Arm Samsung Exynos Tegra architecture support +1 more subsystems
lore
Share & tools
lore

Tim Harvey

author
#0 lore raw

PATCH 1/2 of/irq: Fix irq-mapping in of_irq_parse_raw()

When an interrupt-map contains multiple entries an imap pointer arithmetic bug can cause only the first entry to be properly evaluated and causes the out_irq parameters to be incorrect depending on the #interrupt-cells and #address-cells of the parent interrupt controller.

Specifically, the imap pointer into the interrupt-map table should be adjusted by the parent interrupt controller #interrupt-cells size only as at this point the only the parent unit interrupt specifier needs to be stepped over.

This resolves an issue encountered when using the of_irq_parse_and_map_pci() for the imx6 pcie host controller driver map_irq function where a P2P bridge is on the bus and legacy PCI interrupts are to be used.

Signed-off-by Tim Harvey <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc Jingoo Han <[email protected]>
Cc Lucas Stach <[email protected]>
Cc Mark Rutland <[email protected]>
Cc linux-samsung-soc <[email protected]>
Cc Richard Zhu <[email protected]>
Cc Sascha Hauer <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc Stephen Warren <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Simon Horman <[email protected]>
Cc Thierry Reding <[email protected]>
Cc Ben Dooks <[email protected]>
Cc linux-tegra <[email protected]>
Cc Kukjin Kim <[email protected]>
Cc Shawn Guo <[email protected]>
Cc Grant Likely <[email protected]>
---
 drivers/of/irq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/of/irq.c +3 -3
index 9bcf2cf..8829197 100644
@@ -237,11 +237,11 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 			/* Check for malformed properties */
 			if (WARN_ON(newaddrsize + newintsize > MAX_PHANDLE_ARGS))
 				goto fail;
-			if (imaplen < (newaddrsize + newintsize))
+			if (imaplen < newintsize)
 				goto fail;
 
-			imap += newaddrsize + newintsize;
-			imaplen -= newaddrsize + newintsize;
+			imap += newintsize;
+			imaplen -= newintsize;
 
 			pr_debug(" -> imaplen=%d\n", imaplen);
 		}
Signature · 1 line
1.8.3.2
author
#1 lore raw

PATCH 1/2 of/irq: Fix irq-mapping in of_irq_parse_raw()

Quoted 5 lines · Tim Harvey

When an interrupt-map contains multiple entries an imap pointer arithmetic bug can cause only the first entry to be properly evaluated and causes the out_irq parameters to be incorrect depending on the #interrupt-cells and #address-cells of the parent interrupt controller.

Quoted 9 lines

Specifically, the imap pointer into the interrupt-map table should be adjusted by the parent interrupt controller #interrupt-cells size only as at this point the only the parent unit interrupt specifier needs to be stepped over.

This resolves an issue encountered when using the of_irq_parse_and_map_pci() for the imx6 pcie host controller driver map_irq function where a P2P bridge is on the bus and legacy PCI interrupts are to be used.

Note the subject of my patch was incorrect - its a single patch, not a series. The subject should read:

[PATCH] of/irq: Fix irq-mapping in of_irq_parse_raw()

Sorry for the confusion,

Tim

#2 lore raw

PATCH 1/2 of/irq: Fix irq-mapping in of_irq_parse_raw()

In reply to Tim Harvey
Quoted 4 lines · Tim Harvey

When an interrupt-map contains multiple entries an imap pointer arithmetic bug can cause only the first entry to be properly evaluated and causes the out_irq parameters to be incorrect depending on the #interrupt-cells and #address-cells of the parent interrupt controller.

Tim,

I took a bit closer look at this for you, and I suspect the root fix is this:

Documentation/devicetree/bindings/arm/gic.txt +0 -1
@@ -55,7 +55,6 @@ Example:
        intc: interrupt-controller at fff11000 {
                compatible = "arm,cortex-a9-gic";
                #interrupt-cells = <3>;
-               #address-cells = <1>;
                interrupt-controller;
                reg = <0xfff11000 0x1000>,
                      <0xfff10100 0x100>;

(plus the corresponding purge from the .dt files)

It looks like the implementation does follow the OF specification:

Each mapping entry consists of a 3-tuple of (child-interrupt, interrupt-parent, parent-interrupt). The number of cells for the child-interrupt specifier is determined by the "#address-cells" and "#interrupt-cells"property of this node. The number of cells for the parent-interrupt value is determined by the "#address-cells"and "#interrupt-cells"property values of this node's interrupt-parent.

So by specifying interrupt-cells = 3, address-cells = 1, the GIC is requiring 4 DWs for its interrupt specifier.

I see no reason why it doesn't have an address-cells = 0 like other interrupt controllers..

Setting #address-cells to 0 in the GIC node should be functionally equivalent to your patch below, since newaddrsize will == 0.

Regards,
Jason

Quoted 18 lines
drivers/of/irq.c +4 -3
index 9bcf2cf..8829197 100644
+++ b/drivers/of/irq.c

@@ -237,11 +237,11 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 			/* Check for malformed properties */
 			if (WARN_ON(newaddrsize + newintsize > MAX_PHANDLE_ARGS))
 				goto fail;
-			if (imaplen < (newaddrsize + newintsize))
+			if (imaplen < newintsize)
 				goto fail;
 
-			imap += newaddrsize + newintsize;
-			imaplen -= newaddrsize + newintsize;
+			imap += newintsize;
+			imaplen -= newintsize;
 
 			pr_debug(" -> imaplen=%d\n", imaplen);
 		}
author
#3 lore raw

PATCH 1/2 of/irq: Fix irq-mapping in of_irq_parse_raw()

Quoted 25 lines · Jason Gunthorpe
Quoted 4 lines · Tim Harvey

When an interrupt-map contains multiple entries an imap pointer arithmetic bug can cause only the first entry to be properly evaluated and causes the out_irq parameters to be incorrect depending on the #interrupt-cells and #address-cells of the parent interrupt controller.

Tim,

I took a bit closer look at this for you, and I suspect the root fix is this:

Documentation/devicetree/bindings/arm/gic.txt +0 -1
@@ -55,7 +55,6 @@ Example:
        intc: interrupt-controller at fff11000 {
                compatible = "arm,cortex-a9-gic";
                #interrupt-cells = <3>;
-               #address-cells = <1>;
                interrupt-controller;
                reg = <0xfff11000 0x1000>,
                      <0xfff10100 0x100>;

(plus the corresponding purge from the .dt files)

It looks like the implementation does follow the OF specification:

Quoted 7 lines

Each mapping entry consists of a 3-tuple of (child-interrupt, interrupt-parent, parent-interrupt). The number of cells for the child-interrupt specifier is determined by the "#address-cells" and "#interrupt-cells"property of this node. The number of cells for the parent-interrupt value is determined by the "#address-cells"and "#interrupt-cells"property values of this node's interrupt-parent.

Ok - I see I misunderstood the spec with regards to what the address-cells related to.

Quoted 3 lines

So by specifying interrupt-cells = 3, address-cells = 1, the GIC is requiring 4 DWs for its interrupt specifier.

Agreed, and the interrupt mapping for the imx6 pcie host controller:

interrupt-map = <0 0 0 1 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,

                <0 0 0 2 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
                <0 0 0 3 &intc GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
                <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;

has only 3 cells for the parent interrupt, and only 3 are needed so I suppose there is no 'address' necessary.

Quoted 3 lines

I see no reason why it doesn't have an address-cells = 0 like other interrupt controllers..

I agree, and I see that as well for other cortex-a9 gic interrupt controllers.

Quoted 3 lines

Setting #address-cells to 0 in the GIC node should be functionally equivalent to your patch below, since newaddrsize will == 0.

yes, it does. I'll verify and post a followup patch tomorrow for the invalid dtsi's. Thanks for digging into this!

Tim