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

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

Pre-existing issues:
- [High] Missing concurrency protection for shared hardware controller registers.
--

--- Patch [3]: [PATCH v6 3/6] spmi: apple: check transaction status ---
commit 7a35e38337e2fbd8de6bb123e810dc5312c6e28b
Author: Alba Mendez <[email protected]>
spmi: apple: check transaction status

Check for parity errors and missing command ACKs.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 15721cb41d5c8..b1c127cf5f441 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -66,7 +74,7 @@ static int spmi_read_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 rsp;
> +	u32 reply, rsp;
>  	size_t len_read = 0;
>  	u8 i;
>  	int ret;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for multiple
kernel threads to execute spmi_read_cmd() or spmi_write_cmd() concurrently
without locking?

If different threads communicate with multiple PMIC endpoints on the same SPMI
bus, it appears concurrent operations might invoke these functions
simultaneously. The core SPMI subsystem does not serialize transactions across
different slaves.

In both functions, the driver writes to SPMI_CMD_REG and then enters a polling
wait without holding any locks. For example, in spmi_read_cmd():

	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

	ret = apple_spmi_wait_rx_not_empty(ctrl);

Could this unprotected wait window allow interleaved accesses to corrupt the
shared command, status, and response registers? Is a mutex or spinlock
required here to protect the active transaction state from concurrent PMIC
transactions?

[ ... ]

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