[v2,3/3] mtd: rawnand: sunxi: select the packed H6/H616 OOB layout
Commit Message
The H6/H616 controller can configure a separate protected user-data
length for every ECC step. Mainline fills all space left after ECC with
user data. This changes the physical ECC offsets from those used by
Allwinner NAND firmware, so pages written by one layout cannot be
decoded with the other.
The allwinner,randomized-oob property selects the firmware's normal-page
format for the configured controller hardware-ECC geometry. Use four
protected user-data bytes per 1 KiB ECC step, cap the total at 16 bytes,
and assign the entire total to ECC step zero. Reserve the same total before
maximizing ECC strength so the selected strength leaves enough room.
Keep the existing maximized mainline user-data layout when the property
is absent. Older controllers have fixed four-byte user-data registers
whose placement already matches the firmware and need no additional
change.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
drivers/mtd/nand/raw/sunxi_nand.c | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
Comments
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] The free OOB layout for steps > 0 incorrectly overlaps with hardware ECC bytes, leading to uncorrectable data corruption.
- [High] Division by zero kernel panic when NAND_ECC_MAXIMIZE_STRENGTH is configured on 512-byte NAND pages.
--
commit 250f35cb46a45ae57aae72a174fbcad8b8b7a7e5
Author: James Hilliard <james.hilliard1@gmail.com>
Subject: mtd: rawnand: sunxi: select the packed H6/H616 OOB layout
This commit updates the sunxi NAND driver to support the randomized OOB
layout used by the H6/H616 controllers. It configures a separate protected
user-data length for each ECC step and allocates up to 16 bytes for step zero.
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index c1246a9268ca5..1265d900fc276 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
[ ... ]
> @@ -2111,6 +2126,10 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
> bytes -= 2;
>
> bytes -= total_user_data_sz;
> + } else if (sunxi_nand->randomized_oob) {
> + total_user_data_sz =
> + sunxi_nfc_h6_user_data_sz(nsectors);
> + bytes -= total_user_data_sz;
> } else {
> /*
> * User-data lengths are encoded in four-byte units. Reserve
[Severity: High]
This is a pre-existing issue, but I noticed a potential division by zero
kernel panic in sunxi_nand_hw_ecc_ctrl_init() when NAND_ECC_MAXIMIZE_STRENGTH
is configured on 512-byte NAND pages.
If a NAND device with a 512-byte page size is configured with the
nand-ecc-maximize property in the device tree, mtd->writesize will be 512.
This function forces ecc->size = 1024 and calculates:
nsectors = mtd->writesize / ecc->size;
This resolves to 0, and a few lines down the code executes:
bytes /= nsectors;
Would it be safer to gracefully fail (e.g., returning -EINVAL) instead of
allowing a fatal division by zero during device probe?
[Severity: Critical]
This is also a pre-existing issue, but I wanted to ask about the free OOB
layout defined in sunxi_nand_ooblayout_free(), which isn't modified by
this patch.
For sections greater than 0, the driver does:
oobregion->offset = sunxi_get_ecc_offset(sunxi_nand, ecc, section);
oobregion->length = user_data_sz;
This appears to incorrectly overlap with hardware ECC bytes, defining the MTD
free region exactly over the hardware ECC bytes instead of the user data
bytes.
Could this lead to unrecoverable data corruption if standard filesystems
(like JFFS2 or UBIFS) write metadata to the free OOB area, unknowingly
overwriting the ECC codes?
@@ -224,6 +224,9 @@
#define USER_DATA_SZ 4
#define SUNXI_NFC_MAX_USER_DATA_SZ 32
+/* The randomized H6/H616 layout packs at most 16 bytes before ECC step 0. */
+#define SUNXI_NFC_H6_MAX_USER_DATA_SZ 16
+
/**
* struct sunxi_nand_chip_sel - stores information related to NAND Chip Select
*
@@ -2037,8 +2040,14 @@ static void sunxi_nand_detach_chip(struct nand_chip *nand)
sunxi_nand->user_data_bytes = NULL;
}
-static int sunxi_nfc_maximize_user_data(struct nand_chip *nand, uint32_t oobsize,
- int ecc_bytes, int nsectors)
+static unsigned int sunxi_nfc_h6_user_data_sz(int nsectors)
+{
+ return min(nsectors * USER_DATA_SZ,
+ SUNXI_NFC_H6_MAX_USER_DATA_SZ);
+}
+
+static int sunxi_nfc_init_user_data(struct nand_chip *nand, uint32_t oobsize,
+ int ecc_bytes, int nsectors)
{
struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
@@ -2054,6 +2063,12 @@ static int sunxi_nfc_maximize_user_data(struct nand_chip *nand, uint32_t oobsize
if (!sunxi_nand->user_data_bytes)
return -ENOMEM;
+ if (sunxi_nand->randomized_oob) {
+ sunxi_nand->user_data_bytes[0] =
+ sunxi_nfc_h6_user_data_sz(nsectors);
+ return 0;
+ }
+
for (step = 0; (step < nsectors) && (remaining_bytes > 0); step++) {
for (i = 0; i < c->nuser_data_tab; i++) {
if (c->user_data_len_tab[i] > remaining_bytes)
@@ -2111,6 +2126,10 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
bytes -= 2;
bytes -= total_user_data_sz;
+ } else if (sunxi_nand->randomized_oob) {
+ total_user_data_sz =
+ sunxi_nfc_h6_user_data_sz(nsectors);
+ bytes -= total_user_data_sz;
} else {
/*
* User-data lengths are encoded in four-byte units. Reserve
@@ -2177,12 +2196,12 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
nsectors = mtd->writesize / ecc->size;
/*
- * The rationale for variable data length is to prioritize maximum ECC
- * strength, and then use the remaining space for user data.
+ * The default variable-length layout prioritizes maximum ECC strength,
+ * then uses the remaining space for user data.
*/
if (nfc->caps->reg_user_data_len) {
- ret = sunxi_nfc_maximize_user_data(nand, mtd->oobsize,
- ecc->bytes, nsectors);
+ ret = sunxi_nfc_init_user_data(nand, mtd->oobsize,
+ ecc->bytes, nsectors);
if (ret)
return ret;
}