diff mbox series

[v3,13/15] mtd: rawnand: sunxi: avoid duplicate chip setup before page commands

Message ID 20260909-submit-sunxi-nand-vendor-oob-layout-v1-v3-13-838cb0ba1547@gmail.com (mailing list archive)
State New
Headers
Series mtd: rawnand: sunxi: support the Allwinner randomized OOB format |

Commit Message

James Hilliard Sept. 9, 2026, 8:30 a.m. UTC 
The page helpers invoke exec_op(), which selects the chip and programs
its timing and spare-area registers. Remove the identical setup directly
before those helpers in the read callbacks and PIO write callbacks.

Keep explicit selection in the DMA write callback: it accesses controller
registers and prepares DMA before issuing the program-begin operation.
Do not cache controller state or change selection after a controller reset.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/mtd/nand/raw/sunxi_nand.c | 12 ------------
 1 file changed, 12 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 9, 2026, 8:54 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Wait queue spinlock corruption and false completion due to concurrent complete() and init_completion() after a timeout.
--

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

mtd: rawnand: sunxi: avoid duplicate chip setup before page commands

The page helpers invoke exec_op(), which selects the chip and programs
its timing and spare-area registers. Remove the identical setup directly
before those helpers in the read callbacks and PIO write callbacks.

> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index dc5b3814657e7..0d829103b144b 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -1743,8 +1743,6 @@ static int sunxi_nfc_hw_ecc_read_page(struct nand_chip *nand, uint8_t *buf,
>  	int ret, i, cur_off = 0;
>  	bool erased_chunk_found = false;
>  
> -	sunxi_nfc_select_chip(nand, nand->cur_cs);
> -
>  	ret = nand_read_page_op(nand, page, 0, NULL, 0);
>  	if (ret)
>  		return ret;

[ ... ]

While reviewing this code, I noticed a separate issue in sunxi_nfc_wait_events():

drivers/mtd/nand/raw/sunxi_nand.c:sunxi_nfc_wait_events() {
    ...
	if (!use_polling) {
		init_completion(&nfc->complete);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing issue. Can
this lead to wait queue spinlock corruption and false completions?

If a NAND operation times out in sunxi_nfc_wait_events(), CPU A clears the
interrupt enable register and returns -ETIMEDOUT.

Concurrently, if the IRQ handler is delayed, CPU B executing
sunxi_nfc_interrupt() may read the active status before CPU A clears it, but
get preempted before calling complete().

CPU A then starts a new NAND operation and re-enters sunxi_nfc_wait_events():

drivers/mtd/nand/raw/sunxi_nand.c:sunxi_nfc_wait_events() {
    ...
	if (!use_polling) {
		init_completion(&nfc->complete);
    ...
}

When CPU B resumes and calls complete(), wouldn't it access and modify the
lock concurrently, causing lock corruption?

Furthermore, wouldn't it set the completion flag for the new command, causing
it to return prematurely and process garbage data?

Since there is no structural prevention like synchronize_irq() or
disable_irq() on the timeout path, does this need to be addressed?
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
index bbfed22640c8..9dc742ee4344 100644
--- a/drivers/mtd/nand/raw/sunxi_nand.c
+++ b/drivers/mtd/nand/raw/sunxi_nand.c
@@ -1743,8 +1743,6 @@  static int sunxi_nfc_hw_ecc_read_page(struct nand_chip *nand, uint8_t *buf,
 	int ret, i, cur_off = 0;
 	bool erased_chunk_found = false;
 
-	sunxi_nfc_select_chip(nand, nand->cur_cs);
-
 	ret = nand_read_page_op(nand, page, 0, NULL, 0);
 	if (ret)
 		return ret;
@@ -1793,8 +1791,6 @@  static int sunxi_nfc_hw_ecc_read_page_dma(struct nand_chip *nand, u8 *buf,
 {
 	int ret;
 
-	sunxi_nfc_select_chip(nand, nand->cur_cs);
-
 	ret = nand_read_page_op(nand, page, 0, NULL, 0);
 	if (ret)
 		return ret;
@@ -1823,8 +1819,6 @@  static int sunxi_nfc_hw_ecc_read_subpage(struct nand_chip *nand,
 	if (sunxi_nand->randomized_oob)
 		return sunxi_nfc_hw_ecc_read_page(nand, bufpoi, false, page);
 
-	sunxi_nfc_select_chip(nand, nand->cur_cs);
-
 	ret = nand_read_page_op(nand, page, 0, NULL, 0);
 	if (ret)
 		return ret;
@@ -1865,8 +1859,6 @@  static int sunxi_nfc_hw_ecc_read_subpage_dma(struct nand_chip *nand,
 	if (to_sunxi_nand(nand)->randomized_oob)
 		return sunxi_nfc_hw_ecc_read_page_dma(nand, buf, false, page);
 
-	sunxi_nfc_select_chip(nand, nand->cur_cs);
-
 	ret = nand_read_page_op(nand, page, 0, NULL, 0);
 	if (ret)
 		return ret;
@@ -1890,8 +1882,6 @@  static int sunxi_nfc_hw_ecc_write_page(struct nand_chip *nand,
 	struct nand_ecc_ctrl *ecc = &nand->ecc;
 	int ret = 0, i, cur_off = 0;
 
-	sunxi_nfc_select_chip(nand, nand->cur_cs);
-
 	ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
 	if (ret)
 		return ret;
@@ -1935,8 +1925,6 @@  static int sunxi_nfc_hw_ecc_write_subpage(struct nand_chip *nand,
 	struct nand_ecc_ctrl *ecc = &nand->ecc;
 	int ret = 0, i, cur_off = 0;
 
-	sunxi_nfc_select_chip(nand, nand->cur_cs);
-
 	ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
 	if (ret)
 		return ret;