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

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

[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.

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 <tharvey@gateworks.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Jingoo Han <jg1.han@samsung.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-samsung-soc <linux-samsung-soc@vger.kernel.org>
Cc: Richard Zhu <r65037@freescale.com>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: linux-tegra <linux-tegra@vger.kernel.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Grant Likely <grant.likely@linaro.org>
---
 drivers/of/irq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 9bcf2cf..8829197 100644
--- a/drivers/of/irq.c
+++ 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);
 		}
-- 
1.8.3.2

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

[Tim Harvey]

On Tue, Mar 4, 2014 at 6:54 AM, Tim Harvey <tharvey@gateworks.com> wrote:
> 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.
>

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

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

[Jason Gunthorpe]

On Tue, Mar 04, 2014 at 06:54:24AM -0800, Tim Harvey wrote:
> 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:

--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -55,7 +55,6 @@ Example:
        intc: interrupt-controller@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

> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> 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);
>  		}

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

[Tim Harvey]

On Tue, Mar 11, 2014 at 1:15 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Tue, Mar 04, 2014 at 06:54:24AM -0800, Tim Harvey wrote:
>> 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:
>
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -55,7 +55,6 @@ Example:
>         intc: interrupt-controller@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.

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

>
> 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.

>
> 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.

>
> 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