Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces a regression for small-page NANDs by explicitly propagating errors from `nand_change_read_column_op()`, which unconditionally returns `-ENOTSUPP` when `mtd->writesize <= 512`.
--
commit 63fba1d4fa0b9df218cee82050011ec93e712247
Author: James Hilliard <james.hilliard1@gmail.com>
mtd: rawnand: sunxi: propagate page-setup and erased-check errors
This commit stops before starting PIO or DMA transfers when page setup fails
and shares the erased-chunk fallback between PIO and DMA. It routes PIO page
and subpage error exits through ECC disable before returning the transfer
error.
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index 172c7c4b0ba6d..d5d8d383b6d12 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
[ ... ]
> @@ -1067,6 +1067,40 @@ static void sunxi_nfc_hw_ecc_update_stats(struct nand_chip *nand,
> }
> }
>
> +/*
> + * Return 1 for an erased chunk or 0 for an uncorrectable chunk, with ECC
> + * statistics updated in either case. Negative values report transport errors.
> + */
> +static int sunxi_nfc_hw_ecc_read_error(struct nand_chip *nand,
> + u8 *data, int data_off,
> + u8 *oob, int oob_off,
> + unsigned int user_data_sz,
> + unsigned int *max_bitflips)
> +{
> + struct nand_ecc_ctrl *ecc = &nand->ecc;
> + unsigned int oob_len = ecc->bytes + user_data_sz;
> + int ret;
> +
> + /* Check the physical representation for bitflips in erased pages. */
> + if (nand->options & NAND_NEED_SCRAMBLING) {
> + ret = nand_change_read_column_op(nand, data_off, data,
> + ecc->size, false);
> + if (ret)
> + return ret;
> + }
> +
> + ret = nand_change_read_column_op(nand, oob_off, oob, oob_len, false);
> + if (ret)
> + return ret;
[Severity: High]
Will this introduce a regression for small-page NANDs?
When an ECC error is detected, the driver attempts an erased chunk check via
sunxi_nfc_hw_ecc_read_error(), which calls nand_change_read_column_op().
Looking at nand_change_read_column_op() in drivers/mtd/nand/raw/nand_base.c,
it does not support column changes for small-page NANDs and unconditionally
returns -ENOTSUPP:
drivers/mtd/nand/raw/nand_base.c:nand_change_read_column_op() {
...
/* Small page NANDs do not support column change. */
if (mtd->writesize <= 512)
return -ENOTSUPP;
...
}
By explicitly propagating this error, it appears read operations on erased
pages, bad block scans, and OOB data reads on small-page NANDs will now abort
with a transport error (-ENOTSUPP) instead of correctly reporting ECC errors
(-EBADMSG) or processing erased chunks. Prior to this patch, the error from
nand_change_read_column_op() was safely ignored in this path, allowing the
driver to fall back to reporting an uncorrectable ECC error gracefully.
> +
> + ret = nand_check_erased_ecc_chunk(data, ecc->size, oob, oob_len, NULL, 0,
> + ecc->strength);
> +
> + sunxi_nfc_hw_ecc_update_stats(nand, max_bitflips, ret);
> +
> + return ret >= 0;
> +}
> +
@@ -1067,6 +1067,40 @@ static void sunxi_nfc_hw_ecc_update_stats(struct nand_chip *nand,
}
}
+/*
+ * Return 1 for an erased chunk or 0 for an uncorrectable chunk, with ECC
+ * statistics updated in either case. Negative values report transport errors.
+ */
+static int sunxi_nfc_hw_ecc_read_error(struct nand_chip *nand,
+ u8 *data, int data_off,
+ u8 *oob, int oob_off,
+ unsigned int user_data_sz,
+ unsigned int *max_bitflips)
+{
+ struct nand_ecc_ctrl *ecc = &nand->ecc;
+ unsigned int oob_len = ecc->bytes + user_data_sz;
+ int ret;
+
+ /* Check the physical representation for bitflips in erased pages. */
+ if (nand->options & NAND_NEED_SCRAMBLING) {
+ ret = nand_change_read_column_op(nand, data_off, data,
+ ecc->size, false);
+ if (ret)
+ return ret;
+ }
+
+ ret = nand_change_read_column_op(nand, oob_off, oob, oob_len, false);
+ if (ret)
+ return ret;
+
+ ret = nand_check_erased_ecc_chunk(data, ecc->size, oob, oob_len, NULL, 0,
+ ecc->strength);
+
+ sunxi_nfc_hw_ecc_update_stats(nand, max_bitflips, ret);
+
+ return ret >= 0;
+}
+
static int sunxi_nfc_hw_ecc_correct(struct nand_chip *nand, u8 *data, u8 *oob,
int step, u32 status, u32 pattern_found,
unsigned int user_data_sz, bool *erased)
@@ -1115,7 +1149,6 @@ static int sunxi_nfc_hw_ecc_read_chunk(struct nand_chip *nand,
struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
unsigned int user_data_sz = sunxi_nfc_user_data_sz(sunxi_nand, step);
struct nand_ecc_ctrl *ecc = &nand->ecc;
- int raw_mode = 0;
u32 pattern_found;
bool bbm = !step;
bool erased;
@@ -1158,25 +1191,13 @@ static int sunxi_nfc_hw_ecc_read_chunk(struct nand_chip *nand,
return 1;
if (ret < 0) {
- /*
- * Re-read the data with the randomizer disabled to identify
- * bitflips in erased pages.
- */
- if (nand->options & NAND_NEED_SCRAMBLING)
- nand_change_read_column_op(nand, data_off, data,
- ecc->size, false);
- else
+ if (!(nand->options & NAND_NEED_SCRAMBLING))
memcpy_fromio(data, nfc->regs + NFC_RAM0_BASE,
ecc->size);
- nand_change_read_column_op(nand, oob_off, oob,
- ecc->bytes + user_data_sz, false);
-
- ret = nand_check_erased_ecc_chunk(data, ecc->size, oob,
- ecc->bytes + user_data_sz,
- NULL, 0, ecc->strength);
- if (ret >= 0)
- raw_mode = 1;
+ return sunxi_nfc_hw_ecc_read_error(nand, data, data_off,
+ oob, oob_off, user_data_sz,
+ max_bitflips);
} else {
memcpy_fromio(data, nfc->regs + NFC_RAM0_BASE, ecc->size);
@@ -1193,7 +1214,7 @@ static int sunxi_nfc_hw_ecc_read_chunk(struct nand_chip *nand,
sunxi_nfc_hw_ecc_update_stats(nand, max_bitflips, ret);
- return raw_mode;
+ return 0;
}
/*
@@ -1260,6 +1281,8 @@ static int sunxi_nfc_hw_ecc_read_chunks_dma(struct nand_chip *nand, uint8_t *buf
struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
struct mtd_info *mtd = nand_to_mtd(nand);
struct nand_ecc_ctrl *ecc = &nand->ecc;
+ unsigned int corrected = mtd->ecc_stats.corrected;
+ unsigned int failed = mtd->ecc_stats.failed;
unsigned int max_bitflips = 0;
int ret, i, raw_mode = 0;
struct scatterlist sg;
@@ -1354,29 +1377,18 @@ static int sunxi_nfc_hw_ecc_read_chunks_dma(struct nand_chip *nand, uint8_t *buf
if (!(status & NFC_ECC_ERR(i)))
continue;
- /*
- * Re-read the data with the randomizer disabled to
- * identify bitflips in erased pages.
- * TODO: use DMA to read page in raw mode
- */
- if (randomized)
- nand_change_read_column_op(nand, data_off,
- data, ecc->size,
- false);
-
- /* TODO: use DMA to retrieve OOB */
- nand_change_read_column_op(nand,
- mtd->writesize + oob_off,
- oob, ecc->bytes + user_data_sz, false);
-
- ret = nand_check_erased_ecc_chunk(data, ecc->size, oob,
- ecc->bytes + user_data_sz,
- NULL, 0,
- ecc->strength);
- if (ret >= 0)
+ ret = sunxi_nfc_hw_ecc_read_error(nand, data, data_off, oob,
+ mtd->writesize + oob_off,
+ user_data_sz,
+ &max_bitflips);
+ if (ret < 0) {
+ /* The caller retries the whole read in PIO mode. */
+ mtd->ecc_stats.corrected = corrected;
+ mtd->ecc_stats.failed = failed;
+ return ret;
+ }
+ if (ret)
raw_mode = 1;
-
- sunxi_nfc_hw_ecc_update_stats(nand, &max_bitflips, ret);
}
}
@@ -1471,7 +1483,9 @@ static int sunxi_nfc_hw_ecc_read_page(struct nand_chip *nand, uint8_t *buf,
sunxi_nfc_select_chip(nand, nand->cur_cs);
- nand_read_page_op(nand, page, 0, NULL, 0);
+ ret = nand_read_page_op(nand, page, 0, NULL, 0);
+ if (ret)
+ return ret;
sunxi_nfc_hw_ecc_enable(nand);
@@ -1487,7 +1501,7 @@ static int sunxi_nfc_hw_ecc_read_page(struct nand_chip *nand, uint8_t *buf,
&cur_off, &max_bitflips,
i, oob_required, page);
if (ret < 0)
- return ret;
+ goto out;
else if (ret)
raw_mode = true;
}
@@ -1496,9 +1510,11 @@ static int sunxi_nfc_hw_ecc_read_page(struct nand_chip *nand, uint8_t *buf,
sunxi_nfc_hw_ecc_read_extra_oob(nand, nand->oob_poi, &cur_off,
!raw_mode, page);
+ ret = max_bitflips;
+out:
sunxi_nfc_hw_ecc_disable(nand);
- return max_bitflips;
+ return ret;
}
static int sunxi_nfc_hw_ecc_read_page_dma(struct nand_chip *nand, u8 *buf,
@@ -1508,7 +1524,9 @@ static int sunxi_nfc_hw_ecc_read_page_dma(struct nand_chip *nand, u8 *buf,
sunxi_nfc_select_chip(nand, nand->cur_cs);
- nand_read_page_op(nand, page, 0, NULL, 0);
+ ret = nand_read_page_op(nand, page, 0, NULL, 0);
+ if (ret)
+ return ret;
ret = sunxi_nfc_hw_ecc_read_chunks_dma(nand, buf, oob_required, page,
nand->ecc.steps);
@@ -1532,7 +1550,9 @@ static int sunxi_nfc_hw_ecc_read_subpage(struct nand_chip *nand,
sunxi_nfc_select_chip(nand, nand->cur_cs);
- nand_read_page_op(nand, page, 0, NULL, 0);
+ ret = nand_read_page_op(nand, page, 0, NULL, 0);
+ if (ret)
+ return ret;
sunxi_nfc_hw_ecc_enable(nand);
@@ -1550,12 +1570,14 @@ static int sunxi_nfc_hw_ecc_read_subpage(struct nand_chip *nand,
&cur_off, &max_bitflips, i,
false, page);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = max_bitflips;
+out:
sunxi_nfc_hw_ecc_disable(nand);
- return max_bitflips;
+ return ret;
}
static int sunxi_nfc_hw_ecc_read_subpage_dma(struct nand_chip *nand,
@@ -1567,7 +1589,9 @@ static int sunxi_nfc_hw_ecc_read_subpage_dma(struct nand_chip *nand,
sunxi_nfc_select_chip(nand, nand->cur_cs);
- nand_read_page_op(nand, page, 0, NULL, 0);
+ ret = nand_read_page_op(nand, page, 0, NULL, 0);
+ if (ret)
+ return ret;
ret = sunxi_nfc_hw_ecc_read_chunks_dma(nand, buf, false, page, nchunks);
if (ret >= 0)