Re: [PATCH v2 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 synchronization (lock) around SPMI register accesses leads to hardware register corruption.
- [High] The driver does not flush the RX FIFO before initiating a new command, making it highly vulnerable to stream desynchronization.
--

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

spmi: apple: check transaction status

This commit adds checks for parity errors and missing command ACKs in the
Apple SPMI controller driver. It reads the reply status from the SPMI
response register and returns an error if the transaction fails.

> 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
[ ... ]
> @@ -62,7 +68,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);

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying
spmi_read_cmd(), is there a missing lock around the SPMI register accesses?

When the SPMI command is written to the hardware register via
writel(spmi_cmd, spmi->regs + SPMI_CMD_REG), concurrent SPMI transactions
from different client drivers (like regulators or RTCs) could interleave
their command writes and FIFO reads. Since the SPMI core does not serialize
these callbacks, shouldn't the controller driver implement its own locking
to prevent overwriting SPMI_CMD_REG before a transaction completes?

[Severity: High]
This is also a pre-existing issue, but does the driver need to flush the
RX FIFO before initiating a new command?

When a new command is issued via writel(spmi_cmd, spmi->regs + SPMI_CMD_REG),
if a previous transaction timed out or a slave responded with excess data,
leftover bytes might remain in the RX FIFO. Won't a subsequent transaction
inadvertently read this stale data instead of its own response, potentially
returning corrupted values to client drivers?

> -	u32 rsp;
> +	u32 reply, rsp;
>  	size_t len_read = 0;
>  	u8 i;
>  	int ret;

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but similar to spmi_read_cmd(), are we
missing a lock around the hardware register accesses in spmi_write_cmd()?

Write operations also access the shared hardware controller without
synchronization when writing to SPMI_CMD_REG, exposing it to concurrent
overwrites.

[Severity: High]
This is a pre-existing issue, but does spmi_write_cmd() also need to
ensure the RX FIFO is drained of stale data before writing a new command?

Write commands similarly fail to flush the RX FIFO prior to execution,
making them vulnerable to stream desynchronization if earlier commands
timed out.

> +	u32 reply;
>  	size_t i = 0, j;
>  	int ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-t603x-spmi-v2-0-f43e5f10e583@chaosmail.tech?part=3