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

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

New issues:
- [High] The FIFO flush command is written to the wrong register (`SPMI_RSP_REG` instead of `SPMI_ACT_REG`).
- [High] Missing `spmi->prev_fail = true` assignment when aborting due to an empty RX FIFO.

Pre-existing issues:
- [High] Bus transactions are not serialized, leading to potential race conditions on the FIFO and controller registers.
--

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

spmi: apple: Validate FIFO state

Check for data before reading the body of a reply, and check for
end of data afterwards.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 376cf682c43e0..4d5356bf9bbf1 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,

[Severity: High]
This is a pre-existing issue, but since spmi_read_cmd() lacks a mutex or 
spinlock, what prevents concurrent calls from different client drivers 
(like regulators or RTC) from interleaving?

Without serialization, concurrent accesses to the controller registers could 
corrupt the state or wedge the controller. This pre-existing issue also 
affects the newly added prev_fail flag, which could be overwritten or 
misread during interleaved bus operations.

>  	u8 i;
>  	int ret;
>  
> +	if (spmi->prev_fail) {
> +		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);

[Severity: High]
Is it intended to write SPMI_ACT_FIFO_FLUSH to SPMI_RSP_REG? 

It looks like this might have meant to write to the newly defined 
SPMI_ACT_REG instead. Writing an action command to the response register 
could leave the FIFO unflushed and leave the controller permanently wedged 
after an error.

> +		spmi->prev_fail = false;
> +	}
> +
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);
[ ... ]
> @@ -78,6 +87,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	/* Read SPMI data reply */
>  	while (len_read < len) {
> +		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			dev_err_ratelimited(&ctrl->dev,
> +					    "FIFO lacks reply data, controller stuck?\n");
> +			return -EIO;

[Severity: High]
When the loop aborts here due to an empty RX FIFO, shouldn't spmi->prev_fail 
be set to true before returning?

If the hardware response is slow and causes an early exit without setting 
this flag, the next transaction will not flush the delayed reply. Could this 
cause the subsequent transaction to read stale reply data as its own, leading 
to silent data corruption for downstream PMIC clients?

> +		}
>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
>  		i = 0;
>  		while ((len_read < len) && (i < 4)) {
[ ... ]
> @@ -97,6 +116,11 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,

[Severity: High]
This is a pre-existing issue, but similar to spmi_read_cmd(), the 
spmi_write_cmd() function also lacks synchronization, leaving it vulnerable to 
concurrent accesses from different SPMI bus clients.

>  	size_t i = 0, j;
>  	int ret;
>  
> +	if (spmi->prev_fail) {
> +		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);

[Severity: High]
Does this also need to be written to SPMI_ACT_REG instead of SPMI_RSP_REG?

> +		spmi->prev_fail = false;
> +	}
> +

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