Re: [PATCH v5 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] The SPMI controller lacks a lock to serialize concurrent read/write commands, leading to hardware FIFO corruption and race conditions on the `prev_fail` flag.
--

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

spmi: apple: Validate FIFO state

This commit adds validation of the FIFO state 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..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 is a pre-existing issue, but does the lack of locking here allow
concurrent accesses to corrupt the hardware FIFO?

Because the hardware sequence involves multiple unprotected MMIO operations,
interleaving these steps from concurrent threads can mix the command and
response streams. This can happen if two different SPMI devices on the same
controller are accessed concurrently, as the SPMI framework core does not
serialize accesses across the controller.

Additionally, could the introduction of the prev_fail flag create a data
race? If one thread evaluates prev_fail and flushes the FIFO just after a
concurrent thread has written its command, it might actively abort the
concurrent transaction.

[ ... ]
> @@ -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 isn't a bug introduced by this patch, but similarly to spmi_read_cmd(),
does this unprotected multi-step sequence allow for concurrent accesses to
corrupt the hardware FIFO?

Could the same race condition with the prev_fail flag occur here if another
thread is currently reading or writing to the controller?

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