Re: [PATCH v10 01/26] mtd: rawnand: sunxi: centralize OOB handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: rawnand: sunxi: premature oob_ops selection in sunxi_nand_attach_chip()
- [Medium] mtd: rawnand: sunxi: unaddressed maintainer NACK on vendor OOB layout ops
--

commit 8fda5c99d9befa1771418516828f040de60c5845
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: sunxi: centralize OOB handling

This commit centralizes hardware ECC result interpretation, protected OOB
retrieval, erased-chunk checking, and user-data layout reservation behind
per-chip sunxi_nfc_oob_ops callbacks for both PIO and DMA paths.

> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index f41feebe42571..80006d1a33d68 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
[ ... ]
> @@ -2066,6 +2096,58 @@ static int sunxi_nfc_maximize_user_data(struct nand_chip *nand, uint32_t oobsize
[ ... ]
> +/*
> + * Both modes use the same layout and ECC policy. Scrambled transfers also
> + * compensate the first two user bytes to keep the on-flash BBM unmodified.
> + */
> +static const struct sunxi_nfc_oob_ops sunxi_nfc_plain_oob_ops = {
> +	.min_user_data = sunxi_nfc_oob_min_user_data,
> +	.init_user_data = sunxi_nfc_oob_init_user_data,
> +	.read_batch = sunxi_nfc_oob_read_batch,
> +	.finish_read = sunxi_nfc_oob_finish_read,
> +};
> +
> +static const struct sunxi_nfc_oob_ops sunxi_nfc_scrambled_plain_bbm_oob_ops = {
> +	.min_user_data = sunxi_nfc_oob_min_user_data,
> +	.init_user_data = sunxi_nfc_oob_init_user_data,
> +	.transform_bbm = sunxi_nfc_randomize_bbm,
> +	.read_batch = sunxi_nfc_oob_read_batch,
> +	.finish_read = sunxi_nfc_oob_finish_read,
> +};

[Severity: Medium]
As reported by Miquel Raynal on v6 [1], the series adding support for the
vendor-specific Allwinner randomized OOB layout was declined due to
maintainability concerns and kernel policy against vendor-specific NAND
layouts, with an invitation to submit only the standalone fixes outside of
the randomized layout support.

Here, sunxi_nfc_plain_oob_ops and sunxi_nfc_scrambled_plain_bbm_oob_ops use
identical callbacks for min_user_data, init_user_data, read_batch, and
finish_read, differing only in transform_bbm. The remaining indirection in
struct sunxi_nfc_oob_ops is only used by the vendor-specific randomized OOB
layout patch later in the series.

Could the PIO and DMA ECC read helpers be shared directly and the standalone
fixes split out without introducing the struct sunxi_nfc_oob_ops vtable?

[ ... ]
> @@ -2254,6 +2303,9 @@ static int sunxi_nand_attach_chip(struct nand_chip *nand)
>  	if (!ecc->size || !ecc->strength)
>  		return -EINVAL;
>  
> +	sunxi_nand->oob_ops = nand->options & NAND_NEED_SCRAMBLING ?
> +		&sunxi_nfc_scrambled_plain_bbm_oob_ops : &sunxi_nfc_plain_oob_ops;

[Severity: High]
Does selecting sunxi_nand->oob_ops in sunxi_nand_attach_chip() break bad
block marker compensation for chips that set NAND_NEED_SCRAMBLING during
nand_manufacturer_init()?

In nand_scan_with_ids(), nand_attach() runs before nand_scan_tail():

  nand_scan_with_ids()
    -> nand_scan_ident()
    -> nand_attach()
      -> sunxi_nand_attach_chip()
    -> nand_scan_tail()
      -> nand_manufacturer_init()

For ONFI-compliant chips where nand_onfi_detect() succeeds in nand_detect()
and jumps to ident_done without setting NAND_NEED_SCRAMBLING, the flag can
be set later in nand_manufacturer_init(), such as in
drivers/mtd/nand/raw/nand_toshiba.c:

  toshiba_nand_init()
    -> tc58teg5dclta00_init()
      chip->options |= NAND_NEED_SCRAMBLING;

Because sunxi_nand_attach_chip() runs before nand_manufacturer_init(),
sunxi_nand->oob_ops is latched to sunxi_nfc_plain_oob_ops (where
transform_bbm is NULL). Later, during page and OOB reads and writes,
sunxi_nfc_randomizer_config() and sunxi_nfc_randomizer_enable() dynamically
check nand->options & NAND_NEED_SCRAMBLING and enable hardware scrambling,
while sunxi_nfc_hw_ecc_get_prot_oob_bytes() and
sunxi_nfc_hw_ecc_set_prot_oob_bytes() check sunxi_nand->oob_ops->transform_bbm
and skip calling sunxi_nfc_randomize_bbm().

[1] https://lore.kernel.org/r/871p9v4a1x.fsf@bootlin.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-submit-sunxi-nand-vendor-oob-layout-v1-v10-0-dca1f6ed29a8@gmail.com?part=1