[PATCH v2 0/2] spi: spi-qpic-snand: fix the stale ECC context pointer

[PATCH v2 0/2] spi: spi-qpic-snand: fix the stale ECC context pointer

[Johan Alvarado]

qcom_spi_ecc_init_ctx_pipelined() installs the ooblayout but never
publishes the ECC context it allocates, so qcom_spi_ooblayout_ecc() and
qcom_spi_ooblayout_free() run against a pointer that describes something
else - a zeroed struct on a first probe, the previous attempt's freed
context on a retry. On IPQ5018 the qcom,smem-part parser makes that retry
routine, and like half the boots on a Mercusys MR80X failed to mount the
rootfs.

Patch 1 is the fix and is unchanged from v1. Patch 2 removes what becomes
redundant once the context is published. It is a cleanup with no
functional change, so it carries no Fixes: tag and is not marked for
stable.

Patch 1 was applied to mtd/fixes as 93bc7c4d2f41 on 2026-09-04 and
dropped the same day, so nothing from this series is queued.

v1: https://lore.kernel.org/all/20260825013848.1056946-1-contact@c127.dev/

Changes in v2:
 - Add Gabor Juhos' Tested-by to patch 1.
 - Add patch 2, dropping the now redundant assignment in
   qcom_spi_ecc_prepare_io_req_pipelined() and the unused zeroed
   struct qpic_ecc allocated in qcom_spi_probe(). Suggested by Gabor
   Juhos, kept separate so the stable backport stays minimal.
 - Rebase onto spi/for-next, which carries commit 44d39535cd4f ("spi:
   spi-qpic-snand: remove interim 'dev_data' variable from
   qcom_spi_probe()") touching the same probe lines as patch 2.

Both patches build with W=1 and sparse without warnings, and each patch
builds on its own.

Johan Alvarado (2):
  spi: spi-qpic-snand: publish the ECC context to snandc->qspi
  spi: spi-qpic-snand: drop the redundant ECC context handling

 drivers/spi/spi-qpic-snand.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)


base-commit: 98100d83adc8e17606605d131e685b1fbedfe092
-- 
2.55.0

[PATCH v2 1/2] spi: spi-qpic-snand: publish the ECC context to snandc->qspi

[Johan Alvarado]

qcom_spi_ooblayout_ecc() and qcom_spi_ooblayout_free() read the ECC
configuration through snandc->qspi->ecc. qcom_spi_probe() points it at a
zeroed scratch struct and only qcom_spi_ecc_prepare_io_req_pipelined(),
which runs on page I/O, ever updates it. qcom_spi_ecc_init_ctx_pipelined()
installs the ooblayout but does not publish the context it just
allocated, and qcom_spi_ecc_cleanup_ctx_pipelined() frees that context
without clearing the pointer.

spinand_init() calls mtd_ooblayout_count_freebytes() right after the ECC
context is created and before any page I/O, so the ooblayout always runs
against a pointer that does not describe the current context:

  - On a first probe it reads the zeroed struct from qcom_spi_probe(),
    so steps, bytes and bbm_size are 0. The count then returns 0 rather
    than an error, so the probe continues with mtd->oobavail set to 0.

  - On a probe retry it reads the ecc_cfg the previous attempt freed.

A retry is easy to hit. On IPQ5018 with the qcom,smem-part parser the
partition parse returns -EPROBE_DEFER until SMEM has probed, so the
first spi-nand probe defers. It defers inside
mtd_device_parse_register(), after mtd_otp_nvmem_add() has already read
the factory OTP - that read goes through prepare_io_req and leaves
snandc->qspi->ecc pointing at the context that spinand_cleanup() then
frees. The second probe allocates a new context, never publishes it, and
computes the OOB layout from the freed one. Once the slab has been
reused, qecc->steps holds garbage and

  oobregion->length = qecc->steps * 4;

goes negative. qcom_spi_ooblayout_free() only reports -ERANGE for
section 1 and later, so mtd_ooblayout_count_bytes() sums the regions and
returns that negative length as the byte count. The -512 below is
steps * 4 with steps == -128. It is a byte count that happens to
collide with -ERESTARTSYS, not an error the driver returned.
spinand_init() takes it as an error, and because it is not
-EPROBE_DEFER the driver core never retries and the NAND never appears:

  spi-nand spi0.0: ESMT SPI NAND was found.
  spi-nand spi0.0: probe with driver spi-nand failed with error -512
  UBI error: cannot open mtd rootfs, error -2
  Waiting for root device /dev/ubiblock0_1...

On a Mercusys MR80X (IPQ5018, ESMT F50D1G41LB) about half of the boots
failed to mount the rootfs, the outcome depending on whether the freed
memory had been overwritten yet.

Publish the context when it is created and clear the pointer when it is
destroyed. Clearing leaves snandc->qspi->ecc NULL after cleanup, which
is safe: the mtd is unregistered before cleanup_ctx runs, so no
ooblayout callback can follow.

Fixes: 7304d1909080 ("spi: spi-qpic: add driver for QCOM SPI NAND flash Interface")
Cc: stable@vger.kernel.org
Tested-by: Gabor Juhos <j4g8y7@gmail.com>
Signed-off-by: Johan Alvarado <contact@c127.dev>
---
Unchanged from v1. Both hunks are untouched by 44d39535cd4f, so this
applies to v7.3-rc2 and to spi/for-next alike.

 drivers/spi/spi-qpic-snand.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 9eafaea74f6b..20045d4a46fa 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -411,6 +411,8 @@ static int qcom_spi_ecc_init_ctx_pipelined(struct nand_device *nand)
 	dev_dbg(snandc->dev, "ECC strength: %u bits per %u bytes\n",
 		ecc_cfg->strength, ecc_cfg->step_size);
 
+	snandc->qspi->ecc = ecc_cfg;
+
 	return 0;
 
 err_free_ecc_cfg:
@@ -427,6 +429,7 @@ static void qcom_spi_ecc_cleanup_ctx_pipelined(struct nand_device *nand)
 
 	kfree(snandc->qspi->oob_buf);
 	snandc->qspi->oob_buf = NULL;
+	snandc->qspi->ecc = NULL;
 	kfree(ecc_cfg);
 }
 
-- 
2.55.0

[PATCH v2 2/2] spi: spi-qpic-snand: drop the redundant ECC context handling

[Johan Alvarado]

qcom_spi_ecc_init_ctx_pipelined() now publishes the ECC context to
snandc->qspi->ecc, so the assignment in
qcom_spi_ecc_prepare_io_req_pipelined() repeats what the pointer already
holds, and the zeroed struct qpic_ecc that qcom_spi_probe() allocates is
never read.

The pointer is non-NULL only between context creation and destruction,
and every reader runs inside that window. The ooblayout callbacks are
installed by init_ctx. The page read, write and program helpers run only
when prepare_io_req has set page_rw or oob_rw. qcom_spi_block_erase()
runs only while the mtd is registered, which happens after init_ctx and
ends before cleanup_ctx. The controller drives a single chip select, so
the per-controller pointer and the per-chip context cannot disagree.

Remove both. No functional change.

Suggested-by: Gabor Juhos <j4g8y7@gmail.com>
Signed-off-by: Johan Alvarado <contact@c127.dev>
---
Tested on a Mercusys MR80X (IPQ5018, ESMT F50D1G41LB) with both patches
backported to 6.18.44. Five boots, rootfs mounted every time, mtd
reported 0 ECC failures and 0 corrected bits, and a 4 MiB file on the
UBIFS overlay kept its sha256 across the reboots. Overlay churn raised
the UBI maximum erase counter from 13 to 15, so qcom_spi_block_erase()
ran repeatedly. Build-tested on this base with W=1 and sparse, no
warnings, patch 1 alone and both patches applied.

 drivers/spi/spi-qpic-snand.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 20045d4a46fa..760ee1c29402 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -437,9 +437,7 @@ static int qcom_spi_ecc_prepare_io_req_pipelined(struct nand_device *nand,
 						 struct nand_page_io_req *req)
 {
 	struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
-	struct qpic_ecc *ecc_cfg = nand_to_ecc_ctx(nand);
 
-	snandc->qspi->ecc = ecc_cfg;
 	snandc->qspi->raw_rw = false;
 	snandc->qspi->oob_rw = false;
 	snandc->qspi->page_rw = false;
@@ -1586,14 +1584,9 @@ static int qcom_spi_probe(struct platform_device *pdev)
 	struct spi_controller *ctlr;
 	struct qcom_nand_controller *snandc;
 	struct qpic_spi_nand *qspi;
-	struct qpic_ecc *ecc;
 	struct resource *res;
 	int ret;
 
-	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
-	if (!ecc)
-		return -ENOMEM;
-
 	qspi = devm_kzalloc(dev, sizeof(*qspi), GFP_KERNEL);
 	if (!qspi)
 		return -ENOMEM;
@@ -1613,7 +1606,6 @@ static int qcom_spi_probe(struct platform_device *pdev)
 	snandc->dev = dev;
 	snandc->qspi = qspi;
 	snandc->qspi->ctlr = ctlr;
-	snandc->qspi->ecc = ecc;
 
 	snandc->props = of_device_get_match_data(dev);
 	if (!snandc->props) {
-- 
2.55.0

Re: [PATCH v2 2/2] spi: spi-qpic-snand: drop the redundant ECC context handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deterministic kernel panic in qcom_spi_block_erase() when the host ECC engine is bypassed (e.g., via non-NAND SPI memory drivers like spi-nor, or ON-DIE/software ECC).
--

commit a97a071d79125bb5db4d55c7d03a5f945fa92aef
Author: Johan Alvarado <contact@c127.dev>

    spi: spi-qpic-snand: drop the redundant ECC context handling
    
    qcom_spi_ecc_init_ctx_pipelined() now publishes the ECC context to
    snandc->qspi->ecc, so the assignment in
    qcom_spi_ecc_prepare_io_req_pipelined() repeats what the pointer already
    holds, and the zeroed struct qpic_ecc that qcom_spi_probe() allocates is
    never read.

> The pointer is non-NULL only between context creation and destruction,
> and every reader runs inside that window. The ooblayout callbacks are
> installed by init_ctx. The page read, write and program helpers run only
> when prepare_io_req has set page_rw or oob_rw. qcom_spi_block_erase()
> runs only while the mtd is registered, which happens after init_ctx and
> ends before cleanup_ctx.

Is this assumption accurate for all configurations?

If the device tree specifies ON-DIE or software ECC (nand,ecc-engine =
"on-die"), or if a non-NAND SPI memory device (like spi-nor) is attached to
this controller, won't the host's init_ctx be bypassed?

> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 20045d4a46fad..760ee1c29402f 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c
[ ... ]
> @@ -1586,14 +1584,9 @@ static int qcom_spi_probe(struct platform_device *pdev)
>  	struct spi_controller *ctlr;
>  	struct qcom_nand_controller *snandc;
>  	struct qpic_spi_nand *qspi;
> -	struct qpic_ecc *ecc;
>  	struct resource *res;
>  	int ret;
>  
> -	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> -	if (!ecc)
> -		return -ENOMEM;
> -

[Severity: High]
If init_ctx is bypassed as described above, will removing this allocation
lead to a NULL pointer dereference in qcom_spi_block_erase()?

If snandc->qspi->ecc remains NULL, a standard block erase operation
delegated through qcom_spi_exec_op() to qcom_spi_block_erase() would appear
to dereference it:

drivers/spi/spi-qpic-snand.c:qcom_spi_block_erase() {
	struct qpic_ecc *ecc_cfg = snandc->qspi->ecc;
	...
	snandc->regs->cfg0 = cpu_to_le32((ecc_cfg->cfg0_raw & ~CW_PER_PAGE_MASK) |
					 FIELD_PREP(CW_PER_PAGE_MASK, 0));
}

>  	qspi = devm_kzalloc(dev, sizeof(*qspi), GFP_KERNEL);
>  	if (!qspi)
>  		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911184416.109790-1-contact@c127.dev?part=2

Re: [PATCH v2 2/2] spi: spi-qpic-snand: drop the redundant ECC context handling

[Johan Alvarado]

On Fri, Sep 11, 2026 at 07:10:10PM +0000, sashiko-bot@kernel.org wrote:
> Is this assumption accurate for all configurations?
> If the device tree specifies ON-DIE or software ECC (nand,ecc-engine =
> "on-die"), or if a non-NAND SPI memory device (like spi-nor) is attached to
> this controller, won't the host's init_ctx be bypassed?

The engine is selected by the nand-ecc-engine phandle, or by
nand-no-ecc-engine and nand-use-soft-ecc-engine; the core does not read a
nand,ecc-engine = "on-die" property. A chip node that phandles itself, or
that omits the property and takes the SPI-NAND default, does land on the
on-die engine, and qcom_spi_ecc_init_ctx_pipelined() is then never called.

That configuration has never worked. The page helpers are gated on
qspi->page_rw and qspi->oob_rw, which only
qcom_spi_ecc_prepare_io_req_pipelined() sets, so qcom_spi_read_page()
returns 0 with the buffer untouched while spi_mem_no_dirmap_read() reports
the full length as read. Removing the phandle on an IPQ5018 board confirms
it: the chip probes, then UBI reports "no valid UBI magic found inside
mtd15" and "failed to attach mtd13, error -22". A block erase there took
cfg0_raw and cfg1_raw from the zeroed scratch struct, which is a wrong
erase configuration rather than a working one. So this patch changes no
configuration that works today, and the three in-tree boards using this
controller all set nand-ecc-engine = <&qpic_nand>.

For spi-nor, the binding documents a spi-nand child only, and
qcom_spi_cmd_mapping() returns -EOPNOTSUPP for opcodes outside the
SPI-NAND set, RDSR 0x05 and SFDP 0x5A among them, so such a probe fails
before an mtd is registered.

A mismatched device tree should still not panic, and the silent read is
the worse half of it. Refusing page access and block erase in
qcom_spi_exec_op() while the context is NULL covers both. I have that
patch ready and will send it on top once this series is applied, so it
does not hold anything here up. It cannot go in qcom_spi_supports_op():
spinand_select_op_variant() uses that callback to choose the cache op
templates during detection, long before any ECC context exists, so
rejecting page operations there leaves no variant supported and the chip
fails to probe with "unknown raw ID".

Best regards,
Johan