Re: [PATCH v5 01/20] EDAC/synopsys: Fix ECC status data and IRQ disable race condition

[Serge Semin <fancer.lancer@gmail.com>]

On Sun, Apr 21, 2024 at 12:07:30PM +0200, Borislav Petkov wrote:
> On Tue, Apr 16, 2024 at 01:06:11PM +0300, Serge Semin wrote:
> > It looks indeed crazy because the method is called enable_intr() and
> > is called in the IRQ handler. Right, re-enabling the IRQ in the handler
> > doesn't look good. But under the hood it was just a way to fix the
> > problem described in the commit you cited. enable_intr() just gets
> > back the IRQ Enable flags cleared a bit before in the
> > zynqmp_get_error_info() method.
> > 
> > The root cause of the problem is that the IRQ status/clear flags:
> > ECCCLR.ecc_corrected_err_clr	(R/W1C)
> > ECCCLR.ecc_uncorrected_err_clr	(R/W1C)
> > ECCCLR.ecc_corr_err_cnt_clr	(R/W1C)
> > ECCCLR.ecc_uncorr_err_cnt_clr	(R/W1C)
> > etc
> > 
> > and the IRQ enable/disable flags (since v3.10a):
> > ECCCLR.ecc_corrected_err_intr_en	(R/W)
> > ECCCLR.ecc_uncorrected_err_intr_en	(R/W)
> > 
> > reside in a single register - ECCCLR (Synopsys has renamed it to
> > ECCCTL since v3.10a due to adding the IRQ En/Dis flags).
> > 
> > Thus any concurrent access to that CSR like "Clear IRQ
> > status/counters" and "IRQ disable/enable" need to be protected from
> > the race condition.
> 
> Ok, let's pick this apart one-by-one. I'll return to the rest you're
> explaining as needed.
> 
> So, can writes to the status/counter bits while writing the *same* bit
> to the IRQ enable/disable bit prevent any race conditions?

No, because the clear and enable/disable bits belong to the same CSR.
While you are writing the clear+same/enable bits, the concurrent IO
may have changed the same/enable bits. Like this:

     IRQ-handler                        |    IRQ-disabler
                                        |
 tmp = clear_sts_bits | enable_irq_bits;|
                                        | ECCCLR = 0; // disable IRQ
 ECCCLR = tmp;                          |
----------------------------------------+--------------------------------------

As a result even though the IRQ-disabler cleared the IRQ-enable bits,
the IRQ-handler got them back to being set. The same will happen if we
get to write the *same* bits in the handler:

     IRQ-handler                        |    IRQ-disabler
                                        |
 tmp = ECCCLR | clear_sts_bits;         |
                                        | ECCCLR = 0; // disable IRQs
 ECCCLR = tmp;                          |
----------------------------------------+--------------------------------------

The last example is almost the same as what happens at the moment and
what I am fixing in this patch. The difference is that there is a
greater number of ECCCLR CSR changes performed in the IRQ-handler
context, which makes the critical section even wider than it could be:

     IRQ-handler                        |    IRQ-disabler
                                        |
zynqmp_get_error_info:                  |
 ECCCLR = clear_sts_bits;               |
 ECCCLR = 0; // actually redundant      |
...                                     | ECCCLR = 0; // disable IRQs
enable_intr:                            |
 ECCCLR = enable_irq_bits;              |
----------------------------------------+--------------------------------------

> 
> Meaning, you only change the status and counter bits and you preserve
> the same value in the IRQ disable/enable bit?

AFAICS this won't help to solve the race condition because writing the
preserved value of the enable/disable bits is the cause of the race
condition. The critical section is in concurrent flushing of different
values to the ECCCLR.*en bits. The only ways to solve that are:
1. prevent the concurrent access
2. serialize the critical section

> 
> IOW, I'm thinking of shadowing that ECCCTL in software so that we update
> it from the shadowed value.

I don't see the shadowing will help to prevent what is happening
unless you know some shadow-register pattern I am not aware of. AFAIR
the shadow register is normally utilized for the cases of:
1. read ops returns an incorrect value or a CSR couldn't be read
2. IO bus is too slow in order to speed-up the RMW-pattern
In any case the shadowed value and the process of the data flushing
would need to be protected with a lock anyway in order to sync the
shadow register content and the actual value written to the CSR.

> 
> Because, AFAIU, the spinlock won't help if you grab it, clear the
> status/counter bits and disable the interrupt in the process. You want
> to only clear the status/counter bits and leave the interrupt enabled.
> 
> Right?

Right, but the spinlock will help. What I need to do deal with two
concurrent operations:
IRQ-handler:  clear the status/counter bits and leave the IRQ enable
              bits as is.
IRQ-disabler: clear the IRQ enable bits
These actions need to be serialized in order to prevent the race
condition.

> 
> IOW, in one single write you do:
> 
> ECCCLR.ecc_corrected_err_clr=1
> ECCCLR.ecc_uncorrected_err_clr=1
> ECCCLR.ecc_corr_err_cnt_clr=1
> ECCCLR.ecc_uncorr_err_cnt_clr=1
> ECCCLR.ecc_corrected_err_intr_en=1
> ECCCLR.ecc_uncorrected_err_intr_en=1
> 
> ?

This won't be help because the concurrent IRQ-disabler could have
already cleared the IRQ enable bits while the IRQ-handler is being
executed and about to write to the ECCCLR register. Like this:

     IRQ-handler                        |    IRQ-disabler
                                        |
 tmp = clear_sts_bits | enable_irq_bits;|
                                        | ECCCLR = 0; // disable IRQ
 ECCCLR = tmp;                          |
----------------------------------------+--------------------------------------

Even if we get to add the spin-lock serializing the ECCCLR writes it
won't solve the problem since the IRQ-disabler critical section could
be executed a bit before the IRQ-handler critical section so the later
one will just re-enable the IRQs disabled by the former one.

Here is what is suggested in my patch to fix the problem:

     IRQ-handler                        |    IRQ-disabler
                                        |
zynqmp_get_error_info:                  |
                                        | lock_irqsave
                                        | ECCCLR = 0; // disable IRQs
                                        | unlock_irqrestore
 lock_irqsave;                          |
 tmp = ECCCLR | clear_sts_bits;         |
 ECCCLR = tmp;                          |
 unlock_irqrestore;                     |
----------------------------------------+--------------------------------------

See, the IRQ-status/counters clearing and IRQ disabling processes are
serialized so the former one wouldn't override the values written by
the later one.

Here is the way it would have looked in case of the shadow-register
implementation:

     IRQ-handler                        |    IRQ-disabler
                                        |
zynqmp_get_error_info:                  |
                                        | lock_irqsave
                                        | shadow_en_bits = 0;
                                        | ECCCLR = shadow_en_bits; // disable IRQs
                                        | unlock_irqrestore
 lock_irqsave;                          |
 tmp = clear_sts_bits | shadow_en_bits; |
 ECCCLR = tmp;                          |
 unlock_irqrestore;                     |
----------------------------------------+--------------------------------------

The shadow-register pattern just prevents one ECCCLR read op. The
shadowed data sync would have needed the serialization anyway. Seeing
the DW DDR uMCTL2 controller CSRs are always memory mapped, I don't
see using the shadow-register CSR would worth being implemented unless
you meant something different.

-Serge(y)

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette