[PATCH 0/7] PCI irq mapping fixes and cleanups

[Tim Harvey]

On Mon, Mar 3, 2014 at 4:01 PM, Jason Gunthorpe
<jgunthorpe at obsidianresearch.com> wrote:
> On Mon, Mar 03, 2014 at 03:40:43PM -0800, Tim Harvey wrote:
>
>> of_irq_parse_and_map_pci().  The GIC function that translates the
>> interrupt per domain is given irq_data: 0x123 0x04 0x00
>
> This has been shifted by 1 byte..
>
>> IRQ 123, which should get 32 added to it for irq155).  Instead, the
>> implementation of gic_irq_domain_xlate()
>> (http://lxr.missinglinkelectronics.com/linux/drivers/irqchip/irq-gic.c#L832)
>> adds 32 to the 0x04 returning 20:
>> [    1.841781] of_irq_parse_raw:  /soc/pcie at 0x01000000:00000001
>> [    1.841813] of_irq_parse_raw: ipar=/soc/pcie at 0x01000000, size=1
>> [    1.841838]  -> addrsize=3
>> [    1.841870]  -> match=1 (imaplen=28)
>                               ^^^^^^^^^^^^^
>
> That looks odd, it should be the number of dwords in the
> interrupt-map, you have 4 lines of 8 dwords each, so it should be
> 32.

(+cc Grant Likely)

imaplen does indeed get initialized to 32 (size of interrupt-map /
sizeof(u32)) but its printed above after its been decremented in the
loop which is misleading
(http://lxr.missinglinkelectronics.com/linux/drivers/of/irq.c#L201)

The issue appears to me to be a bug in of_irq_parse_raw() which has
been around since Graht's original commit:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/of/irq.c?id=7dc2e1134a22dc242175d5321c0c9e97d16eb87b

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
                        /* 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);
                }

The issue is that the interrupt-map table point (imap) needs to be
incremented over the parent unit interrupt specifier which is
newintsize cells, not newaddrsize + newintsize cells.  The invalid
calculation would cause the pointer to get mis-aligned and thus only
the first interrupt entry would ever get properly checked for a match.

It looks like of_irq_parse_raw() is only called from
of_irq_parse_pci() which prior to Lucas' patch was only called from
pci_read_irq_line() called from pcibios_setup_device() used in
arch/arm/powerpc, so perhaps this function isn't widely used
explaining why the bug was never caught.

I'll post a patch shortly with the above fix.

<snip>

>> [    1.841972] irq_create_of_mapping: calling xlate for 123/4/0 3
>
> And it is the wrong data.. 123/4/0 is

right - this is shifted because of the issue above.

With the above patch Lucas' original patch now operates correctly to
resolve the 4 legacy PCI interrupts required when using a P2P bridge
on the IMX6.

Tim