Xilinx PIC and kernel interrupt handler

Xilinx PIC and kernel interrupt handler

[Stig Telfer]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

Hi -

There appears to be a kernel bug in the 2.4 and 2.6.17.7 kernel trees 
relating to reading the interrupt vector from the Xilinx PIC 
(xilinx_pic_get_irq() in xilinx_pic.c).  As I see it, here's the 
problem: If no interrupt is pending, the register should read all ones. 
  However, the IVR is only as wide as the number of interrupt sources.  
The routine mistakenly assumes sign extension and checks for a 32-bit 
read of -1 instead of a read of w bits where w is the number of 
connected interrupt sources.

The 2.6 version also has a search-and-replace glitch relating to 
removal of the reversal of bit numbering.  I have attached a two line 
patch (for 2.6.17.7) that makes the IVR comparison against the right 
bit pattern and removes the remnants of the former bit-reversal code.

Share and enjoy,
Stig Telfer


[-- Attachment #2: xilinx_pic.patch --]
[-- Type: application/octet-stream, Size: 475 bytes --]

diff -Naurb linux-2.6.17.7/arch/ppc/syslib/xilinx_pic.c linux-2.6.17.7-stig/arch/ppc/syslib/xilinx_pic.c
--- linux-2.6.17.7/arch/ppc/syslib/xilinx_pic.c	2006-07-25 04:36:01.000000000 +0100
+++ linux-2.6.17.7-stig/arch/ppc/syslib/xilinx_pic.c	2006-07-31 14:20:04.000000000 +0100
@@ -97,8 +97,8 @@
 	 */
 
 	irq = intc_in_be32(intc + IVR);
-	if (irq != -1)
-		irq = irq;
+	if (irq == (1 << XPAR_INTC_MAX_NUM_INTR_INPUTS) - 1)
+		irq = -1;
 
 	pr_debug("get_irq: %d\n", irq);
 

RE: Xilinx PIC and kernel interrupt handler

[Martin, Tim]

Regarding the IVR patch:  Have you seen this bug in practice, or just
from examining the code?

The reason I ask is I've recently looked at this myself, and was under
the impression that "w" is the width of the data bus (DB) (per page 9 of
dcr_intc.pdf).  So regardless of how many interrupt sources are
connected, assuming the data bus width is 32 bits, w=3D32.

I've specifically confirmed this is true if you have less than 32
interrupt sources connected, the one named INT0 shows up in bit position
31 (w-1 for w=3D32 is 31) of the ISR and IPR.  Bit position 31 in PPC
notation is the LSB.

Tim=20

> -----Original Message-----
> From: linuxppc-embedded-bounces+tmartin=3Dviasat.com@ozlabs.org=20
> [mailto:linuxppc-embedded-bounces+tmartin=3Dviasat.com@ozlabs.or
> g] On Behalf Of Stig Telfer
> Sent: Monday, July 31, 2006 6:51 AM
> To: linuxppc-embedded@ozlabs.org
> Subject: Xilinx PIC and kernel interrupt handler=20
>=20
> Hi -
>=20
> There appears to be a kernel bug in the 2.4 and 2.6.17.7=20
> kernel trees relating to reading the interrupt vector from=20
> the Xilinx PIC
> (xilinx_pic_get_irq() in xilinx_pic.c).  As I see it, here's the
> problem: If no interrupt is pending, the register should read=20
> all ones.=20
>   However, the IVR is only as wide as the number of interrupt=20
> sources. =20
> The routine mistakenly assumes sign extension and checks for=20
> a 32-bit read of -1 instead of a read of w bits where w is=20
> the number of connected interrupt sources.
>=20
> The 2.6 version also has a search-and-replace glitch relating=20
> to removal of the reversal of bit numbering.  I have attached=20
> a two line patch (for 2.6.17.7) that makes the IVR comparison=20
> against the right bit pattern and removes the remnants of the=20
> former bit-reversal code.
>=20
> Share and enjoy,
> Stig Telfer
>=20
>=20

Re: Xilinx PIC and kernel interrupt handler

[Stig Telfer]

Hi Tim -

You are right, there are many references in the INTC doc to w meaning 
the data bus width.  I think I have mis-read the IVR section.

However, the patch was based on real-world observations.  Under high 
system activity I occasionally see IVR reads returning 0x7F, and our 
system has 7 interrupts connected to that PIC.  That's where my 
hypothesis about the bit-extension came from.

As an aside, an immediate second read of the IVR returns a valid vector 
number to service.  Curious on a uniprocessor system...

Regards,
Stig


On 31 Jul 2006, at 19:06, Martin, Tim wrote:

> Regarding the IVR patch:  Have you seen this bug in practice, or just
> from examining the code?
>
> The reason I ask is I've recently looked at this myself, and was under
> the impression that "w" is the width of the data bus (DB) (per page 9 
> of
> dcr_intc.pdf).  So regardless of how many interrupt sources are
> connected, assuming the data bus width is 32 bits, w=32.
>
> I've specifically confirmed this is true if you have less than 32
> interrupt sources connected, the one named INT0 shows up in bit 
> position
> 31 (w-1 for w=32 is 31) of the ISR and IPR.  Bit position 31 in PPC
> notation is the LSB.
>
> Tim
>
>> -----Original Message-----
>> From: linuxppc-embedded-bounces+tmartin=viasat.com@ozlabs.org
>> [mailto:linuxppc-embedded-bounces+tmartin=viasat.com@ozlabs.or
>> g] On Behalf Of Stig Telfer
>> Sent: Monday, July 31, 2006 6:51 AM
>> To: linuxppc-embedded@ozlabs.org
>> Subject: Xilinx PIC and kernel interrupt handler
>>
>> Hi -
>>
>> There appears to be a kernel bug in the 2.4 and 2.6.17.7
>> kernel trees relating to reading the interrupt vector from
>> the Xilinx PIC
>> (xilinx_pic_get_irq() in xilinx_pic.c).  As I see it, here's the
>> problem: If no interrupt is pending, the register should read
>> all ones.
>>   However, the IVR is only as wide as the number of interrupt
>> sources.
>> The routine mistakenly assumes sign extension and checks for
>> a 32-bit read of -1 instead of a read of w bits where w is
>> the number of connected interrupt sources.
>>
>> The 2.6 version also has a search-and-replace glitch relating
>> to removal of the reversal of bit numbering.  I have attached
>> a two line patch (for 2.6.17.7) that makes the IVR comparison
>> against the right bit pattern and removes the remnants of the
>> former bit-reversal code.
>>
>> Share and enjoy,
>> Stig Telfer
>>
>>