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

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

Pre-existing issues:
- [High] Missing synchronization in SPMI controller read/write callbacks allows concurrent bus transactions to corrupt controller registers and shared state.
- [High] The error recovery logic writes the FIFO flush command to the read-only response register instead of the action register.
- [High] Multiple signed integer overflows due to unsafe bitwise shifts result in undefined behavior.
--

--- Patch [3]: [PATCH v4 3/6] spmi: apple: check transaction status ---
commit 879bd117b98da2cbe05de1381fdc2003dc4e22ae
Author: Alba Mendez <[email protected]>

spmi: apple: check transaction status

This patch updates the Apple SPMI controller driver to check for parity errors
and missing command acknowledgments during transactions. By verifying the parity
status and ACK bits in the reply frame, the driver can now detect and report
transaction failures instead of silently ignoring them.

Signed-off-by: Alba Mendez <[email protected]>
Signed-off-by: Sasha Finkelstein <[email protected]>

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 4d5356bf9bbf1..7ebf899edf173 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);

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a lock here
allow concurrent bus transactions to corrupt controller registers and shared
state?

The SPMI framework does not serialize read_cmd and write_cmd controller
callbacks. Multiple PMIC device drivers on the same SPMI bus could initiate
transactions concurrently.

Without a spinlock or mutex, wouldn't concurrent accesses interleave writes
to SPMI_CMD_REG and cause race conditions when handling the shared
spmi->prev_fail state?

>  	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);

[Severity: High]
This is a pre-existing issue, but do multiple bitwise shifts in this driver
invoke undefined behavior via signed integer overflow?

In apple_spmi_pack_cmd(), when packing the command:

return opc | sid << 8 | saddr << 16 | (len - 1) | (1 << 15);

If saddr is >= 0x8000, it gets promoted to a signed int and shifted by 16,
which overflows into the sign bit.

Similarly, in spmi_write_cmd() where buf[i++] is shifted by 24 (when >= 128),
and in spmi_read_cmd() where 0xff is shifted by 24, these shift signed ints
into the sign bit.

Could this allow the compiler to miscompile the addressing and data unpacking
logic?

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

[ ... ]

> @@ -82,12 +90,12 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (ret)
>  		return ret;
>  
> -	/* Discard SPMI reply status */
> -	readl(spmi->regs + SPMI_RSP_REG);
> +	reply = readl(spmi->regs + SPMI_RSP_REG);
>  
>  	/* Read SPMI data reply */
>  	while (len_read < len) {
>  		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			spmi->prev_fail = true;

[Severity: High]
This isn't a bug introduced by this patch, but since we are actively setting
spmi->prev_fail to true here, does the error recovery logic write the FIFO
flush command to the wrong register?

When recovering from a previous failure, the driver does:

writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);

SPMI_RSP_REG is a read-only response register. Shouldn't this flush command
be written to the action register (SPMI_ACT_REG) instead? Otherwise, the
FIFO might never be flushed, leading to a permanently stuck controller state.

>  			dev_err_ratelimited(&ctrl->dev,
>  					    "FIFO lacks reply data, controller stuck?\n");
>  			return -EIO;

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