[PATCH 1/3] mtd: rawnand: marvell: Ensure program page operations are successful

[Ravi Minnikanti]

On 7/17/23 12:42, Miquel Raynal wrote:
> The NAND core complies with the ONFI specification, which itself
> mentions that after any program or erase operation, a status check
> should be performed to see whether the operation was finished *and*
> successful.
> 
> The NAND core offers helpers to finish a page write (sending the
> "PAGE PROG" command, waiting for the NAND chip to be ready again, and
> checking the operation status). But in some cases, advanced controller
> drivers might want to optimize this and craft their own page write
> helper to leverage additional hardware capabilities, thus not always
> using the core facilities.
> 
> Some drivers, like this one, do not use the core helper to finish a page
> write because the final cycles are automatically managed by the
> hardware. In this case, the additional care must be taken to manually
> perform the final status check.
> 
> Let's read the NAND chip status at the end of the page write helper and
> return -EIO upon error.
> 
> Cc: stable at vger.kernel.org
> Fixes: 02f26ecf8c77 ("mtd: nand: add reworked Marvell NAND controller driver")
> Reported-by: Aviram Dali <aviramd at marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> 
> ---
> 
> Hello Aviram,
> 
> I have not tested this, but based on your report I believe the status
> check is indeed missing here and could sometimes lead to unnoticed
> partial writes.
> 
> Please test on your side and reply with your Tested-by if you validate
> the change.
> 
> Any backport on kernels predating v4.17 will likely fail because of a
> folder rename, so you will have to do the backport manually if needed.
> 
> Thanks,
> Miquèl
> ---
>  drivers/mtd/nand/raw/marvell_nand.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index 30c15e4e1cc0..576441095012 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -1162,6 +1162,7 @@ static int marvell_nfc_hw_ecc_hmg_do_write_page(struct nand_chip *chip,
>  		.ndcb[2] = NDCB2_ADDR5_PAGE(page),
>  	};
>  	unsigned int oob_bytes = lt->spare_bytes + (raw ? lt->ecc_bytes : 0);
> +	u8 status;
>  	int ret;
>  
>  	/* NFCv2 needs more information about the operation being executed */
> @@ -1195,7 +1196,18 @@ static int marvell_nfc_hw_ecc_hmg_do_write_page(struct nand_chip *chip,
>  
>  	ret = marvell_nfc_wait_op(chip,
>  				  PSEC_TO_MSEC(sdr->tPROG_max));
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	/* Check write status on the chip side */
> +	ret = nand_status_op(chip, &status);
> +	if (ret)
> +		return ret;
> +
> +	if (status & NAND_STATUS_FAIL)
> +		return -EIO;
> +
> +	return 0;
>  }
>  
>  static int marvell_nfc_hw_ecc_hmg_write_page_raw(struct nand_chip *chip,
> @@ -1624,6 +1636,7 @@ static int marvell_nfc_hw_ecc_bch_write_page(struct nand_chip *chip,
>  	int data_len = lt->data_bytes;
>  	int spare_len = lt->spare_bytes;
>  	int chunk, ret;
> +	u8 status;
>  
>  	marvell_nfc_select_target(chip, chip->cur_cs);
>  
> @@ -1660,6 +1673,14 @@ static int marvell_nfc_hw_ecc_bch_write_page(struct nand_chip *chip,
>  	if (ret)
>  		return ret;
>  
> +	/* Check write status on the chip side */
> +	ret = nand_status_op(chip, &status);
> +	if (ret)
> +		return ret;
> +
> +	if (status & NAND_STATUS_FAIL)
> +		return -EIO;
> +
>  	return 0;
>  }
>  

Patch working as expected. Tested on 3 different NAND chips.

Tested-by: Ravi Chandra Minnikanti <rminnikanti at marvell.com>