| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AANLkTi=dU4oYfqrBGYmSCmYf2cRsBX-Pf6A9X9N0YaZA@mail.gmail.com>
Date: Thu, 7 Oct 2010 12:34:09 -0600
From: Grant Likely <grant.likely@...retlab.ca>
To: John Williams <john.williams@...alogix.com>
Cc: Linux Kernel list <linux-kernel@...r.kernel.org>,
linuxppc-dev@...abs.org, Michal Simek <michal.simek@...alogix.com>,
devicetree-discuss <devicetree-discuss@...ts.ozlabs.org>
Subject: Re: PPC: Possible bug in prom_parse.c:of_irq_map_raw?
Reaching way back into the past....
John, did you ever solve your issue here? Comments below.
On Tue, Feb 9, 2010 at 8:21 PM, John Williams
<john.williams@...alogix.com> wrote:
> Hi,
>
> Perhaps this is my misunderstanding, but I'm looking at the bit of
> code in of_irq_map_raw() that iterates the interrupt-map DTS node,
> which I am seeing to behave strangely when handling the interrupt-map
> property on a PCI bridge node.
>
> Early in the function, we get the #address-cells value from the node -
> in this case the PCI bridge, and store it in local var addrsize:
>
> /* Look for this #address-cells. We have to implement the old linux
> * trick of looking for the parent here as some device-trees rely on it
> */
> old = of_node_get(ipar);
> do {
> tmp = of_get_property(old, "#address-cells", NULL);
> tnode = of_get_parent(old);
> of_node_put(old);
> old = tnode;
> } while(old && tmp == NULL);
> of_node_put(old);
> old = NULL;
> addrsize = (tmp == NULL) ? 2 : *tmp;
>
> DBG(" -> addrsize=%d\n", addrsize);
>
>
> Later, once we've found the interrupt-map and are iterating over it
> attempting to match on PCI device addresses, there is this little
> fragment:
>
> /* Get the interrupt parent */
> if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
> newpar = of_node_get(of_irq_dflt_pic);
> else
> newpar =
> of_find_node_by_phandle((phandle)*imap);
> imap++;
> --imaplen;
>
> /* Check if not found */
> if (newpar == NULL) {
> DBG(" -> imap parent not found !\n");
> goto fail;
> }
>
> /* Get #interrupt-cells and #address-cells of new
> * parent
> */
> tmp = of_get_property(newpar, "#interrupt-cells", NULL);
> if (tmp == NULL) {
> DBG(" -> parent lacks #interrupt-cells !\n");
> goto fail;
> }
> newintsize = *tmp;
> tmp = of_get_property(newpar, "#address-cells", NULL);
> newaddrsize = (tmp == NULL) ? 0 : *tmp;
>
> Finally, later in the loop we update addrsize=newaddrsize
>
> As I read this code, and the behaviour I'm seeing, is that if the
> interrupt controller parent device doesn't have an #address-cells
> entry, then this get_property will return fail, therefore setting
> newaddrsize to zero. This then means that subsequent match attempts
> when iterating the interrupt-map always fail, because the match length
> (addrsize) is 0.
Correct. The interrupt-map property contains the following fields:
child-unit-address child-irq irq-controller irq-parent-unit-address parent-irq
In the *vast majority* of cases, the irq-parent-unit-address is a
zero-length field because #address-cells isn't present on the
interrupt controller parent. So effectively interrupt-map becomes:
child-unit-address child-irq irq-controller parent-irq
See epapr 1.0 for a full discussion
>
> Maybe I'm missing something here, but shouldn't this last bit of code
> instead be:
>
> tmp = of_get_property(newpar, "#address-cells", NULL);
> newaddrsize = (tmp == NULL) ? addrsize : *tmp;
>
> ^^^^^^^^^
> ?
>
> In other words, if the parent node has an #address-cells value, then
> use it, otherwise stick to the #address-cells value we picked up for
> the actual node in question (ie the PCI bridge).
No, because at this point we absolutely do want to know how big the
parent #address-cells is, and if it is missing, we need to use 0. If
the child's addrsize continued to be used, then the interrupt-map
parsing would get unaligned.
The inner loop is over the entries in interrupt-map. addrsize and
intsize are only updated in the case where a match is found in the
table. If a match isn't found, then it should bail out to the 'fail'
label.
> The way this is manifesting itself in the system I'm looking at is
> that only the PCI device matching the first entry in the PCI bridge
> interrupt-map property is correctly matching. For PCI devices that
> require more than one pass through the interrupt-map loop, addrsize
> goes to zero and no further match is possible.
Something sounds fishy. If you're still having problems, can you
enable #define DEBUG in drivers/of/irq.c and post the output?
> I might be able to hack around this in the device-tree by setting
> #address-cells=<3> in the interrupt controller node, but that doesn't
> smell right either - it's only true for PCI devices for a start,
> whereas this controller handles interrupts from many sources on the
> 32-bit PLB bus as well. I looked at the ML510 DTS in boot/dts and
> it's not doing anything like this.
Yeah, that's not right, and it would mess up the interrupt-map
parsing. You'd end up with more hard to debug problems.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists