Re: [PATCH v5 2/2] spi: spi-mem: Add Realtek SPI NOR flash controller driver
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-30 07:11:26
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] spi: rtk-nor: missing error handling for pm_runtime_resume_and_get() in remove path - [High] spi: rtk-nor: rtk_spi_supports_op() returns true for unsupported commands due to bool coercion - [High] spi: rtk-nor: rtk_spi_do_write_and_cmds() silently truncates multi-byte writes - [High] spi: rtk-nor: stateful 4-byte mode tracking breaks dirmap memory-mapped accesses - [High] spi: rtk-nor: missing DUAL_MODE_EN for write operations and exec_op reads causes data corruption -- commit 3bdf61831cde1ea53f0bf3d584243f565bbd2cc7 Author: Jyan Chou [off-list ref] spi: spi-mem: Add Realtek SPI NOR flash controller driver Add a spi-mem driver for the SPI NOR Flash Controller (SFC) found on Realtek DHC SoC. The controller supports single and dual I/O transfers and offloads the payload to an integrated DMA engine.
quoted hunk ↗ jump to hunk
diff --git a/drivers/spi/spi-rtk-nor.c b/drivers/spi/spi-rtk-nor.c --- /dev/null +++ b/drivers/spi/spi-rtk-nor.c
[ ... ]
quoted hunk ↗ jump to hunk
+static void rtk_spi_read_mode(struct rtk_spi_host *host, const struct spi_mem_op *op) +{ + u32 opcode = op->cmd.opcode; + u32 dummy_cycles, val; + + if (op->data.buswidth == 2) + opcode |= DUAL_MODE_EN; + + writel(opcode, host->regbase + SFC_OPCODE); + + val = readl(host->regbase + SFC_CTL); + val |= RW_DATAEN | ADDR_EN;
[Severity: High] If a stateless 3-byte command (like Read SFDP) is issued via rtk_spi_exec_op() while the flash is in 4-byte mode, host->is_4byte is set to false and the hardware register is updated. If a subsequent dirmap read bypasses rtk_spi_exec_op() and calls rtk_spi_read_mode() here, it appears the 4-byte enable register (SFC_ADR_FOUR_BYTE_EN) is not updated from the dirmap template. Could this cause the controller to send a 3-byte address for a 4-byte dirmap operation, resulting in garbage data reads? [ ... ]
quoted hunk ↗ jump to hunk
+static void rtk_spi_write_mode(struct rtk_spi_host *host, + const struct spi_mem_op *op) +{ + u32 opcode = op->cmd.opcode; + u32 val; + + writel(opcode, host->regbase + SFC_OPCODE);
[Severity: High] rtk_spi_supports_op() accepts buswidth == 2 for all data operations, and rtk_spi_read_mode() above properly sets the DUAL_MODE_EN bit for dirmap reads. Is DUAL_MODE_EN missing here in rtk_spi_write_mode()? If the SPI NOR core initiates a dual I/O Page Program, will the controller execute it in standard 1-1-1 mode and potentially misconfigure the hardware? [ ... ]
quoted hunk ↗ jump to hunk
+static int rtk_spi_command_read(struct rtk_spi_host *host, const struct spi_mem_op *op) +{ + size_t len = op->data.nbytes; + loff_t offset = op->addr.val; + u8 opcode = op->cmd.opcode; + u32 dummy_cycles, val; + + writel(opcode, host->regbase + SFC_OPCODE);
[Severity: High] Similarly, does this code also miss setting DUAL_MODE_EN for exec_op reads when dual I/O is requested? [ ... ]
quoted hunk ↗ jump to hunk
+static int rtk_spi_do_write_and_cmds(struct rtk_spi_host *host, const struct spi_mem_op *op) +{ + u8 opcode = op->cmd.opcode; + u32 ctl_val = 0; + + writel(opcode, host->regbase + SFC_OPCODE); + + if (op->data.nbytes > 0) + ctl_val |= RW_DATAEN; + + if (op->addr.nbytes > 0) + ctl_val |= ADDR_EN; + + writel(ctl_val, host->regbase + SFC_CTL); + + if (op->data.nbytes > 0) { + const u8 *buf = op->data.buf.out; + + if (op->addr.val + op->data.nbytes > host->dirmap_size) + return -EINVAL; + + writeb(*buf, host->iobase + op->addr.val); + } else {
[Severity: High] When exec_op is used for multi-byte writes (where op->data.nbytes > 1), this executes a single writeb() without a loop and returns success. Does this silently discard the rest of the payload in op->data.buf.out? If the SPI NOR core falls back to exec_op for multi-byte register writes, it appears only the first byte is written. [ ... ]
quoted hunk ↗ jump to hunk
+static bool rtk_spi_supports_op(struct spi_mem *mem, + const struct spi_mem_op *op) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ /* + * The data window is access-triggered: each CPU access to host->iobase + * generates one complete SPI transaction. Consecutive byte accesses + * therefore cannot be merged into a single transaction, so + * multi-byte stateless commands are not supported by this controller. + */ + if (!op->addr.nbytes && op->data.nbytes > 1) + return -EOPNOTSUPP;
[Severity: High] Since rtk_spi_supports_op() returns a bool, won't returning -EOPNOTSUPP evaluate to true? Does this cause the driver to erroneously advertise support for multi-byte stateless commands instead of rejecting them? [ ... ]
quoted hunk ↗ jump to hunk
+static void rtk_spi_remove(struct platform_device *pdev) +{ + struct spi_controller *ctrl = platform_get_drvdata(pdev); + struct rtk_spi_host *host = spi_controller_get_devdata(ctrl); + struct device *dev = &pdev->dev; + + spi_unregister_controller(ctrl); + + if (pm_runtime_resume_and_get(dev) < 0) + dev_warn(dev, "Failed to resume device during remove\n"); + + pm_runtime_put_noidle(dev); + pm_runtime_disable(dev); + pm_runtime_set_suspended(dev); + pm_runtime_dont_use_autosuspend(dev); + reset_control_assert(host->rstc); + clk_disable_unprepare(host->clk); +}
[Severity: High] If pm_runtime_resume_and_get(dev) fails (returns < 0), it does not increment the device's PM usage counter. Since the execution unconditionally proceeds to call pm_runtime_put_noidle(dev) and clk_disable_unprepare(host->clk), will this result in a PM usage counter underflow (double put) and a clock enable counter underflow? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260930065945.88008-1-eleanor.lin@realtek.com?part=2