Re: [PATCH v6 01/18] mtd: rawnand: sunxi: drain interrupts before reusing the completion

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 12/10/2026 at 12:26:00 -06, James Hilliard <james.hilliard1@gmail.com> wrote:

> On Mon, Oct 12, 2026 at 12:02 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
>>
>> Hi James,
>>
>> On 14/09/2026 at 15:09:47 -06, James Hilliard <james.hilliard1@gmail.com> wrote:
>>
>> > A timed-out wait can return while the interrupt handler is still using
>> > the previous operation's interrupt status and enable mask. Starting the
>> > next wait calls init_completion() on the same object, which can reset its
>> > waitqueue lock while the old handler calls complete(). The old handler
>> > can also complete the new wait or restore an obsolete interrupt mask.
>> >
>> > Initialize the completion once in probe and use reinit_completion() for
>> > each interrupt-driven wait. On timeout, disable and synchronize the IRQ
>> > before masking controller interrupts and acknowledging the waited-for
>> > events. Draining the handler first also prevents a partial-event handler
>> > from rewriting the enable mask after it has been cleared. Flush these
>> > register writes before re-enabling the IRQ line, leaving the controller's
>> > interrupt sources masked.
>> >
>> > For successful waits, move complete() after the handler's register updates
>> > so that the next operation cannot race with those updates. Keep polling
>> > unchanged and leave controller and DMA abort handling to the existing
>> > callers.
>> >
>> > Fixes: 1fef62c1423b ("mtd: nand: add sunxi NAND flash controller support")
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>> > ---
>>
>> So from the commit log I understand that you are fixing a race. The
>> problem seems legitimate.
>>
>> >  drivers/mtd/nand/raw/sunxi_nand.c | 26 ++++++++++++++++++++------
>> >  1 file changed, 20 insertions(+), 6 deletions(-)
>> >
>>
>> ...
>>
>> > @@ -414,12 +416,13 @@ static irqreturn_t sunxi_nfc_interrupt(int irq, void *dev_id)
>> >       if (!(ien & st))
>> >               return IRQ_NONE;
>> >
>> > -     if ((ien & st) == ien)
>> > -             complete(&nfc->complete);
>> > -
>> > +     /* Finish updating the interrupt state before waking the next operation. */
>> >       writel(st & NFC_INT_MASK, nfc->regs + NFC_REG_ST);
>> >       writel(~st & ien & NFC_INT_MASK, nfc->regs + NFC_REG_INT);
>> >
>> > +     if ((ien & st) == ien)
>> > +             complete(&nfc->complete);
>> > +
>> >       return IRQ_HANDLED;
>> >  }
>> >
>> > @@ -435,16 +438,19 @@ static int sunxi_nfc_wait_events(struct sunxi_nfc *nfc, u32 events,
>> >               timeout_ms = NFC_DEFAULT_TIMEOUT_MS;
>> >
>> >       if (!use_polling) {
>> > -             init_completion(&nfc->complete);
>> > +             reinit_completion(&nfc->complete);
>>
>> The whole change around (re)init_completion looks relevant (here and in the handler).
>>
>> >
>> >               writel(events, nfc->regs + NFC_REG_INT);
>> >               ret = wait_for_completion_timeout(&nfc->complete,
>> >                                               msecs_to_jiffies(timeout_ms));
>> > -             if (!ret)
>> > +             if (!ret) {
>> > +                     /* Drain the handler before it can restore an old IRQ mask. */
>> > +                     disable_irq(nfc->irq);
>>
>> disable_irq() for serializing accesses to a single register?
>
> The intention was to drain an in-flight handler before cleaning up the
> timed-out wait, rather than just serialize the register write.

I guess here you relate to the race explained below.

> disable_irq() provides that synchronization as well as masking the line.
> It is only used on timeout, and this IRQ is requested without
> IRQF_SHARED.

This is clearly debatable, since we can identify the IRQ, that flag
could (should, maybe) have been set. With your approach it would have
been even more impacting. But that's not the point.

> A spinlock could also address this, provided it covers the handler's
> status/enable reads through complete(), together with the waiter's
> cleanup and rearming. Protecting only the register writes would leave
> the handler able to act on an old snapshot.

I'm not sure what you call "only the register writes", you can call
complete() from within the lock if that makes sense for this case,
yes. complete() does not block so it is fine.

...

>> commit log, I now disagree with the first part of:
>>
>> >The old handler can also complete the new wait or restore an obsolete interrupt mask.
>>
>> "complete the new wait"? I don't see how a stale handler completes the
>> next wait given the handler is a hard IRQ and the interrupt enable
>> register is cleared on timeout.
>
> The case I had in mind is an already-running handler on another CPU:
>
> CPU 0: Read st and ien for operation A.
> CPU 1: Time out operation A, mask/acknowledge its interrupts,
>        then eventually reinitialize the completion for operation B.
> CPU 0: Call complete() based on the saved st and ien from operation A.

For this to fire, the hard IRQ handler on CPU0 would have to be stopped
between reading st/ien and calling complete() long enough to span:
- A's timeout
- A's cleanup
- B's wait being rearmed
On this hardware, how is this reachable?

> Clearing the device's enable register does not invalidate those local
> values or wait for the running handler to finish. Hard-IRQ context
> prevents this overlap on the same CPU, but not across CPUs.
>
> That distinction was not explicit enough in the commit message. I'm
> open to a different implementation; the requirement I wanted to preserve
> is that the old handler cannot update the mask or signal the completion
> after the waiter has cleaned up and reused it.

Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/