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

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On 27/09/18 11:33, Chris Packham wrote:
> Hi Daniel,
> 
> On 27/09/18 09:24, Daniel Mack wrote:
>> 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 mitigate this race, check for the RDY bits after the IRQ was enabled,
>> and only sleep on the condition if the controller isn't ready yet.
>>
>> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Daniel Mack <daniel@xxxxxxxxxx>
>> ---
>>    drivers/mtd/nand/raw/marvell_nand.c | 14 +++++++++++---
>>    1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
>> index 666f34b58dec..e96ec7b9a152 100644
>> --- a/drivers/mtd/nand/raw/marvell_nand.c
>> +++ b/drivers/mtd/nand/raw/marvell_nand.c
>> @@ -613,7 +613,8 @@ static int marvell_nfc_wait_cmdd(struct nand_chip *chip)
>>    static int marvell_nfc_wait_op(struct nand_chip *chip, unsigned int timeout_ms)
>>    {
>>    	struct marvell_nfc *nfc = to_marvell_nfc(chip->controller);
>> -	int ret;
>> +	int ret = -EALREADY;
> 
> Won't this cause us to hit the if (!ret) below if we don't end up
> calling wait_for_completion_timeout()?

Nope boolean logic fail.

> In fact I did just hit something
> like this on my Armada-385 based board
> 
> marvell-nfc f10d0000.nand-controller: Timeout waiting for RB signal
> ubi0 warning: do_sync_erase: error -5 while erasing PEB 755, retry
>

But this still might be something else, it doesn't happen regularly. For 
what it's worth

Tested-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>

>> +	u32 st;
>>    
>>    	/* Timeout is expressed in ms */
>>    	if (!timeout_ms)
>> @@ -622,8 +623,15 @@ static int marvell_nfc_wait_op(struct nand_chip *chip, unsigned int timeout_ms)
>>    	init_completion(&nfc->complete);
>>    
>>    	marvell_nfc_enable_int(nfc, NDCR_RDYM);
>> -	ret = wait_for_completion_timeout(&nfc->complete,
>> -					  msecs_to_jiffies(timeout_ms));
>> +
>> +	/*
>> +	 * Check if the NDSR_RDY bits have already been set before the
>> +	 * interrupt was enabled.
>> +	 */
>> +	st = readl_relaxed(nfc->regs + NDSR);
>> +	if (!(st & (NDSR_RDY(0) | NDSR_RDY(1))))
>> +		ret = wait_for_completion_timeout(&nfc->complete,
>> +						  msecs_to_jiffies(timeout_ms));
>>    	marvell_nfc_disable_int(nfc, NDCR_RDYM);
>>    	marvell_nfc_clear_int(nfc, NDSR_RDY(0) | NDSR_RDY(1));
>>    	if (!ret) {
>>
> 
> 


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