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

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

Pre-existing issues:
- [High] Missing controller lock for SPMI transactions, leading to data corruption and racy shared state.
--

--- Patch [2]: [PATCH v6 2/6] spmi: apple: Validate FIFO state ---
commit 70a1216b809937a36a9579df121874ded0a12def
Author: Alba Mendez <[email protected]>

spmi: apple: Validate FIFO state

This commit adds checks for data before reading a reply body and checks for
the end of data afterwards. It also introduces a state flag to track previous
failures and flush the FIFO accordingly.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 376cf682c43e0..15721cb41d5c8 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -67,6 +71,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	u8 i;
>  	int ret;
>  
> +	if (spmi->prev_fail) {
> +		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
> +		spmi->prev_fail = false;
> +	}
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for multiple
clients on the SPMI bus to execute transactions concurrently?

The driver does not appear to hold a lock when executing SPMI transactions.
If multiple clients on the SPMI bus (e.g., a PMIC and a GPIO controller)
attempt concurrent reads or writes, their accesses to the shared hardware
FIFOs will interleave, potentially violating the transaction protocol and
corrupting data.

This patch exacerbates the issue in spmi_read_cmd by adding spmi->prev_fail,
which is evaluated and modified here without mutual exclusion. Could this
create a race condition where multiple threads recovering from an error
simultaneously flush the active FIFO, potentially destroying in-flight
commands from other threads?

[ ... ]
> @@ -97,6 +117,11 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	size_t i = 0, j;
>  	int ret;
>  
> +	if (spmi->prev_fail) {
> +		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
> +		spmi->prev_fail = false;
> +	}
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: High]
This is a pre-existing issue, but similar to spmi_read_cmd, multi-word
writes to the shared TX FIFO and access to driver state occur here without
locking. Could concurrent accesses from different clients lead to interleaved
writes and data corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2