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@xxxxxxxxxxxxxxx
> Fixes: 02f26ecf8c77 ("mtd: nand: add reworked Marvell NAND controller driver")
> Reported-by: Aviram Dali <aviramd@xxxxxxxxxxx>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxxx>
