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

[sashiko-bot@kernel.org]

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