Re: [PATCH v1 11/11] spi: dw: Add support for StarFive JHB100 SoC SFC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The JHB100 quirk does not properly validate or handle address lengths other than 3 or 4 bytes, leading to syscon state desynchronization and silent address truncation.
- [High] The JHB100 filter interrupt mask register is masked during initialization but is not restored on system resume.
--

--- Patch [11]: [PATCH v1 11/11] spi: dw: Add support for StarFive JHB100 SoC SFC ---
commit 3a943425e81f5dc20e515a24b6937c56ee3c08e8
Author: Changhuang Liang <changhuang.liang@starfivetech.com>

spi: dw: Add support for StarFive JHB100 SoC SFC

Add support for the StarFive JHB100 SoC SPI Flash Controller (SFC). It
introduces customizations over the Synopsys DW SSI v2.00a, including
separate instruction/address registers, a filter interrupt mask register,
and requires a syscon phandle for address mode switching.

> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index ef1abb540f069..c4e53c7456b26 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
[ ... ]
> -static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_op *op)
> +static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_op *op,
> +				      struct spi_mem *mem)
>  {
> -	/* Send cmd as 32 bit value */
> -	dw_write_io_reg(dws, DW_SPI_DR, op->cmd.opcode);
> -	if (op->addr.nbytes) {
> -		dw_write_io_reg(dws, DW_SPI_DR, lower_32_bits(op->addr.val));
> -		if (op->addr.nbytes > 4) {
> -			/* address more than 32bit */
> -			dw_write_io_reg(dws, DW_SPI_DR, upper_32_bits(op->addr.val));
> +	if (dws->quirk_flags & DW_SPI_QUIRK_JHB100) {
> +		dw_write_io_reg(dws, DW_SPI_JHB100_INST, op->cmd.opcode);
> +		if (op->addr.nbytes)
> +			dw_write_io_reg(dws, DW_SPI_JHB100_ADDR, op->addr.val);

[Severity: High]
If op->addr.nbytes is greater than 4, does this 32-bit register write via
dw_write_io_reg() silently truncate the 64-bit op->addr.val address value?

The SPI core can issue mem_op structures with address lengths up to 7 bytes 
(which dw_spi_supports_enh_mem_op allows). Could standard SPI flash
operations utilizing these non-standard address lengths experience data
corruption or communication failure due to this silent dropping of upper bits?

[ ... ]
> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
> index 603e81a92c57a..236ac5fa9cd06 100644
> --- a/drivers/spi/spi-dw-mmio.c
> +++ b/drivers/spi/spi-dw-mmio.c
[ ... ]
> +static void dw_spi_jhb100_set_addr_nbyte(struct spi_device *spi, u8 nbyte)
> +{
> +	struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
> +	struct dw_spi_mmio *dwsmmio = container_of(dws, struct dw_spi_mmio, dws);
> +	struct regmap *syscon = dwsmmio->priv;
> +
> +	if (nbyte == 3) {
> +		regmap_update_bits(syscon, JHB100_ADDRMODE_CS,
> +				   BIT(spi_get_chipselect(spi, 0)),
> +				   0);
> +	} else if (nbyte == 4) {
> +		regmap_update_bits(syscon, JHB100_ADDRMODE_CS,
> +				   BIT(spi_get_chipselect(spi, 0)),
> +				   BIT(spi_get_chipselect(spi, 0)));
> +	}
> +}

[Severity: High]
What happens if nbyte is something other than 3 or 4 (like 1, 2, or 5)?

Does this silently ignore unsupported address lengths instead of rejecting
them, potentially leaving the syscon hardware in a stale or misconfigured
state during transfers?

> +
> +static int dw_spi_jhb100_init(struct platform_device *pdev,
> +			      struct dw_spi_mmio *dwsmmio)
> +{
> +	struct regmap *syscon;
> +
> +	syscon = syscon_regmap_lookup_by_phandle(dev_of_node(&pdev->dev),
> +						 "starfive,sfc-filter-syscon");
> +	if (IS_ERR(syscon))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(syscon),
> +				     "syscon regmap lookup failed\n");
> +
> +	dwsmmio->priv = syscon;
> +
> +	dwsmmio->dws.set_addr_nbyte = dw_spi_jhb100_set_addr_nbyte;
> +	dwsmmio->dws.ip = DW_HSSI_ID;
> +	dwsmmio->dws.quirk_flags = DW_SPI_QUIRK_JHB100;
> +
> +	dw_spi_jhb100_mask_intr(&dwsmmio->dws, 0xff);

[Severity: High]
Since this applies the interrupt mask during probe initialization, is there
corresponding logic needed to restore this register during PM resume?

If the SPI controller's power domain is lost during suspend,
DW_SPI_JHB100_FILTER_IMR might revert to its hardware reset value on resume.

The mmio suspend/resume path (dw_spi_mmio_resume) calls
dw_spi_resume_controller(), which ultimately executes dw_spi_hw_init(). 
However, that function resets standard registers and lacks any awareness of
this quirk-specific DW_SPI_JHB100_FILTER_IMR register.

If the default reset state leaves the filter interrupts unmasked, could this
trigger an unhandled interrupt storm locking up the CPU after the system
resumes?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709055204.138168-1-changhuang.liang@starfivetech.com?part=11