[PATCH 0/7] spi: spi-qpic-snand: use generic code from the 'qpic_common' module

[PATCH 0/7] spi: spi-qpic-snand: use generic code from the 'qpic_common' module

[Gabor Juhos]

Since commit cfae2182fa7b ("mtd: rawnand: qcom: move common code to the
'qpic_common' module"), the 'qpic_common' module provides several generic
functions which can be utilized from the 'spi-qpic-snand' driver instead
of the locally implemented code.

This series converts the driver to use the common code to reduce code
duplication.

In order to make review easier, there are separate patches for each
functions and another one for reusing the 'qcom_nandc_read_stats'
structure.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Gabor Juhos (7):
      spi: spi-qpic-snand: use the common qcom_nandc_check_flash_errors() function
      spi: spi-qpic-snand: use the common qcom_nandc_config_page_read() function
      spi: spi-qpic-snand: use the common qcom_nandc_config_page_write() function
      spi: spi-qpic-snand: use the common qcom_nandc_config_cw_write() function
      spi: spi-qpic-snand: use the common qcom_nandc_set_read_loc_first() function
      spi: spi-qpic-snand: use the common qcom_nandc_set_read_loc_last() function
      spi: spi-qpic-snand: use the common 'qcom_nandc_read_stats' structure

 drivers/spi/spi-qpic-snand.c | 137 ++++++++-----------------------------------
 1 file changed, 24 insertions(+), 113 deletions(-)
---
base-commit: 4a8faaea975be15cb636646c0d4c6ff1c2110baa
change-id: 20261028-qpic-snand-use-common-functions-12b18fc7a1e5
prerequisite-change-id: 20260922-qpic-snand-fix-singlecw-readloc-c4c89b4369f5:v2
prerequisite-patch-id: 7f8a48bccb2e3c8de950bbddacfefe052cea50b8

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/7] spi: spi-qpic-snand: use the common qcom_nandc_check_flash_errors() function

[Gabor Juhos]

The common function from the 'qpic_common' module provides the same
functionality as the locally implemented counterpart.

Use the common function and remove the local one in order to reduce
code duplication.

No functional changes intended.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 41685a286e38..981e16221b59 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -592,22 +592,6 @@ static void qcom_spi_config_single_cw_page_read(struct qcom_nand_controller *sna
 	qcom_spi_config_cw_read(snandc, use_ecc, cw);
 }
 
-static int qcom_spi_check_raw_flash_errors(struct qcom_nand_controller *snandc, int cw_cnt)
-{
-	int i;
-
-	qcom_nandc_dev_to_mem(snandc, true);
-
-	for (i = 0; i < cw_cnt; i++) {
-		u32 flash = le32_to_cpu(snandc->reg_read_buf[i]);
-
-		if (flash & (FS_OP_ERR | FS_MPU_ERR))
-			return -EIO;
-	}
-
-	return 0;
-}
-
 static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
 				 const struct spi_mem_op *op)
 {
@@ -651,7 +635,7 @@ static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
 		return ret;
 	}
 
-	ret = qcom_spi_check_raw_flash_errors(snandc, 1);
+	ret = qcom_nandc_check_flash_errors(snandc, 1);
 	if (ret)
 		return ret;
 
@@ -819,7 +803,7 @@ static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_bu
 		return ret;
 	}
 
-	return qcom_spi_check_raw_flash_errors(snandc, 1);
+	return qcom_nandc_check_flash_errors(snandc, 1);
 }
 
 static int qcom_spi_read_page_raw(struct qcom_nand_controller *snandc,

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/7] spi: spi-qpic-snand: use the common qcom_nandc_check_flash_errors() function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] spi: spi-qpic: OOB data corruption in qcom_spi_read_last_cw()
  [1]
--

commit e9db2cf0f483f41efa5a2350f20a79b1f0a4fe75
Author: Gabor Juhos <j4g8y7@gmail.com>

spi: spi-qpic-snand: use the common qcom_nandc_check_flash_errors() function

This commit replaces the local qcom_spi_check_raw_flash_errors() helper
with the common qcom_nandc_check_flash_errors() function from qpic_common.

> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 41685a286e381..981e16221b59e 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c

[ ... ]

> @@ -651,7 +635,7 @@ static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
>  		return ret;
>  	}
>  
> -	ret = qcom_spi_check_raw_flash_errors(snandc, 1);
> +	ret = qcom_nandc_check_flash_errors(snandc, 1);
>  	if (ret)
>  		return ret;

[Severity: Medium]
This problem wasn't introduced by this patch, but can qcom_spi_read_last_cw()
return corrupted OOB data [1] when a raw OOB read spans beyond the bad
block marker?

qcom_spi_read_page() routes all raw OOB-only reads (snandc->qspi->oob_rw &&
snandc->qspi->raw_rw) to qcom_spi_read_last_cw():

drivers/spi/spi-qpic-snand.c:qcom_spi_read_page() {
	...
	if (snandc->qspi->oob_rw && snandc->qspi->raw_rw)
		return qcom_spi_read_last_cw(snandc, op);
	...
}

However, qcom_spi_read_last_cw() only reads the final codeword into
snandc->data_buffer and copies op->data.nbytes starting from bbpos:

drivers/spi/spi-qpic-snand.c:qcom_spi_read_last_cw() {
	...
	bbpos = mtd->writesize - ecc_cfg->cw_size * (num_cw - 1);
	...
	memcpy(op->data.buf.in, snandc->data_buffer + bbpos, op->data.nbytes);
}

Because physical OOB bytes are distributed across all codewords in a
multi-codeword page, a full raw OOB read (such as via nanddump -n --oob)
misses the OOB data from earlier codewords and returns shifted data from
the final codeword instead.

[1] https://sashiko.dev/#/bug/linux-d2e2b583-de83-4f91-a87f-a7c888dd45c9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-qpic-snand-use-common-functions-v1-0-1e8601e2db84@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/7] spi: spi-qpic-snand: use the common qcom_nandc_config_page_read() function

[Gabor Juhos]

The common function from the 'qpic_common' module provides the same
functionality as the locally implemented counterpart.

Use the common function and remove the local one in order to reduce
code duplication.

No functional changes intended.

Note: in comparison to the local version, the generic function runs
additional code when the 'nandc->props->qpic_version2' flag is not
set, but that does not affect the behaviour of the current driver,
because the respective flag is set since commit 52efc89ccf66 ("spi:
spi-qpic-snand: set 'qpic_version2' flag").

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 981e16221b59..cf75c142303f 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -518,17 +518,6 @@ static void qcom_spi_set_read_loc(struct qcom_nand_controller *snandc, int cw, i
 						   read_size, is_last_read_loc);
 }
 
-static void qcom_spi_config_page_read(struct qcom_nand_controller *snandc)
-{
-	qcom_write_reg_dma(snandc, &snandc->regs->addr0, NAND_ADDR0, 2, 0);
-	qcom_write_reg_dma(snandc, &snandc->regs->cfg0, NAND_DEV0_CFG0, 3, 0);
-	qcom_write_reg_dma(snandc, &snandc->regs->erased_cw_detect_cfg_clr,
-			   NAND_ERASED_CW_DETECT_CFG, 1, 0);
-	qcom_write_reg_dma(snandc, &snandc->regs->erased_cw_detect_cfg_set,
-			   NAND_ERASED_CW_DETECT_CFG, 1,
-			   NAND_ERASED_CW_SET | NAND_BAM_NEXT_SGL);
-}
-
 static void
 qcom_spi_config_cw_read(struct qcom_nand_controller *snandc, bool use_ecc, int cw)
 {
@@ -588,7 +577,7 @@ static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
 static void qcom_spi_config_single_cw_page_read(struct qcom_nand_controller *snandc,
 						bool use_ecc, int cw)
 {
-	qcom_spi_config_page_read(snandc);
+	qcom_nandc_config_page_read(snandc);
 	qcom_spi_config_cw_read(snandc, use_ecc, cw);
 }
 
@@ -758,7 +747,7 @@ static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_bu
 	snandc->regs->exec = cpu_to_le32(1);
 
 	qcom_spi_set_read_loc(snandc, raw_cw, 0, 0, ecc_cfg->cw_size, 1);
-	qcom_spi_config_page_read(snandc);
+	qcom_nandc_config_page_read(snandc);
 
 	data_size1 = mtd->writesize - ecc_cfg->cw_size * (num_cw - 1);
 	oob_size1 = ecc_cfg->bbm_size;
@@ -863,7 +852,7 @@ static int qcom_spi_read_page_ecc(struct qcom_nand_controller *snandc,
 	snandc->regs->exec = cpu_to_le32(1);
 
 	qcom_clear_bam_transaction(snandc);
-	qcom_spi_config_page_read(snandc);
+	qcom_nandc_config_page_read(snandc);
 
 	for (i = 0; i < num_cw; i++) {
 		int data_size, oob_size;
@@ -944,7 +933,7 @@ static int qcom_spi_read_page_oob(struct qcom_nand_controller *snandc,
 	snandc->regs->ecc_bch_cfg = cpu_to_le32(ecc_bch_cfg);
 	snandc->regs->exec = cpu_to_le32(1);
 
-	qcom_spi_config_page_read(snandc);
+	qcom_nandc_config_page_read(snandc);
 
 	for (i = 0; i < num_cw; i++) {
 		int data_size, oob_size;

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/7] spi: spi-qpic-snand: use the common qcom_nandc_config_page_read() function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] spi: spi-qpic: unhandled DMA descriptor preparation
  failures [1]
--

commit 05d954fd8faf497964c92778fc951ed3a1d8862c
Author: Gabor Juhos <j4g8y7@gmail.com>

spi: spi-qpic-snand: use the common qcom_nandc_config_page_read() function

This commit removes the local qcom_spi_config_page_read() helper and replaces
its callers with the common qcom_nandc_config_page_read() function from
qpic_common.

> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 981e16221b59e..cf75c142303f8 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c

[ ... ]

> @@ -588,7 +577,7 @@ static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
>  static void qcom_spi_config_single_cw_page_read(struct qcom_nand_controller *snandc,
>  						bool use_ecc, int cw)
>  {
> -	qcom_spi_config_page_read(snandc);
> +	qcom_nandc_config_page_read(snandc);
>  	qcom_spi_config_cw_read(snandc, use_ecc, cw);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can
qcom_spi_config_single_cw_page_read() and qcom_nandc_config_page_read()
silently drop DMA descriptor preparation errors [1] and submit a
truncated BAM command sequence?

Both helpers are void and ignore the return value of qcom_write_reg_dma() ->
qcom_prep_bam_dma_desc_cmd(), which can fail with -ENOMEM under memory
pressure or -EINVAL if the BAM command element array is full.  The caller
then runs qcom_submit_descs() on the partially constructed descriptor list,
which can leave the QPIC SPI NAND controller in an undefined state or corrupt
flash I/O.

[1] https://sashiko.dev/#/bug/linux-28d007e2-6585-44cc-ae82-b3da2495b9ab

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-qpic-snand-use-common-functions-v1-0-1e8601e2db84@gmail.com?part=2

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/7] spi: spi-qpic-snand: use the common qcom_nandc_config_page_write() function

[Gabor Juhos]

The common function from the 'qpic_common' module provides the same
functionality as the locally implemented counterpart.

Use the common function and remove the local one in order to reduce
code duplication.

No functional changes intended.

Note: in comparison to the local version, the generic function runs
additional code when the 'nandc->props->qpic_version2' flag is not
set, but that does not affect the behaviour of the current driver,
because the respective flag is set since commit 52efc89ccf66 ("spi:
spi-qpic-snand: set 'qpic_version2' flag").

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index cf75c142303f..a825bdb93a8a 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -1063,12 +1063,6 @@ static int qcom_spi_read_page(struct qcom_nand_controller *snandc,
 	return 0;
 }
 
-static void qcom_spi_config_page_write(struct qcom_nand_controller *snandc)
-{
-	qcom_write_reg_dma(snandc, &snandc->regs->addr0, NAND_ADDR0, 2, 0);
-	qcom_write_reg_dma(snandc, &snandc->regs->cfg0, NAND_DEV0_CFG0, 3, 0);
-}
-
 static void qcom_spi_config_cw_write(struct qcom_nand_controller *snandc)
 {
 	qcom_write_reg_dma(snandc, &snandc->regs->cmd, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
@@ -1113,7 +1107,7 @@ static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
 	snandc->regs->ecc_bch_cfg = cpu_to_le32(ecc_bch_cfg);
 	snandc->regs->exec = cpu_to_le32(1);
 
-	qcom_spi_config_page_write(snandc);
+	qcom_nandc_config_page_write(snandc);
 
 	for (i = 0; i < num_cw; i++) {
 		int data_size1, data_size2, oob_size1, oob_size2;
@@ -1194,7 +1188,7 @@ static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
 	snandc->regs->ecc_bch_cfg = cpu_to_le32(ecc_bch_cfg);
 	snandc->regs->exec = cpu_to_le32(1);
 
-	qcom_spi_config_page_write(snandc);
+	qcom_nandc_config_page_write(snandc);
 
 	for (i = 0; i < num_cw; i++) {
 		int data_size, oob_size;
@@ -1275,7 +1269,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
 	/* override new oob content to last codeword */
 	mtd_ooblayout_get_databytes(snandc->qspi->mtd, snandc->data_buffer + data_size,
 				    oob_buf, 0, snandc->qspi->mtd->oobavail);
-	qcom_spi_config_page_write(snandc);
+	qcom_nandc_config_page_write(snandc);
 	qcom_write_data_dma(snandc, FLASH_BUF_ACC, snandc->data_buffer, data_size + oob_size, 0);
 	qcom_spi_config_cw_write(snandc);
 

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/7] spi: spi-qpic-snand: use the common qcom_nandc_config_page_write() function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] spi: qpic-snand: OOB data ignored during combined data and
  OOB operations [1]
--

commit 62e1a50299465a13da198b1f4ac1536c497b8877
Author: Gabor Juhos <j4g8y7@gmail.com>

spi: spi-qpic-snand: use the common qcom_nandc_config_page_write() function

This commit replaces the local qcom_spi_config_page_write() helper with the
common qcom_nandc_config_page_write() function from the qpic_common module.

> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index cf75c142303f8..a825bdb93a8a1 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c

[ ... ]

> @@ -1194,7 +1188,7 @@ static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
>  	snandc->regs->ecc_bch_cfg = cpu_to_le32(ecc_bch_cfg);
>  	snandc->regs->exec = cpu_to_le32(1);
>  
> -	qcom_spi_config_page_write(snandc);
> +	qcom_nandc_config_page_write(snandc);

[Severity: Critical]
This problem wasn't introduced by this patch, but does qcom_spi_program_ecc()
and qcom_spi_read_page_ecc() ignore the caller-supplied OOB data during
combined page and OOB operations [1]?

When the spinand framework performs a combined operation via
spinand_write_page() or spinand_read_page(), op->data.buf points to a single
contiguous buffer containing both the page data and OOB data.  However, both
functions hardcode oob_buf to the internal snandc->qspi->oob_buf without
copying the caller's OOB payload in from op->data.buf.out + mtd->writesize on
writes, or copying the read OOB data back out to op->data.buf.in +
mtd->writesize on reads:

drivers/spi/spi-qpic-snand.c:qcom_spi_program_ecc() {
	...
	if (snandc->qspi->data_buf)
		data_buf = snandc->qspi->data_buf;

	oob_buf = snandc->qspi->oob_buf;
	...
}

As a result, combined reads return stale or uninitialized OOB bytes to the
caller, and combined writes silently program garbage into the flash OOB area.

[1] https://sashiko.dev/#/bug/linux-7dd8ed16-d251-4477-8c16-a5a7a68cc6e1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-qpic-snand-use-common-functions-v1-0-1e8601e2db84@gmail.com?part=3

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/7] spi: spi-qpic-snand: use the common qcom_nandc_config_cw_write() function

[Gabor Juhos]

The common function from the 'qpic_common' module provides the same
functionality as the locally implemented counterpart.

Use the common function and remove the local one in order to reduce
code duplication.

No functional changes intended.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index a825bdb93a8a..6774a0a5db2a 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -1063,17 +1063,6 @@ static int qcom_spi_read_page(struct qcom_nand_controller *snandc,
 	return 0;
 }
 
-static void qcom_spi_config_cw_write(struct qcom_nand_controller *snandc)
-{
-	qcom_write_reg_dma(snandc, &snandc->regs->cmd, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
-	qcom_write_reg_dma(snandc, &snandc->regs->exec, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
-	qcom_read_reg_dma(snandc, NAND_FLASH_STATUS, 1, NAND_BAM_NEXT_SGL);
-
-	qcom_write_reg_dma(snandc, &snandc->regs->clrflashstatus, NAND_FLASH_STATUS, 1, 0);
-	qcom_write_reg_dma(snandc, &snandc->regs->clrreadstatus, NAND_READ_STATUS, 1,
-			   NAND_BAM_NEXT_SGL);
-}
-
 static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
 				const struct spi_mem_op *op)
 {
@@ -1144,7 +1133,7 @@ static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
 		qcom_write_data_dma(snandc, reg_off, oob_buf, oob_size2, 0);
 		oob_buf += oob_size2;
 
-		qcom_spi_config_cw_write(snandc);
+		qcom_nandc_config_cw_write(snandc);
 	}
 
 	ret = qcom_submit_descs(snandc);
@@ -1214,7 +1203,7 @@ static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
 			}
 		}
 
-		qcom_spi_config_cw_write(snandc);
+		qcom_nandc_config_cw_write(snandc);
 
 		if (data_buf)
 			data_buf += data_size;
@@ -1271,7 +1260,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
 				    oob_buf, 0, snandc->qspi->mtd->oobavail);
 	qcom_nandc_config_page_write(snandc);
 	qcom_write_data_dma(snandc, FLASH_BUF_ACC, snandc->data_buffer, data_size + oob_size, 0);
-	qcom_spi_config_cw_write(snandc);
+	qcom_nandc_config_cw_write(snandc);
 
 	ret = qcom_submit_descs(snandc);
 	if (ret) {

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/7] spi: spi-qpic-snand: use the common qcom_nandc_config_cw_write() function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] spi: qpic-snand: improper raw OOB write handling in
  qcom_spi_program_execute() [1]
--

commit 31ce15b93da7dda82ea19398b4abcc5e1c175880
Author: Gabor Juhos <j4g8y7@gmail.com>

spi: spi-qpic-snand: use the common qcom_nandc_config_cw_write() function

This commit replaces the local qcom_spi_config_cw_write() helper with the
common qcom_nandc_config_cw_write() function from qpic_common.

> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index a825bdb93a8a1..6774a0a5db2a7 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c

[ ... ]

> @@ -1271,7 +1260,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>  				    oob_buf, 0, snandc->qspi->mtd->oobavail);
>  	qcom_nandc_config_page_write(snandc);
>  	qcom_write_data_dma(snandc, FLASH_BUF_ACC, snandc->data_buffer, data_size + oob_size, 0);
> -	qcom_spi_config_cw_write(snandc);
> +	qcom_nandc_config_cw_write(snandc);
>  
>  	ret = qcom_submit_descs(snandc);
>  	if (ret) {

[Severity: Critical]
This problem wasn't introduced by this patch, but does qcom_spi_program_oob()
[1] silently discard raw OOB writes such as bad block markers from
spinand_markbad()?

spinand_markbad() issues a raw OOB write with datalen = 0, ooblen = 2, and
mode = MTD_OPS_RAW, which sets page_rw = false, oob_rw = true, and
raw_rw = true in qcom_spi_ecc_prepare_io_req_pipelined().  However,
qcom_spi_program_execute() ignores raw_rw when page_rw is false and routes
the write to qcom_spi_program_oob():

drivers/spi/spi-qpic-snand.c:qcom_spi_program_execute() {
	if (snandc->qspi->page_rw && snandc->qspi->raw_rw)
		return qcom_spi_program_raw(snandc, op);

	if (snandc->qspi->page_rw)
		return qcom_spi_program_ecc(snandc, op);

	if (snandc->qspi->oob_rw)
		return qcom_spi_program_oob(snandc, op);
    ...
}

qcom_spi_program_oob() sets oob_buf to snandc->qspi->data_buf (the start of
the page buffer rather than the OOB offset) and unconditionally enables ECC
via ecc_cfg->ecc_bch_cfg.  As a result, mtd_ooblayout_get_databytes() reads
0xff instead of the bad block marker and programs it to flash with ECC
enabled, leaving defective blocks unmarked and allowing subsequent writes to
failing blocks.

[1] https://sashiko.dev/#/bug/linux-da37c243-e4f1-4870-8190-46ae2f5b3282

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-qpic-snand-use-common-functions-v1-0-1e8601e2db84@gmail.com?part=4

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 5/7] spi: spi-qpic-snand: use the common qcom_nandc_set_read_loc_first() function

[Gabor Juhos]

The common function from the 'qpic_common' module provides the same
functionality as the locally implemented counterpart.

Use the common function and remove the local one in order to reduce
code duplication.

No functional changes intended.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 6774a0a5db2a..ad91edd5f6df 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -122,27 +122,6 @@ struct qpic_spi_nand {
 	bool quad_mode;
 };
 
-static void qcom_spi_set_read_loc_first(struct qcom_nand_controller *snandc,
-					int reg, int cw_offset, int read_size,
-					int is_last_read_loc)
-{
-	__le32 locreg_val;
-	u32 val = FIELD_PREP(READ_LOCATION_OFFSET_MASK, cw_offset) |
-		  FIELD_PREP(READ_LOCATION_SIZE_MASK, read_size) |
-		  FIELD_PREP(READ_LOCATION_LAST_MASK, is_last_read_loc);
-
-	locreg_val = cpu_to_le32(val);
-
-	if (reg == NAND_READ_LOCATION_0)
-		snandc->regs->read_location0 = locreg_val;
-	else if (reg == NAND_READ_LOCATION_1)
-		snandc->regs->read_location1 = locreg_val;
-	else if (reg == NAND_READ_LOCATION_2)
-		snandc->regs->read_location2 = locreg_val;
-	else if (reg == NAND_READ_LOCATION_3)
-		snandc->regs->read_location3 = locreg_val;
-}
-
 static void qcom_spi_set_read_loc_last(struct qcom_nand_controller *snandc,
 				       int reg, int cw_offset, int read_size,
 				       int is_last_read_loc)
@@ -514,8 +493,10 @@ static void qcom_spi_set_read_loc(struct qcom_nand_controller *snandc, int cw, i
 		return qcom_spi_set_read_loc_last(snandc, reg_base, cw_offset,
 						  read_size, is_last_read_loc);
 	else
-		return qcom_spi_set_read_loc_first(snandc, reg_base, cw_offset,
-						   read_size, is_last_read_loc);
+		return qcom_nandc_set_read_loc_first(snandc, reg_base,
+						     cw_offset,
+						     read_size,
+						     is_last_read_loc);
 }
 
 static void

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 6/7] spi: spi-qpic-snand: use the common qcom_nandc_set_read_loc_last() function

[Gabor Juhos]

The common function from the 'qpic_common' module provides the same
functionality as the locally implemented counterpart.

Use the common function and remove the local one in order to reduce
code duplication.

No functional changes intended.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index ad91edd5f6df..4105a04afcdb 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -122,27 +122,6 @@ struct qpic_spi_nand {
 	bool quad_mode;
 };
 
-static void qcom_spi_set_read_loc_last(struct qcom_nand_controller *snandc,
-				       int reg, int cw_offset, int read_size,
-				       int is_last_read_loc)
-{
-	__le32 locreg_val;
-	u32 val = FIELD_PREP(READ_LOCATION_OFFSET_MASK, cw_offset) |
-		  FIELD_PREP(READ_LOCATION_SIZE_MASK, read_size) |
-		  FIELD_PREP(READ_LOCATION_LAST_MASK, is_last_read_loc);
-
-	locreg_val = cpu_to_le32(val);
-
-	if (reg == NAND_READ_LOCATION_LAST_CW_0)
-		snandc->regs->read_location_last0 = locreg_val;
-	else if (reg == NAND_READ_LOCATION_LAST_CW_1)
-		snandc->regs->read_location_last1 = locreg_val;
-	else if (reg == NAND_READ_LOCATION_LAST_CW_2)
-		snandc->regs->read_location_last2 = locreg_val;
-	else if (reg == NAND_READ_LOCATION_LAST_CW_3)
-		snandc->regs->read_location_last3 = locreg_val;
-}
-
 static struct qcom_nand_controller *nand_to_qcom_snand(struct nand_device *nand)
 {
 	struct nand_ecc_engine *eng = nand->ecc.engine;
@@ -490,8 +469,9 @@ static void qcom_spi_set_read_loc(struct qcom_nand_controller *snandc, int cw, i
 	reg_base += reg * 4;
 
 	if (cw == (num_cw - 1))
-		return qcom_spi_set_read_loc_last(snandc, reg_base, cw_offset,
-						  read_size, is_last_read_loc);
+		return qcom_nandc_set_read_loc_last(snandc, reg_base, cw_offset,
+						    read_size,
+						    is_last_read_loc);
 	else
 		return qcom_nandc_set_read_loc_first(snandc, reg_base,
 						     cw_offset,

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 7/7] spi: spi-qpic-snand: use the common 'qcom_nandc_read_stats' structure

[Gabor Juhos]

The 'qcom_nandc_read_stats' structure defined in the 'nand-qpic-common.h'
header and the locally defined 'snandc_read_status' structure are
identical type wise and both are serving the same purpose.

Drop the local structure and use the global one instead to reduce
duplication.

No functional changes.

Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 4105a04afcdb..6c5878f3f8b6 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -64,12 +64,6 @@
 #define BAD_BLOCK_MARKER_SIZE		0x2
 #define OOB_BUF_SIZE			128
 
-struct snandc_read_status {
-	__le32 snandc_flash;
-	__le32 snandc_buffer;
-	__le32 snandc_erased_cw;
-};
-
 /*
  * ECC state struct
  * @corrected:		ECC corrected
@@ -610,7 +604,7 @@ static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
 
 static int qcom_spi_check_error(struct qcom_nand_controller *snandc)
 {
-	struct snandc_read_status *buf;
+	struct qcom_nandc_read_stats *buf;
 	struct qpic_ecc *ecc_cfg = snandc->qspi->ecc;
 	int i, num_cw = snandc->qspi->num_cw;
 	bool flash_op_err = false, erased;
@@ -621,14 +615,14 @@ static int qcom_spi_check_error(struct qcom_nand_controller *snandc)
 	snandc->qspi->ecc_stats.corrected = 0;
 
 	qcom_nandc_dev_to_mem(snandc, true);
-	buf = (struct snandc_read_status *)snandc->reg_read_buf;
+	buf = (struct qcom_nandc_read_stats *)snandc->reg_read_buf;
 
 	for (i = 0; i < num_cw; i++, buf++) {
 		u32 flash, buffer, erased_cw;
 
-		flash = le32_to_cpu(buf->snandc_flash);
-		buffer = le32_to_cpu(buf->snandc_buffer);
-		erased_cw = le32_to_cpu(buf->snandc_erased_cw);
+		flash = le32_to_cpu(buf->flash);
+		buffer = le32_to_cpu(buf->buffer);
+		erased_cw = le32_to_cpu(buf->erased_cw);
 
 		if ((flash & FS_OP_ERR) && (buffer & BS_UNCORRECTABLE_BIT)) {
 			if (ecc_cfg->bch_enabled)

-- 
2.56.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/7] spi: spi-qpic-snand: use generic code from the 'qpic_common' module

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 405 bytes --]

On Mon, Nov 09, 2026 at 08:41:01PM +0100, Gabor Juhos wrote:

> base-commit: 4a8faaea975be15cb636646c0d4c6ff1c2110baa
> change-id: 20261028-qpic-snand-use-common-functions-12b18fc7a1e5
> prerequisite-change-id: 20260922-qpic-snand-fix-singlecw-readloc-c4c89b4369f5:v2
> prerequisite-patch-id: 7f8a48bccb2e3c8de950bbddacfefe052cea50b8

I can't apply this to anything, please resend when it can be applied.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/7] spi: spi-qpic-snand: use generic code from the 'qpic_common' module

[Gabor Juhos]

2026. 11. 10. 19:46 keltezéssel, Mark Brown írta:
> On Mon, Nov 09, 2026 at 08:41:01PM +0100, Gabor Juhos wrote:
> 
>> base-commit: 4a8faaea975be15cb636646c0d4c6ff1c2110baa
>> change-id: 20261028-qpic-snand-use-common-functions-12b18fc7a1e5
>> prerequisite-change-id: 20260922-qpic-snand-fix-singlecw-readloc-c4c89b4369f5:v2
>> prerequisite-patch-id: 7f8a48bccb2e3c8de950bbddacfefe052cea50b8
> 
> I can't apply this to anything, please resend when it can be applied.

Sorry for the trouble, it depends on the following patch:

Link:
https://lore.kernel.org/r/20261106-qpic-snand-fix-singlecw-readloc-v2-1-18de8226fe15@gmail.com
I will resend the series once that one gets applied.

I assumed that adding the prerequisite-change-id is enough to claim the
dependency. I will state the dependencies explicitly next time.

Regards,
Gabor

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/7] spi: spi-qpic-snand: use generic code from the 'qpic_common' module

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1034 bytes --]

On Tue, Nov 10, 2026 at 10:18:11PM +0100, Gabor Juhos wrote:
> 2026. 11. 10. 19:46 keltezéssel, Mark Brown írta:
> > On Mon, Nov 09, 2026 at 08:41:01PM +0100, Gabor Juhos wrote:

> >> base-commit: 4a8faaea975be15cb636646c0d4c6ff1c2110baa
> >> change-id: 20261028-qpic-snand-use-common-functions-12b18fc7a1e5
> >> prerequisite-change-id: 20260922-qpic-snand-fix-singlecw-readloc-c4c89b4369f5:v2
> >> prerequisite-patch-id: 7f8a48bccb2e3c8de950bbddacfefe052cea50b8

> > I can't apply this to anything, please resend when it can be applied.

> I assumed that adding the prerequisite-change-id is enough to claim the
> dependency. I will state the dependencies explicitly next time.

My whole process for tracking and reviewing things depends on actually
being able to apply them, and even with things tracked it's far too much
work to follow random stacks of dependencies.  It's getting frustrating
how common it is becoming for people to send out things that depend on
multi-level stacks of random in flight patches.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/