Re: [PATCH v5 2/2] spi: spi-mem: Add Realtek SPI NOR flash controller driver

[sashiko-bot@kernel.org]

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 <jyanchou@realtek.com>

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.

> 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
[ ... ]
> +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?

[ ... ]
> +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?

[ ... ]
> +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?

[ ... ]
> +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.

[ ... ]
> +static bool rtk_spi_supports_op(struct spi_mem *mem,
> +				const struct spi_mem_op *op)
> +{
[ ... ]
> +	/*
> +	 * 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?

[ ... ]
> +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