diff mbox

[v8,0/4] arm: KGDB NMI/FIQ support

Message ID 53C4F745.3070701@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Thompson July 15, 2014, 9:41 a.m. UTC
On 14/07/14 14:51, Harro Haan wrote:
> On 10 July 2014 10:03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>
>> This patchset makes it possible to use kgdb's NMI infrastructure on ARM
>> platforms.
>>
>> The patches have been previously circulated as part of a large patchset
>> mixing together ARM architecture code and driver changes
>> (http://thread.gmane.org/gmane.linux.ports.arm.kernel/333901 ). This
>> patchset is dramatically cut down to include only the arch/arm code. The
>> driver code (irqchip and tty/serial) will follow when/if the arch code
>> is accepted.
>>
>> The first two patches modify the FIQ infrastructure to allow interrupt
>> controller drivers to register callbacks (the fiq_chip structure) to
>> manage FIQ routings and to ACK and EOI the FIQ. This makes it possible
>> to use FIQ in multi-platform kernels and with recent ARM interrupt
>> controllers.
>>
> 
> Daniel,
> 
> Thanks for the patches. I have tested the fiq and irq-gic patches on
> an i.MX6 (SabreSD board) for a different purpose:
> A FIQ timer interrupt at 1 kHz. The TWD watchdog block is used in
> timer mode for this (interrupt ID 30). The GIC affinity is set to CPU
> core 0 only for this interrupt ID.
> 
> I was surprised by the following behavior:
> $ cat /proc/interrupts
>            CPU0       CPU1       CPU2       CPU3
>  29:       5459       3381       3175       3029       GIC  29  twd
>  30:         11          0          0          0       GIC  30  fake-fiq
> 
> Once every few seconds is interrupt ID 30 handled by the regular GIC
> handler instead of the FIQ exception path (which causes for example a
> bit more latencies). The other thousands of FIQ's are handled by the
> FIQ exception path. The problem is also confirmed by the following
> stackframe of the Lauterbach tooling:
> fake_fiq_handler(irq = 30, ...)
> handle_percpu_devid_irq(irq = 30, ...)
> generic_handle_irq(irq = 30)
> handle_IRQ(irq = 30, ...)
> gic_handle_irq
> __irq_svc
> -->exception
> 
> Notes:
> - The problem will occur more often by executing the following command:
>   $ while true; do hackbench 20; done &
> - Reading the GIC_CPU_INTACK register returns the interrupt ID of the
> highest priority pending interrupt.
> - The GIC_CPU_INTACK register (used by fiq_ack) will return spurious
> interrupt ID 0x3FF when read in fake_fiq_handler, because
> GIC_CPU_INTACK is already read by gic_handle_irq.
> - The FIQ exception will not occur anymore after gic_handle_irq
> read/acknowledges the FIQ by reading the GIC_CPU_INTACK register
> 
> From the behavior above I conclude that the GIC_CPU_INTACK register is
> first updated before the FIQ exception is generated, so there is a
> small period of time that gic_handle_irq can read/acknowledge the FIQ.

Makes sense.

Avoiding this problem on GICv2 is easy (thanks to the aliased intack
register) but on iMX.6 we have only a GICv1.


> I can reduce the number of occurrences (not prevent it) by adding the
> following hack to irq-gic.c
> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
> gic_handle_irq(struct pt_regs *regs
>   u32 irqstat, irqnr;
>   struct gic_chip_data *gic = &gic_data[0];
>   void __iomem *cpu_base = gic_data_cpu_base(gic);
> 
>   do {
> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
> & (1 << 30))
> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>   irqnr = irqstat & ~0x1c00;

I've made a more complete attempt to fix this. Could you test the
following? (and be prepared to fuzz the line numbers)

 {
 	u32 irqstat, irqnr;
@@ -310,8 +332,10 @@ static void __exception_irq_entry
gic_handle_irq(struct pt_regs *regs)
 	void __iomem *cpu_base = gic_data_cpu_base(gic);

 	do {
+		local_fiq_disable();
 		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
-		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
+		irqnr = gic_handle_spurious_group_0(gic, irqstat);
+		local_fiq_enable();

 		if (likely(irqnr > 15 && irqnr < 1021)) {
 			irqnr = irq_find_mapping(gic->domain, irqnr);

Comments

Harro Haan July 15, 2014, 1:04 p.m. UTC | #1
On 15 July 2014 11:41, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 14/07/14 14:51, Harro Haan wrote:
>> On 10 July 2014 10:03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>>
>>> This patchset makes it possible to use kgdb's NMI infrastructure on ARM
>>> platforms.
>>>
>>> The patches have been previously circulated as part of a large patchset
>>> mixing together ARM architecture code and driver changes
>>> (http://thread.gmane.org/gmane.linux.ports.arm.kernel/333901 ). This
>>> patchset is dramatically cut down to include only the arch/arm code. The
>>> driver code (irqchip and tty/serial) will follow when/if the arch code
>>> is accepted.
>>>
>>> The first two patches modify the FIQ infrastructure to allow interrupt
>>> controller drivers to register callbacks (the fiq_chip structure) to
>>> manage FIQ routings and to ACK and EOI the FIQ. This makes it possible
>>> to use FIQ in multi-platform kernels and with recent ARM interrupt
>>> controllers.
>>>
>>
>> Daniel,
>>
>> Thanks for the patches. I have tested the fiq and irq-gic patches on
>> an i.MX6 (SabreSD board) for a different purpose:
>> A FIQ timer interrupt at 1 kHz. The TWD watchdog block is used in
>> timer mode for this (interrupt ID 30). The GIC affinity is set to CPU
>> core 0 only for this interrupt ID.
>>
>> I was surprised by the following behavior:
>> $ cat /proc/interrupts
>>            CPU0       CPU1       CPU2       CPU3
>>  29:       5459       3381       3175       3029       GIC  29  twd
>>  30:         11          0          0          0       GIC  30  fake-fiq
>>
>> Once every few seconds is interrupt ID 30 handled by the regular GIC
>> handler instead of the FIQ exception path (which causes for example a
>> bit more latencies). The other thousands of FIQ's are handled by the
>> FIQ exception path. The problem is also confirmed by the following
>> stackframe of the Lauterbach tooling:
>> fake_fiq_handler(irq = 30, ...)
>> handle_percpu_devid_irq(irq = 30, ...)
>> generic_handle_irq(irq = 30)
>> handle_IRQ(irq = 30, ...)
>> gic_handle_irq
>> __irq_svc
>> -->exception
>>
>> Notes:
>> - The problem will occur more often by executing the following command:
>>   $ while true; do hackbench 20; done &
>> - Reading the GIC_CPU_INTACK register returns the interrupt ID of the
>> highest priority pending interrupt.
>> - The GIC_CPU_INTACK register (used by fiq_ack) will return spurious
>> interrupt ID 0x3FF when read in fake_fiq_handler, because
>> GIC_CPU_INTACK is already read by gic_handle_irq.
>> - The FIQ exception will not occur anymore after gic_handle_irq
>> read/acknowledges the FIQ by reading the GIC_CPU_INTACK register
>>
>> From the behavior above I conclude that the GIC_CPU_INTACK register is
>> first updated before the FIQ exception is generated, so there is a
>> small period of time that gic_handle_irq can read/acknowledge the FIQ.
>
> Makes sense.
>
> Avoiding this problem on GICv2 is easy (thanks to the aliased intack
> register) but on iMX.6 we have only a GICv1.
>
>
>> I can reduce the number of occurrences (not prevent it) by adding the
>> following hack to irq-gic.c
>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>> gic_handle_irq(struct pt_regs *regs
>>   u32 irqstat, irqnr;
>>   struct gic_chip_data *gic = &gic_data[0];
>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>
>>   do {
>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>> & (1 << 30))
>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>   irqnr = irqstat & ~0x1c00;
>
> I've made a more complete attempt to fix this. Could you test the
> following? (and be prepared to fuzz the line numbers)

Thanks Daniel, I have tested it, see the comments below.

>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 73ae896..309bf2c 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -303,6 +303,28 @@ static int gic_set_wake(struct irq_data *d,
> unsigned int on)
>  #define gic_set_wake   NULL
>  #endif
>
> +/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
> + * workaround will only work for level triggered interrupts (and in
> + * its current form is actively harmful on systems that don't support
> + * FIQ).
> + */
> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32
> irqstat)
> +{
> +       u32 irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> +
> +       if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
> +               return irqnr;
> +
> +       if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
> +                         (irqnr / 32 * 4)) &
> +           BIT(irqnr % 32))
> +               return irqnr;
> +
> +       /* this interrupt was spurious (needs to be handled as FIQ) */
> +       writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);

This will NOT work, because of the note I mentioned above:
"The FIQ exception will not occur anymore after gic_handle_irq
read/acknowledges the FIQ by reading the GIC_CPU_INTACK register"
So with this code you will say End Of Interrupt at the GIC level,
without actually handling the interrupt, so you are missing an
interrupt.
I did the following test to confirm the missing interrupt:
I have changed the periodic timer interrupt by an one-shot timer
interrupt. The one-shot timer interrupt is programmed by the FIQ
handler for the next FIQ interrupt. As expected: When the problem
occurs, no more FIQ interrupts are generated.

> +       return 1023;
> +}
> +
>  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>  {
>         u32 irqstat, irqnr;
> @@ -310,8 +332,10 @@ static void __exception_irq_entry
> gic_handle_irq(struct pt_regs *regs)
>         void __iomem *cpu_base = gic_data_cpu_base(gic);
>
>         do {
> +               local_fiq_disable();

It is a bit weird to disable the "Non-Maskable Interrupt" at every
interrupt, because of a problem that occurs once every few thousand
interrupts

>                 irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> -               irqnr = irqstat & GICC_IAR_INT_ID_MASK;
> +               irqnr = gic_handle_spurious_group_0(gic, irqstat);
> +               local_fiq_enable();
>
>                 if (likely(irqnr > 15 && irqnr < 1021)) {
>                         irqnr = irq_find_mapping(gic->domain, irqnr);


The following type of changes could fix the problem for me:

@@ -290,19 +290,66 @@ static int gic_set_wake(struct irq_data *d,
unsigned int on)

 #else
 #define gic_set_wake NULL
 #endif

+extern void (*fiq_handler)(void);
+
+/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
+ * workaround will only work for level triggered interrupts (and in
+ * its current form is actively harmful on systems that don't support
+ * FIQ).
+ */
+static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32 irqstat)
+{
+ u32 irqnr = irqstat & ~0x1c00;
+
+ if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
+ return irqnr;
+
+ if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
+  (irqnr / 32 * 4)) & BIT(irqnr % 32))
+ return irqnr;
+
+ /*
+ * The FIQ should be disabled before the next FIQ interrupt occurs,
+ * so this only works when the next FIQ interrupt is not "too fast"
+ * after the previous one.
+ */
+ local_fiq_disable();
+
+ /*
+ * Notes:
+ * - The FIQ exception will not occur anymore for this current
+ *   interrupt, because gic_handle_irq has already read/acknowledged
+ *   the GIC_CPU_INTACK register. So handle the FIQ here.
+ * - The fiq_handler below should not call ack_fiq and eoi_fiq,
+ *   because rereading the GIC_CPU_INTACK register returns spurious
+ *   interrupt ID 0x3FF. So probably you will have to add sometime like
+ *   the following to fiq_handler:
+ *   u32 cpsr, irqstat;
+ *   __asm__("mrs %0, cpsr" : "=r" (cpsr));
+ *   if ((cpsr & MODE_MASK) == FIQ_MODE)
+ *   irqstat = ack_fiq();
+ */
+ (*fiq_handler)();
+
+ writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);
+ local_fiq_enable();
+
+ return 1023;
+}
+
 static asmlinkage void __exception_irq_entry gic_handle_irq(struct
pt_regs *regs)
 {
  u32 irqstat, irqnr;
  struct gic_chip_data *gic = &gic_data[0];
  void __iomem *cpu_base = gic_data_cpu_base(gic);

  do {
  irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
- irqnr = irqstat & ~0x1c00;
+ irqnr = gic_handle_spurious_group_0(gic, irqstat);

  if (likely(irqnr > 15 && irqnr < 1021)) {

Regards,

Harro
Daniel Thompson July 15, 2014, 2:52 p.m. UTC | #2
On 15/07/14 14:04, Harro Haan wrote:
>> Makes sense.
>>
>> Avoiding this problem on GICv2 is easy (thanks to the aliased intack
>> register) but on iMX.6 we have only a GICv1.
>>
>>
>>> I can reduce the number of occurrences (not prevent it) by adding the
>>> following hack to irq-gic.c
>>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>>> gic_handle_irq(struct pt_regs *regs
>>>   u32 irqstat, irqnr;
>>>   struct gic_chip_data *gic = &gic_data[0];
>>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>
>>>   do {
>>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>>> & (1 << 30))
>>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>>   irqnr = irqstat & ~0x1c00;
>>
>> I've made a more complete attempt to fix this. Could you test the
>> following? (and be prepared to fuzz the line numbers)
> 
> Thanks Daniel, I have tested it, see the comments below.
> 
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 73ae896..309bf2c 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -303,6 +303,28 @@ static int gic_set_wake(struct irq_data *d,
>> unsigned int on)
>>  #define gic_set_wake   NULL
>>  #endif
>>
>> +/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
>> + * workaround will only work for level triggered interrupts (and in
>> + * its current form is actively harmful on systems that don't support
>> + * FIQ).
>> + */
>> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32
>> irqstat)
>> +{
>> +       u32 irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>> +
>> +       if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
>> +               return irqnr;
>> +
>> +       if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
>> +                         (irqnr / 32 * 4)) &
>> +           BIT(irqnr % 32))
>> +               return irqnr;
>> +
>> +       /* this interrupt was spurious (needs to be handled as FIQ) */
>> +       writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);
> 
> This will NOT work, because of the note I mentioned above:
> "The FIQ exception will not occur anymore after gic_handle_irq
> read/acknowledges the FIQ by reading the GIC_CPU_INTACK register"
> So with this code you will say End Of Interrupt at the GIC level,
> without actually handling the interrupt, so you are missing an
> interrupt.
> I did the following test to confirm the missing interrupt:
> I have changed the periodic timer interrupt by an one-shot timer
> interrupt. The one-shot timer interrupt is programmed by the FIQ
> handler for the next FIQ interrupt. As expected: When the problem
> occurs, no more FIQ interrupts are generated.

Can you confirm whether your timer interrupts are configured level or
edge triggered? Or whether EOIing the GIC causes them to be cleared by
some other means.

A level triggered interrupt that hasn't been serviced should return the
pending state (shouldn't it?).


>> +       return 1023;
>> +}
>> +
>>  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>>  {
>>         u32 irqstat, irqnr;
>> @@ -310,8 +332,10 @@ static void __exception_irq_entry
>> gic_handle_irq(struct pt_regs *regs)
>>         void __iomem *cpu_base = gic_data_cpu_base(gic);
>>
>>         do {
>> +               local_fiq_disable();
> 
> It is a bit weird to disable the "Non-Maskable Interrupt" at every
> interrupt, because of a problem that occurs once every few thousand
> interrupts

Given that simply reading from GIC_CPU_INTACK has significantly
interferes with FIQ reception means that I'm not too worried about this.

Note also that leaving the FIQ unmasked increases worst case latency
here because once we get a group 0 interrupt back from intack then
spurious entry to the FIQ handler (which would see an ACK of 1023) just
wastes cycles.


>>                 irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>> -               irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>> +               irqnr = gic_handle_spurious_group_0(gic, irqstat);
>> +               local_fiq_enable();
>>
>>                 if (likely(irqnr > 15 && irqnr < 1021)) {
>>                         irqnr = irq_find_mapping(gic->domain, irqnr);
> 
> 
> The following type of changes could fix the problem for me:
> 
> @@ -290,19 +290,66 @@ static int gic_set_wake(struct irq_data *d,
> unsigned int on)
> 
>  #else
>  #define gic_set_wake NULL
>  #endif
> 
> +extern void (*fiq_handler)(void);
> +
> +/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
> + * workaround will only work for level triggered interrupts (and in
> + * its current form is actively harmful on systems that don't support
> + * FIQ).
> + */
> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32 irqstat)
> +{
> + u32 irqnr = irqstat & ~0x1c00;
> +
> + if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
> + return irqnr;
> +
> + if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
> +  (irqnr / 32 * 4)) & BIT(irqnr % 32))
> + return irqnr;
> +
> + /*
> + * The FIQ should be disabled before the next FIQ interrupt occurs,
> + * so this only works when the next FIQ interrupt is not "too fast"
> + * after the previous one.
> + */
> + local_fiq_disable();
> +
> + /*
> + * Notes:
> + * - The FIQ exception will not occur anymore for this current
> + *   interrupt, because gic_handle_irq has already read/acknowledged
> + *   the GIC_CPU_INTACK register. So handle the FIQ here.
> + * - The fiq_handler below should not call ack_fiq and eoi_fiq,
> + *   because rereading the GIC_CPU_INTACK register returns spurious
> + *   interrupt ID 0x3FF. So probably you will have to add sometime like
> + *   the following to fiq_handler:
> + *   u32 cpsr, irqstat;
> + *   __asm__("mrs %0, cpsr" : "=r" (cpsr));
> + *   if ((cpsr & MODE_MASK) == FIQ_MODE)
> + *   irqstat = ack_fiq();
> + */
> + (*fiq_handler)();

Any portable approach would have to switch to FIQ mode to run the
handler in order to provide the proper register banks for FIQ handlers
written in assembler.

If we can't get level triggering to work then we have to:

1. Write code to jump correctly into FIQ mode.

2. Modify the gic's ack_fiq() callback to automatically avoid reading
   intack when the workaround is deployed.

The above is why I wanted to see if we can make do with level triggering
(and automatic re-triggering for interrupts such as SGIs that are
cleared by EOI).
Harro Haan July 15, 2014, 3:59 p.m. UTC | #3
On 15 July 2014 16:52, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 15/07/14 14:04, Harro Haan wrote:
>>> Makes sense.
>>>
>>> Avoiding this problem on GICv2 is easy (thanks to the aliased intack
>>> register) but on iMX.6 we have only a GICv1.
>>>
>>>
>>>> I can reduce the number of occurrences (not prevent it) by adding the
>>>> following hack to irq-gic.c
>>>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>>>> gic_handle_irq(struct pt_regs *regs
>>>>   u32 irqstat, irqnr;
>>>>   struct gic_chip_data *gic = &gic_data[0];
>>>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>>
>>>>   do {
>>>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>>>> & (1 << 30))
>>>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>>>   irqnr = irqstat & ~0x1c00;
>>>
>>> I've made a more complete attempt to fix this. Could you test the
>>> following? (and be prepared to fuzz the line numbers)
>>
>> Thanks Daniel, I have tested it, see the comments below.
>>
>>>
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index 73ae896..309bf2c 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -303,6 +303,28 @@ static int gic_set_wake(struct irq_data *d,
>>> unsigned int on)
>>>  #define gic_set_wake   NULL
>>>  #endif
>>>
>>> +/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
>>> + * workaround will only work for level triggered interrupts (and in
>>> + * its current form is actively harmful on systems that don't support
>>> + * FIQ).
>>> + */
>>> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32
>>> irqstat)
>>> +{
>>> +       u32 irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>>> +
>>> +       if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
>>> +               return irqnr;
>>> +
>>> +       if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
>>> +                         (irqnr / 32 * 4)) &
>>> +           BIT(irqnr % 32))
>>> +               return irqnr;
>>> +
>>> +       /* this interrupt was spurious (needs to be handled as FIQ) */
>>> +       writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);
>>
>> This will NOT work, because of the note I mentioned above:
>> "The FIQ exception will not occur anymore after gic_handle_irq
>> read/acknowledges the FIQ by reading the GIC_CPU_INTACK register"
>> So with this code you will say End Of Interrupt at the GIC level,
>> without actually handling the interrupt, so you are missing an
>> interrupt.
>> I did the following test to confirm the missing interrupt:
>> I have changed the periodic timer interrupt by an one-shot timer
>> interrupt. The one-shot timer interrupt is programmed by the FIQ
>> handler for the next FIQ interrupt. As expected: When the problem
>> occurs, no more FIQ interrupts are generated.
>
> Can you confirm whether your timer interrupts are configured level or
> edge triggered? Or whether EOIing the GIC causes them to be cleared by
> some other means.

From page 48 of DDI0407I_cortex_a9_mpcore_r4p1_trm.pdf:
Watchdog timers, PPI(3)
Each Cortex-A9 processor has its own watchdog timers that can generate
interrupts, using ID30.

From page 56:
PPI[0], [2],and[3]:b11
interrupt is rising-edge sensitive.

>
> A level triggered interrupt that hasn't been serviced should return the
> pending state (shouldn't it?).
>
>
>>> +       return 1023;
>>> +}
>>> +
>>>  static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
>>>  {
>>>         u32 irqstat, irqnr;
>>> @@ -310,8 +332,10 @@ static void __exception_irq_entry
>>> gic_handle_irq(struct pt_regs *regs)
>>>         void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>
>>>         do {
>>> +               local_fiq_disable();
>>
>> It is a bit weird to disable the "Non-Maskable Interrupt" at every
>> interrupt, because of a problem that occurs once every few thousand
>> interrupts
>
> Given that simply reading from GIC_CPU_INTACK has significantly
> interferes with FIQ reception means that I'm not too worried about this.
>
> Note also that leaving the FIQ unmasked increases worst case latency
> here because once we get a group 0 interrupt back from intack then
> spurious entry to the FIQ handler (which would see an ACK of 1023) just
> wastes cycles.
>
>
>>>                 irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>> -               irqnr = irqstat & GICC_IAR_INT_ID_MASK;
>>> +               irqnr = gic_handle_spurious_group_0(gic, irqstat);
>>> +               local_fiq_enable();
>>>
>>>                 if (likely(irqnr > 15 && irqnr < 1021)) {
>>>                         irqnr = irq_find_mapping(gic->domain, irqnr);
>>
>>
>> The following type of changes could fix the problem for me:
>>
>> @@ -290,19 +290,66 @@ static int gic_set_wake(struct irq_data *d,
>> unsigned int on)
>>
>>  #else
>>  #define gic_set_wake NULL
>>  #endif
>>
>> +extern void (*fiq_handler)(void);
>> +
>> +/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
>> + * workaround will only work for level triggered interrupts (and in
>> + * its current form is actively harmful on systems that don't support
>> + * FIQ).
>> + */
>> +static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32 irqstat)
>> +{
>> + u32 irqnr = irqstat & ~0x1c00;
>> +
>> + if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
>> + return irqnr;
>> +
>> + if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
>> +  (irqnr / 32 * 4)) & BIT(irqnr % 32))
>> + return irqnr;
>> +
>> + /*
>> + * The FIQ should be disabled before the next FIQ interrupt occurs,
>> + * so this only works when the next FIQ interrupt is not "too fast"
>> + * after the previous one.
>> + */
>> + local_fiq_disable();
>> +
>> + /*
>> + * Notes:
>> + * - The FIQ exception will not occur anymore for this current
>> + *   interrupt, because gic_handle_irq has already read/acknowledged
>> + *   the GIC_CPU_INTACK register. So handle the FIQ here.
>> + * - The fiq_handler below should not call ack_fiq and eoi_fiq,
>> + *   because rereading the GIC_CPU_INTACK register returns spurious
>> + *   interrupt ID 0x3FF. So probably you will have to add sometime like
>> + *   the following to fiq_handler:
>> + *   u32 cpsr, irqstat;
>> + *   __asm__("mrs %0, cpsr" : "=r" (cpsr));
>> + *   if ((cpsr & MODE_MASK) == FIQ_MODE)
>> + *   irqstat = ack_fiq();
>> + */
>> + (*fiq_handler)();
>
> Any portable approach would have to switch to FIQ mode to run the
> handler in order to provide the proper register banks for FIQ handlers
> written in assembler.
>
> If we can't get level triggering to work then we have to:
>
> 1. Write code to jump correctly into FIQ mode.
>
> 2. Modify the gic's ack_fiq() callback to automatically avoid reading
>    intack when the workaround is deployed.
>
> The above is why I wanted to see if we can make do with level triggering
> (and automatic re-triggering for interrupts such as SGIs that are
> cleared by EOI).

But the re-triggering introduces extra latencies and a lot of use
cases of FIQ's try to avoid that.
Marek Vasut July 15, 2014, 6:45 p.m. UTC | #4
On Tuesday, July 15, 2014 at 11:41:25 AM, Daniel Thompson wrote:
> On 14/07/14 14:51, Harro Haan wrote:
> > On 10 July 2014 10:03, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> >> This patchset makes it possible to use kgdb's NMI infrastructure on ARM
> >> platforms.
> >> 
> >> The patches have been previously circulated as part of a large patchset
> >> mixing together ARM architecture code and driver changes
> >> (http://thread.gmane.org/gmane.linux.ports.arm.kernel/333901 ). This
> >> patchset is dramatically cut down to include only the arch/arm code. The
> >> driver code (irqchip and tty/serial) will follow when/if the arch code
> >> is accepted.
> >> 
> >> The first two patches modify the FIQ infrastructure to allow interrupt
> >> controller drivers to register callbacks (the fiq_chip structure) to
> >> manage FIQ routings and to ACK and EOI the FIQ. This makes it possible
> >> to use FIQ in multi-platform kernels and with recent ARM interrupt
> >> controllers.
> > 
> > Daniel,
> > 
> > Thanks for the patches. I have tested the fiq and irq-gic patches on
> > an i.MX6 (SabreSD board) for a different purpose:
> > A FIQ timer interrupt at 1 kHz. The TWD watchdog block is used in
> > timer mode for this (interrupt ID 30). The GIC affinity is set to CPU
> > core 0 only for this interrupt ID.
> > 
> > I was surprised by the following behavior:
> > $ cat /proc/interrupts
> > 
> >            CPU0       CPU1       CPU2       CPU3
> >  
> >  29:       5459       3381       3175       3029       GIC  29  twd
> >  30:         11          0          0          0       GIC  30  fake-fiq
> > 
> > Once every few seconds is interrupt ID 30 handled by the regular GIC
> > handler instead of the FIQ exception path (which causes for example a
> > bit more latencies). The other thousands of FIQ's are handled by the
> > FIQ exception path. The problem is also confirmed by the following
> > stackframe of the Lauterbach tooling:
> > fake_fiq_handler(irq = 30, ...)
> > handle_percpu_devid_irq(irq = 30, ...)
> > generic_handle_irq(irq = 30)
> > handle_IRQ(irq = 30, ...)
> > gic_handle_irq
> > __irq_svc
> > -->exception
> > 
> > Notes:
> > 
> > - The problem will occur more often by executing the following command:
> >   $ while true; do hackbench 20; done &
> > 
> > - Reading the GIC_CPU_INTACK register returns the interrupt ID of the
> > highest priority pending interrupt.
> > - The GIC_CPU_INTACK register (used by fiq_ack) will return spurious
> > interrupt ID 0x3FF when read in fake_fiq_handler, because
> > GIC_CPU_INTACK is already read by gic_handle_irq.
> > - The FIQ exception will not occur anymore after gic_handle_irq
> > read/acknowledges the FIQ by reading the GIC_CPU_INTACK register
> > 
> > From the behavior above I conclude that the GIC_CPU_INTACK register is
> > first updated before the FIQ exception is generated, so there is a
> > small period of time that gic_handle_irq can read/acknowledge the FIQ.
> 
> Makes sense.
> 
> Avoiding this problem on GICv2 is easy (thanks to the aliased intack
> register) but on iMX.6 we have only a GICv1.

Yep.

> > I can reduce the number of occurrences (not prevent it) by adding the
> > following hack to irq-gic.c
> > @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
> > gic_handle_irq(struct pt_regs *regs
> > 
> >   u32 irqstat, irqnr;
> >   struct gic_chip_data *gic = &gic_data[0];
> >   void __iomem *cpu_base = gic_data_cpu_base(gic);
> >   
> >   do {
> > 
> > + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
> > & (1 << 30))
> > +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
> > 
> >   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
> >   irqnr = irqstat & ~0x1c00;
> 
> I've made a more complete attempt to fix this. Could you test the
> following? (and be prepared to fuzz the line numbers)

There's also another workaround, look at [1], but it's really a perverse hack 
thus far (blush). What I did there is I got hinted that an L1 page table can 
have this NS bit set. If this bit is set for a mapping, all accesses to memory 
area via that mapping will be non-secure. And then, in turn, by doing a non-
secure read of the INTACK register, it will not ever happen that the FIQ number 
will pop up in the INTACK. I only do a non-secure read of the INTACK register,
all other registers of the GICv1 are read via regular secure-mode accesses.

[1] http://git.denx.de/?p=linux-denx/linux-denx-
marex.git;a=shortlog;h=refs/heads/topic/socfpga/fiq-2014-07-10_01
Daniel Thompson July 16, 2014, 12:54 p.m. UTC | #5
On 15/07/14 19:45, Marek Vasut wrote:
>>> I can reduce the number of occurrences (not prevent it) by adding the
>>> following hack to irq-gic.c
>>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>>> gic_handle_irq(struct pt_regs *regs
>>>
>>>   u32 irqstat, irqnr;
>>>   struct gic_chip_data *gic = &gic_data[0];
>>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>   
>>>   do {
>>>
>>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>>> & (1 << 30))
>>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>>
>>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>>   irqnr = irqstat & ~0x1c00;
>>
>> I've made a more complete attempt to fix this. Could you test the
>> following? (and be prepared to fuzz the line numbers)
> 
> There's also another workaround, look at [1], but it's really a perverse hack 
> thus far (blush). What I did there is I got hinted that an L1 page table can 
> have this NS bit set. If this bit is set for a mapping, all accesses to memory 
> area via that mapping will be non-secure. And then, in turn, by doing a non-
> secure read of the INTACK register, it will not ever happen that the FIQ number 
> will pop up in the INTACK. I only do a non-secure read of the INTACK register,
> all other registers of the GICv1 are read via regular secure-mode accesses.

I'll be looking into this approach.

It is technically a better approach than mine since it prevents the IRQ
handler from ever reading a group 0 interrupt from INTACK.

Unfortunately the tentacles of this workaround reach pretty deep in the
memory management code (rather than being concentrated in the GIC
driver) but the improved runtime behaviour might be worth it.
Harro Haan July 16, 2014, 5:21 p.m. UTC | #6
On 16 July 2014 14:54, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 15/07/14 19:45, Marek Vasut wrote:
>>>> I can reduce the number of occurrences (not prevent it) by adding the
>>>> following hack to irq-gic.c
>>>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>>>> gic_handle_irq(struct pt_regs *regs
>>>>
>>>>   u32 irqstat, irqnr;
>>>>   struct gic_chip_data *gic = &gic_data[0];
>>>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>>
>>>>   do {
>>>>
>>>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>>>> & (1 << 30))
>>>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>>>
>>>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>>>   irqnr = irqstat & ~0x1c00;
>>>
>>> I've made a more complete attempt to fix this. Could you test the
>>> following? (and be prepared to fuzz the line numbers)
>>
>> There's also another workaround, look at [1], but it's really a perverse hack
>> thus far (blush). What I did there is I got hinted that an L1 page table can
>> have this NS bit set. If this bit is set for a mapping, all accesses to memory
>> area via that mapping will be non-secure. And then, in turn, by doing a non-
>> secure read of the INTACK register, it will not ever happen that the FIQ number
>> will pop up in the INTACK. I only do a non-secure read of the INTACK register,
>> all other registers of the GICv1 are read via regular secure-mode accesses.
>
> I'll be looking into this approach.
>
> It is technically a better approach than mine since it prevents the IRQ
> handler from ever reading a group 0 interrupt from INTACK.

Agree, preventing the problem is better than fixing it afterwards.

>
> Unfortunately the tentacles of this workaround reach pretty deep in the
> memory management code (rather than being concentrated in the GIC
> driver) but the improved runtime behaviour might be worth it.

I did some worst case measurements on the SabreSD while running:
$ while true; do hackbench 20; done &

Use banked non-secure GIC_CPU_INTACK register for regular interrupts
(patches by Marek):
The FIQ handler reads the TWD_TIMER_COUNTER 2570 ticks (which is x
1000 / 498 = 5161 nsec) after FIQ interrupt ID30 is generated.
The average is around 497 ticks.
The minimum is around 34 ticks.

Use re-trigger approach by putting it back to pending state (latest
patch by Daniel):
The FIQ handler reads the TWD_TIMER_COUNTER 2678 ticks (which is x
1000 / 498 = 5378 nsec) after FIQ interrupt ID30 is generated.
The average is around 563 ticks (note: almost everything is normal path)
The minimum is around 34 ticks (note: this is the normal path, not the
re-trigger path)

So the results are quite similar.
Daniel Thompson July 17, 2014, 9:20 a.m. UTC | #7
On 16/07/14 18:21, Harro Haan wrote:
> On 16 July 2014 14:54, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>> On 15/07/14 19:45, Marek Vasut wrote:
>>>>> I can reduce the number of occurrences (not prevent it) by adding the
>>>>> following hack to irq-gic.c
>>>>> @@ -297,10 +309,12 @@ static asmlinkage void __exception_irq_entry
>>>>> gic_handle_irq(struct pt_regs *regs
>>>>>
>>>>>   u32 irqstat, irqnr;
>>>>>   struct gic_chip_data *gic = &gic_data[0];
>>>>>   void __iomem *cpu_base = gic_data_cpu_base(gic);
>>>>>
>>>>>   do {
>>>>>
>>>>> + while(readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_PENDING_SET)
>>>>> & (1 << 30))
>>>>> +   printk(KERN_ERR "TEMP: gic_handle_irq: wait for FIQ exception\n");
>>>>>
>>>>>   irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
>>>>>   irqnr = irqstat & ~0x1c00;
>>>>
>>>> I've made a more complete attempt to fix this. Could you test the
>>>> following? (and be prepared to fuzz the line numbers)
>>>
>>> There's also another workaround, look at [1], but it's really a perverse hack
>>> thus far (blush). What I did there is I got hinted that an L1 page table can
>>> have this NS bit set. If this bit is set for a mapping, all accesses to memory
>>> area via that mapping will be non-secure. And then, in turn, by doing a non-
>>> secure read of the INTACK register, it will not ever happen that the FIQ number
>>> will pop up in the INTACK. I only do a non-secure read of the INTACK register,
>>> all other registers of the GICv1 are read via regular secure-mode accesses.
>>
>> I'll be looking into this approach.
>>
>> It is technically a better approach than mine since it prevents the IRQ
>> handler from ever reading a group 0 interrupt from INTACK.
> 
> Agree, preventing the problem is better than fixing it afterwards.
> 
>>
>> Unfortunately the tentacles of this workaround reach pretty deep in the
>> memory management code (rather than being concentrated in the GIC
>> driver) but the improved runtime behaviour might be worth it.
> 
> I did some worst case measurements on the SabreSD while running:
> $ while true; do hackbench 20; done &
> 
> Use banked non-secure GIC_CPU_INTACK register for regular interrupts
> (patches by Marek):
> The FIQ handler reads the TWD_TIMER_COUNTER 2570 ticks (which is x
> 1000 / 498 = 5161 nsec) after FIQ interrupt ID30 is generated.
> The average is around 497 ticks.
> The minimum is around 34 ticks.
> 
> Use re-trigger approach by putting it back to pending state (latest
> patch by Daniel):
> The FIQ handler reads the TWD_TIMER_COUNTER 2678 ticks (which is x
> 1000 / 498 = 5378 nsec) after FIQ interrupt ID30 is generated.
> The average is around 563 ticks (note: almost everything is normal path)
> The minimum is around 34 ticks (note: this is the normal path, not the
> re-trigger path)
> 
> So the results are quite similar.

This is great work.

Be aware that I'd also expect the the performance of my workaround would
drop a little bit when when support for SGIs is added (mostly due to due
to increased code size).
diff mbox

Patch

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 73ae896..309bf2c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -303,6 +303,28 @@  static int gic_set_wake(struct irq_data *d,
unsigned int on)
 #define gic_set_wake	NULL
 #endif

+/* Check for group 0 interrupt spuriously acked as a normal IRQ. This
+ * workaround will only work for level triggered interrupts (and in
+ * its current form is actively harmful on systems that don't support
+ * FIQ).
+ */
+static u32 gic_handle_spurious_group_0(struct gic_chip_data *gic, u32
irqstat)
+{
+	u32 irqnr = irqstat & GICC_IAR_INT_ID_MASK;
+
+	if (!gic_data_fiq_enable(gic) || irqnr >= 1021)
+		return irqnr;
+
+	if (readl_relaxed(gic_data_dist_base(gic) + GIC_DIST_IGROUP +
+			  (irqnr / 32 * 4)) &
+	    BIT(irqnr % 32))
+		return irqnr;
+
+	/* this interrupt was spurious (needs to be handled as FIQ) */
+	writel_relaxed(irqstat, gic_data_cpu_base(gic) + GIC_CPU_EOI);
+	return 1023;
+}
+
 static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)