Re: [PATCH v2] mtd: rawnand: marvell: check for RDY bits after enabling the IRQ

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

Hi Daniel,

Daniel Mack <daniel@zonque.org> wrote on Fri, 28 Sep 2018 09:43:18
+0200:

> Hi Chris,
> 
> On 27/9/2018 11:55 PM, Chris Packham wrote:
> > On 27/09/18 20:56, Boris Brezillon wrote:  
> >> On Thu, 27 Sep 2018 10:11:45 +0200
> >> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >>  
> >>> Hi Daniel,
> >>>
> >>> Daniel Mack <daniel@zonque.org> wrote on Thu, 27 Sep 2018 09:17:51
> >>> +0200:
> >>>  
> >>>> At least on PXA3xx platforms, enabling RDY interrupts in the NDCR register
> >>>> will only cause the IRQ to latch when the RDY lanes are changing, and not
> >>>> in case they are already asserted.
> >>>>
> >>>> This means that if the controller finished the command in flight before
> >>>> marvell_nfc_wait_op() is called, that function will wait for a change in
> >>>> the bit that can't ever happen as it is already set.
> >>>>
> >>>> To address this race, check for the RDY bits after the IRQ was enabled,
> >>>> and complete the completion immediately if the condition is already met.
> >>>>
> >>>> This fixes a bug that was observed with a NAND chip that holds a UBIFS
> >>>> parition on which file system stress tests were executed. When
> >>>> marvell_nfc_wait_op() reports an error, UBI/UBIFS will eventually mount
> >>>> the filesystem read-only, reporting lots of warnings along the way.
> >>>>
> >>>> Fixes: 02f26ecf8c77 mtd: nand: add reworked Marvell NAND controller driver
> >>>> Cc: stable@vger.kernel.org
> >>>> Signed-off-by: Daniel Mack <daniel@zonque.org>
> >>>> ---  
> >>>
> >>> Sorry I haven't had the time to check on my Armada, but you figured it
> >>> out, and the fix looks good to me!
> >>>
> >>> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>
> >>> Boris, do you plan to send another fixes PR of can I take it into
> >>> the nand/next branch?  
> >>
> >> Queued to mtd/master.
> > > After fixing my R/B configuration I get a new error with this patch when  
> > running stress_1 from mtd-utils-2.0.0. I don't see this without the patch.  
> 
> That's strange. So your controller sets the RDY bits before it is ready? Could
> you check whether only checking for NDSR_RDY(0) changes anything? Not sure
> about the handling of NDSR_RDY(1) in this driver anyway ...
> 

I suppose you mean this portion of code is not clear enough?


        u32 st = readl_relaxed(nfc->regs + NDSR);
        u32 ien = (~readl_relaxed(nfc->regs + NDCR)) & NDCR_ALL_INT;

        /*
         * RDY interrupt mask is one bit in NDCR while there are two status
         * bit in NDSR (RDY[cs0/cs2] and RDY[cs1/cs3]).
         */
        if (st & NDSR_RDY(1))
                st |= NDSR_RDY(0);

        if (!(st & ien))
                return IRQ_NONE;


-> st is the status in the NDSR register which has two RDY bits, one
   for each RDY line.
-> ien is a view of the NDCR register which commands the interrupts
   and has one bit to enable both interrupt lines (let's call it
   NDCR_RDYM).

The trick is that NDSR_RDY(0) is the same bit as NDCR_RDYM.
So whenever NDSR_RDY(1) is set, we fake NDSR_RDY(0) to be set (by
setting manually the bit in 'st') so that the (st & ien) comparison
can be true if NDSR_RDY(1) is valid and RDY interrupts are enabled.

With this in mind, I don't see why this

+	st = readl_relaxed(nfc->regs + NDSR);
+	if (st & (NDSR_RDY(0) | NDSR_RDY(1)))
+		complete(&nfc->complete);

would break the driver.

Thanks,
Miquèl