Re: [PATCH 3/7] spmi: apple: check transaction status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing serialization locks for SPMI transactions leads to hardware state corruption.
- [High] Missing RX FIFO flush on timeouts and extra data permanently corrupts bus state.
--

--- Patch [3]: [PATCH 3/7] spmi: apple: check transaction status ---
commit acc23a1dd1c948ebf37045f3db6e344612bde6a3
Author: Alba Mendez <me@alba.sh>

spmi: apple: check transaction status

This commit adds parity error checks and command acknowledgement checks for
SPMI read and write transactions. It ensures the reply frame is read instead
of being discarded to properly evaluate the transaction status.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index b4f442d0b4ad7..4678a9e5270aa 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -73,8 +79,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (ret)
>  		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this function need a
serialization lock for the full multi-step transaction?

The SPMI core subsystem does not serialize read_cmd and write_cmd callbacks,
leaving locking to the controller driver. If multiple threads concurrently
issue SPMI reads or writes via independent client drivers on the same bus,
could their sequences of writing to SPMI_CMD_REG and reading SPMI_RSP_REG
interleave, resulting in hardware state corruption?

Additionally, as another pre-existing issue, if a call to
apple_spmi_wait_rx_not_empty() times out due to a delayed hardware response,
the driver returns an error but doesn't appear to reset the controller or
flush the late response.

If a late response eventually fills the RX FIFO, will the next command's wait
for RX FIFO incorrectly succeed and consume the stale data as its own reply,
corrupting the bus state?

>  
> -	/* Discard SPMI reply status */
> -	readl(spmi->regs + SPMI_RSP_REG);
> +	reply = readl(spmi->regs + SPMI_RSP_REG);
>  
>  	/* Read SPMI data reply */
>  	while (len_read < len) {

[ ... ]

> @@ -93,6 +98,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY))
>  		dev_warn(&ctrl->dev, "FIFO has extra data\n");

[Severity: High]
This is a pre-existing issue, but if extra data is detected in the FIFO, the
driver logs a warning but doesn't appear to drain the remaining words.

Since stale data is left in the RX FIFO, couldn't this guarantee corruption
of the next transaction on the bus?

> +	if ((~reply >> SPMI_REPLY_FRAME_PARITY_OFFSET) & ((1 << len) - 1)) {
> +		dev_err(&ctrl->dev, "some frames failed parity check\n");
> +		return -EIO;
> +	}
>  	return 0;
>  }

[ ... ]

> @@ -101,6 +110,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  {
>  	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
>  	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
> +	u32 reply;
>  	size_t i = 0, j;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but similar to spmi_read_cmd(), does this
function need a serialization lock for its multi-step write sequence?

Since there is no synchronization around the hardware interactions, multiple
un-synchronized writes to the shared SPMI_CMD_REG from concurrent threads
could interleave and destroy the controller's state machine.

> @@ -119,12 +129,15 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (ret)
>  		return ret;
>  
> -	/* Discard */
> -	readl(spmi->regs + SPMI_RSP_REG);
> +	reply = readl(spmi->regs + SPMI_RSP_REG);
>  
>  	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY))
>  		dev_warn(&ctrl->dev, "FIFO has extra data\n");
>  
> +	if (!(reply & SPMI_REPLY_ACK)) {
> +		dev_err(&ctrl->dev, "command not acknowledged\n");
> +		return -EIO;
> +	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-t603x-spmi-v1-0-e1a29fcd2d38@chaosmail.tech?part=3