Maybe a bug in kernel/irq/chip.c unmask_irq(), device irq is masked unexpectedly.

[Yi Zheng]

Hi,

     I found something wrong on my AM3352 SoC machine, the GPIO
triggered IRQ is masked
     unexpectedly.  That bug cause the devices using that GPIO-IRQ can
not work. Even the
     latest kernel version (v5.4-rc2-20-geda57a0e4299)!

     After a long time hacking, I guess the bug is in
kernel/irq/chip.c, the important
     base code for _ALL_ the Processor Platform! That is why this mail
is sent to you.

     Shortly speaking, the bug is about wrong interrupt iteration.
That cause the software
     flag "IRQD_IRQ_MASKED" not refect the real masking status
register in the interrupt
     controller (INTC, drivers/irqchip/irq-omap-intc.c).

     Here is my hw/sw settings:
     (1) FPGA implements some UART, using a GPIO pin as INT signal,

     (2) In the SoC, 481ac000.gpio is under the INTC controller,
irq=28, and hwirq=32
         It is an level triggered IRQ.

     (3) The ISR calling stack shows that handle_level_irq() is the
most important
         function for debugging.

     There is some defects on IRQ processing:

     (1) At the beginning of handle_level_irq(), the IRQ-28 is masked,
and ACK action is executed:
         On my machine, it runs the 'else' branch:
            static inline void mask_ack_irq(struct irq_desc *desc)
            {
                if (desc->irq_data.chip->irq_mask_ack) {
                        desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
                        irq_state_set_masked(desc);
                } else {
                        mask_irq(desc);
                        if (desc->irq_data.chip->irq_ack)
                                desc->irq_data.chip->irq_ack(&desc->irq_data);
                }
            }

         It is an 2-steps procedure:
         1. mask_irq()
         2. desc->irq_data.chip->irq_ack()

         the 2nd step, the function ptr is omap_mask_ack_irq(), which
         _MASK_ the hardware INTC-IRQ-32 and then do the real ACK action.

     (2) mask_irq()/unmask_irq() are not atomic actions:
         They check the IRQD_IRQ_MASKED flag firstly, and then
         mask/unmask the irq by calling the function ptrs which
installed by irq controller drv.
         Then, those 2 functions set/clear the IRQD_IRQ_MASKED flag.

         I think the sequence of the hw/sw action should be mirrored reversed:
         mask_irq():
            check IRQD_IRQ_MASKED;
            set hardware IRQ mask register;
            set software IRQD_IRQ_MASKED flag;

         unmask_irq():
            check IRQD_IRQ_MASKED;
            clear software IRQD_IRQ_MASKED flag; /* NOTE: should
before the hw unmask action!! */
            clear hardware IRQ mask register;

         The current unmask_irq(), hw-mask action runs before sw-mask
action, which gives an very small
         time window. That cause an unexpected iterated IRQ.

     Here is my the detail of my analyzing of handle_level_irq():

     (1) Let record the HW-IRQ-Controller Status and the SW-Flag
IRQD_IRQ_MASKED pair as following:
         (hw-mask, sw-mask).

     (2) In the 1st level of IRQ-28 ISR calling, in unmask_irq(),
after the HW unmask action, and
         before the sw-flag IRQD_IRQ_MASKED is cleared, there is a
VERY SMALL TIME WINDOW,
         in which, another IRQ-28 may triggered.

         In that time window, the mask status is (0, 1), which is no
an valid value.

     (3) In the 2nd level of the ISR(IRQ-28), The mask status is
IRQ-28(0, 1), so
         mask_irq() do nothing, because sw-flag is "1".  That is an
wrong status, the
         programmer thinks that IRQ-28 has been masked, but physically not!

     (4) Before the ACK func-ptr calling, there comes the 3rd level IRQ (28/32)!
         Although mask_irq() do not physically mask the IRQ, ACK acion
(omap_mask_ack_irq)
         of the omap-intc drv mask the IRQ physically. The 3rd ISR runs OK.

         When 3rd level ISR exist, the mask status is (0, 0), that is OK!

     (5) The 3rd level ISR finished, the 2nd level ISR continue.
         It run ACK function ptr -- omap_mask_ack_irq(). The
HW-IRQ-mask is set again!
         Now, the mask status is (1, 0), it is unreasonable value!

     (6) The 2nd level ISR run to cond_unmask_irq(). Due to the
ill-formed mask-status
         value(1, 0), the unmask_irq() will not be called.  Even in
unmask_irq(), another
         checking SW-Flag exists. The real unmask action will not run!

     (7) Now, the 2nd level ISR return, with the mask status (1, 0).
         The 1st level ISR continues, in unmask_irq(), it run
irq_state_clr_masked();
         And it repeatedly clear the IRQD_IRQ_MASKED. The final mask
status is (1, 0).

         What (1, 0) value means? The CPU call will not receive IRQ any more!
         That is my bug phenomenon. If I clean the hardware interrupt
controller's mask bit, my
         devices work again.


      NOTE:
         (1) My SoC is a single core ARM chip: TI-AM3352, so the raw spin-lock
             irq_desc->lock will be optimized to nothing.
handle_level_irq() has no
             spin-lock protection, right?

         (2) In AM3352, INTC driver ACK the IRQ by write 0x01 into INTC Control
             Register(offset 0x48).  The chip doc seems that bit[0] of
INTC-Control Reg is
             only an enable/disable flag.  The IRQ may generated even
if no ACK action
             done. Any one can give me an clarification?

         (3) My analysis is not verified on the real machine. After
some code change for
             debug(add counter to indicates the iteration level, save
the IRQ mask status
             etc.), the device IRQ wrongly masked problem vanished. In
fact, the original
             code can not re-produce the phenomena easily. In tens of
machine, only one
             can get the bug. I have try my best to hacking the code,
but the only
             verified result is here: when bug occur, the HW IRQ is
masked, but the
             IRQD_IRQ_MASKED flag is cleared.

      My fixup is in the attachment, which remove the unexpected time
window of IRQ iteration.
--- kernel/irq/chip.c	2019-07-13 09:28:23.683787367 +0800
+++ /tmp/chip.c	2019-10-08 11:32:35.082258572 +0800
@@ -432,8 +432,8 @@ void unmask_irq(struct irq_desc *desc)
 		return;
 
 	if (desc->irq_data.chip->irq_unmask) {
-		desc->irq_data.chip->irq_unmask(&desc->irq_data);
 		irq_state_clr_masked(desc);
+		desc->irq_data.chip->irq_unmask(&desc->irq_data);
 	}
 }