pSeries boot failure due to wrong interrupt controller

pSeries boot failure due to wrong interrupt controller

[Jeremy Linton]

The OF IRQ logic was refactored a while back, and it appears that it
broke the case where a device doesn't have an interrupt-map
(legacy/ISA platform peripherals). In particular on the older pSeries
platforms the console serial port (keyboard/etc) is attached via a
Winbond super I/O controller. With the refactored logic
of_irq_parse_raw() returns with the out_irq->np pointing at the bus,
rather than the interrupt controller. This results in the platform
logic getting confused about the appropriate interrupt host, and in
ics_rtas_host_match() it doesn't match "chrp,iic" and the xics takes
over and tries to use RTAS to map the interrupt. RTAS then crashes the
machine.

This tweaks the "-> no map, getting parent" behavior to behave as
before, and the machine boots.

Thanks

Signed-off-by: Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1a79806..78b4161b 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -252,7 +252,6 @@ int of_irq_parse_raw(const __be32 *addr, struct
of_phandle_args *out_irq)
                 * Successfully parsed an interrrupt-map translation; copy new
                 * interrupt specifier into the out_irq structure
                 */
-               out_irq->np = newpar;

                match_array = imap - newaddrsize - newintsize;
                for (i = 0; i < newintsize; i++)
@@ -262,6 +261,7 @@ int of_irq_parse_raw(const __be32 *addr, struct
of_phandle_args *out_irq)

        skiplevel:
                /* Iterate again with new parent */
+               out_irq->np=newpar;
                pr_debug(" -> new parent: %s\n", of_node_full_name(newpar));
                of_node_put(ipar);
                ipar = newpar;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in

Re: pSeries boot failure due to wrong interrupt controller

[Rob Herring]

+Grant

On Mon, Jun 22, 2015 at 2:44 PM, Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> The OF IRQ logic was refactored a while back, and it appears that it
> broke the case where a device doesn't have an interrupt-map
> (legacy/ISA platform peripherals). In particular on the older pSeries
> platforms the console serial port (keyboard/etc) is attached via a
> Winbond super I/O controller. With the refactored logic
> of_irq_parse_raw() returns with the out_irq->np pointing at the bus,
> rather than the interrupt controller. This results in the platform
> logic getting confused about the appropriate interrupt host, and in
> ics_rtas_host_match() it doesn't match "chrp,iic" and the xics takes
> over and tries to use RTAS to map the interrupt. RTAS then crashes the
> machine.
>
> This tweaks the "-> no map, getting parent" behavior to behave as
> before, and the machine boots.

I think the real question is why doesn't of_irq_find_parent return the
interrupt controller node instead of the bus node.

While I think this fix is correct, I think of_irq_parse_raw should be
a nop in the case of no interrupt-map.

Rob

>
> Thanks
>
> Signed-off-by: Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 1a79806..78b4161b 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -252,7 +252,6 @@ int of_irq_parse_raw(const __be32 *addr, struct
> of_phandle_args *out_irq)
>                  * Successfully parsed an interrrupt-map translation; copy new
>                  * interrupt specifier into the out_irq structure
>                  */
> -               out_irq->np = newpar;
>
>                 match_array = imap - newaddrsize - newintsize;
>                 for (i = 0; i < newintsize; i++)
> @@ -262,6 +261,7 @@ int of_irq_parse_raw(const __be32 *addr, struct
> of_phandle_args *out_irq)
>
>         skiplevel:
>                 /* Iterate again with new parent */
> +               out_irq->np=newpar;
>                 pr_debug(" -> new parent: %s\n", of_node_full_name(newpar));
>                 of_node_put(ipar);
>                 ipar = newpar;
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pSeries boot failure due to wrong interrupt controller

[Jeremy Linton]

Ping? Anyone?

I'm adding ppc-dev to see if anyone there is interested in picking up the patch..

Thanks,
	Jeremy


Ok so I messed up the formatting a little and was lazy and didn't checkpatch it
cause I mean, it was 1 line! If someone wants it resent I will do so.





On 6/23/2015 8:42 AM, Rob Herring wrote:
> +Grant
> 
> On Mon, Jun 22, 2015 at 2:44 PM, Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> The OF IRQ logic was refactored a while back, and it appears that it
>> broke the case where a device doesn't have an interrupt-map
>> (legacy/ISA platform peripherals). In particular on the older pSeries
>> platforms the console serial port (keyboard/etc) is attached via a
>> Winbond super I/O controller. With the refactored logic
>> of_irq_parse_raw() returns with the out_irq->np pointing at the bus,
>> rather than the interrupt controller. This results in the platform
>> logic getting confused about the appropriate interrupt host, and in
>> ics_rtas_host_match() it doesn't match "chrp,iic" and the xics takes
>> over and tries to use RTAS to map the interrupt. RTAS then crashes the
>> machine.
>>
>> This tweaks the "-> no map, getting parent" behavior to behave as
>> before, and the machine boots.
> 
> I think the real question is why doesn't of_irq_find_parent return the
> interrupt controller node instead of the bus node.
> 
> While I think this fix is correct, I think of_irq_parse_raw should be
> a nop in the case of no interrupt-map.
> 
> Rob
> 
>>
>> Thanks
>>
>> Signed-off-by: Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>>
>> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> index 1a79806..78b4161b 100644
>> --- a/drivers/of/irq.c
>> +++ b/drivers/of/irq.c
>> @@ -252,7 +252,6 @@ int of_irq_parse_raw(const __be32 *addr, struct
>> of_phandle_args *out_irq)
>>                  * Successfully parsed an interrrupt-map translation; copy new
>>                  * interrupt specifier into the out_irq structure
>>                  */
>> -               out_irq->np = newpar;
>>
>>                 match_array = imap - newaddrsize - newintsize;
>>                 for (i = 0; i < newintsize; i++)
>> @@ -262,6 +261,7 @@ int of_irq_parse_raw(const __be32 *addr, struct
>> of_phandle_args *out_irq)
>>
>>         skiplevel:
>>                 /* Iterate again with new parent */
>> +               out_irq->np = newpar;
>>                 pr_debug(" -> new parent: %s\n", of_node_full_name(newpar));
>>                 of_node_put(ipar);
>>                 ipar = newpar;

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pSeries boot failure due to wrong interrupt controller

[Benjamin Herrenschmidt]

On Fri, 2015-06-26 at 12:54 -0500, Jeremy Linton wrote:
> Ping? Anyone?
> 
> I'm adding ppc-dev to see if anyone there is interested in picking up the patch..

Ah I missed that completely. From your discussion, it looks correct,
I'll see if I can find a machine to test, however I think we dropped
support for POWER3 so it might be non-trivial to do so.

And yes, when there is no map, the parent should be the interrupt-parent
with a fallback to the parent node if that doesn't exist.

Cheers,
Ben.

> Thanks,
> 	Jeremy
> 
> 
> Ok so I messed up the formatting a little and was lazy and didn't checkpatch it
> cause I mean, it was 1 line! If someone wants it resent I will do so.
> 
> 
> 
> 
> 
> On 6/23/2015 8:42 AM, Rob Herring wrote:
> > +Grant
> > 
> > On Mon, Jun 22, 2015 at 2:44 PM, Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> The OF IRQ logic was refactored a while back, and it appears that it
> >> broke the case where a device doesn't have an interrupt-map
> >> (legacy/ISA platform peripherals). In particular on the older pSeries
> >> platforms the console serial port (keyboard/etc) is attached via a
> >> Winbond super I/O controller. With the refactored logic
> >> of_irq_parse_raw() returns with the out_irq->np pointing at the bus,
> >> rather than the interrupt controller. This results in the platform
> >> logic getting confused about the appropriate interrupt host, and in
> >> ics_rtas_host_match() it doesn't match "chrp,iic" and the xics takes
> >> over and tries to use RTAS to map the interrupt. RTAS then crashes the
> >> machine.
> >>
> >> This tweaks the "-> no map, getting parent" behavior to behave as
> >> before, and the machine boots.
> > 
> > I think the real question is why doesn't of_irq_find_parent return the
> > interrupt controller node instead of the bus node.
> > 
> > While I think this fix is correct, I think of_irq_parse_raw should be
> > a nop in the case of no interrupt-map.
> > 
> > Rob
> > 
> >>
> >> Thanks
> >>
> >> Signed-off-by: Jeremy Linton <lintonrjeremy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >>
> >> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> >> index 1a79806..78b4161b 100644
> >> --- a/drivers/of/irq.c
> >> +++ b/drivers/of/irq.c
> >> @@ -252,7 +252,6 @@ int of_irq_parse_raw(const __be32 *addr, struct
> >> of_phandle_args *out_irq)
> >>                  * Successfully parsed an interrrupt-map translation; copy new
> >>                  * interrupt specifier into the out_irq structure
> >>                  */
> >> -               out_irq->np = newpar;
> >>
> >>                 match_array = imap - newaddrsize - newintsize;
> >>                 for (i = 0; i < newintsize; i++)
> >> @@ -262,6 +261,7 @@ int of_irq_parse_raw(const __be32 *addr, struct
> >> of_phandle_args *out_irq)
> >>
> >>         skiplevel:
> >>                 /* Iterate again with new parent */
> >> +               out_irq->np = newpar;
> >>                 pr_debug(" -> new parent: %s\n", of_node_full_name(newpar));
> >>                 of_node_put(ipar);
> >>                 ipar = newpar;


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pSeries boot failure due to wrong interrupt controller

[Jeremy Linton]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On 6/26/2015 4:54 PM, Benjamin Herrenschmidt wrote:
> Ah I missed that completely. From your discussion, it looks correct, I'll
> see if I can find a machine to test, however I think we dropped support for
> POWER3 so it might be non-trivial to do so.

	Thanks, the problem originally cropped up on a power4+, I was going to test
it on power5 too, but I don't have access anymore. I do have a power3 as well,
but I think that machine has other issues (the BUG_ON in the platform table
for starters).


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)

iQEcBAEBAgAGBQJVjdPIAAoJEL5i86xrzcy7KA0H+QHVe3IFy12DlgxP0kbbaDtr
D5JLqh9OHBxZohotRzQz5errCIBYGxIYy93P6S2NvQSKS1gqZFFrvdyIs0f3UYo0
izzRVh/Gf10p0d3MJDI0EYC7OlxlP4hJ8lZKU2PNFQvhLT4eE1i/XJAySJLJsSpG
WD9CiP5Flixm/ECBPbyeg9nnc5c+EM4xPhE6J3Zp5pX7gU0cieJb2rzhz/7/TsI1
6AYMOPC4JpL3aK5z4eKuVgzvuJ/MyCjHdwii0TpNBhhhyidq8PEDoH3Y61CIOszq
Uu5HAQoToqBZdKM7siiZY0LJVQVpJaze4zhjexqhGa/Ll0iquj3V13xZ5qjj1eA=
=oeSx
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pSeries boot failure due to wrong interrupt controller

[Benjamin Herrenschmidt]

On Fri, 2015-06-26 at 17:35 -0500, Jeremy Linton wrote:
> On 6/26/2015 4:54 PM, Benjamin Herrenschmidt wrote:
> > Ah I missed that completely. From your discussion, it looks correct, I'll
> > see if I can find a machine to test, however I think we dropped support for
> > POWER3 so it might be non-trivial to do so.
> 
> 	Thanks, the problem originally cropped up on a power4+, I was going to test
> it on power5 too, but I don't have access anymore. I do have a power3 as well,
> but I think that machine has other issues (the BUG_ON in the platform table
> for starters).

Ok, I don't think I still have one of these, but if you re-send the
patch with S-O-B etc... I'll review & ack it.

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html