Re: [PATCH 2/7] spmi: apple: Validate FIFO state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver detects unexpected extra data in the RX FIFO but fails to drain it, leaving the hardware permanently desynchronized for subsequent SPMI commands.
- [High] The driver lacks synchronization locks, allowing concurrent SPMI bus transactions to interleave and corrupt the command and FIFO state.
--

--- Patch [2]: [PATCH 2/7] spmi: apple: Validate FIFO state ---
commit c5c6cb6b2f161dd887167b0a07acf4d194f4cc70
Author: Alba Mendez <me@alba.sh>

spmi: apple: Validate FIFO state

This patch adds checks to ensure there is data before reading the body of a
reply, and checks for the end of data afterwards.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 376cf682c43e0..b4f442d0b4ad7 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -78,6 +78,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,

[Severity: High]
This isn't a bug introduced by this patch, but I noticed spmi_read_cmd()
performs a multi-step bus transaction without acquiring any locks to prevent
concurrent interleaving.

Since the SPMI subsystem core does not serialize callbacks across different
slave devices, if two threads concurrently initiate SPMI transactions, could
their command writes and FIFO reads interleave? This might lead to data
corruption or misdirected responses on a busy bus.

>  	/* Read SPMI data reply */
>  	while (len_read < len) {
> +		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
> +			return -EIO;
> +		}
>  		rsp = 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");

[Severity: High]
This is a pre-existing issue, but since we are now checking for extra data
at the end of spmi_read_cmd(), shouldn't we also drain the FIFO here?

If a device misbehaves and leaves extra data in the RX FIFO, the next SPMI
read or write will immediately see this stale data. It could consume it as
its own response, leaving its actual response in the FIFO, creating a
permanent cascading desynchronization on the bus.

>  	return 0;
>  }
>  
> @@ -115,6 +122,9 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,

[Severity: High]
This is a pre-existing issue, but similar to the read command above,
spmi_write_cmd() also appears to lack synchronization locks.

Would it be safer to wrap the entire transaction in a mutex to prevent
concurrent bus transactions from corrupting the command and FIFO state?

>  	/* Discard */
>  	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");

[Severity: High]
This isn't a bug introduced by this patch, but does this code need to loop
and read from the FIFO until it is actually empty? Otherwise, the hardware
may remain permanently desynchronized for subsequent commands.

>  	return 0;
>  }

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