Re: [PATCH v2 2/7] spmi: apple: Validate FIFO state

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver detects and warns about extra data in the RX FIFO but fails to drain it, permanently desynchronizing subsequent SPMI transactions.
- [High] The driver silently discards the SPMI reply status word, ignoring command NACKs and parity errors.
- [High] The SPMI read and write commands lack mutual exclusion, leading to concurrent hardware accesses clobbering each other.
- [High] The driver polls hardware registers using a sleeping timeout macro (`readl_poll_timeout`), triggering kernel BUGs in atomic contexts.
--

--- Patch [2]: [PATCH v2 2/7] spmi: apple: Validate FIFO state ---
commit 6fe31783af0978f8c2699194bf6efebf6615ed3b
Author: Alba Mendez <me@alba.sh>

spmi: apple: Validate FIFO state

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

> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -78,6 +78,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,

[Severity: High]
This is a pre-existing issue, but does spmi_read_cmd() lack mutual exclusion?

Concurrent calls to the SPMI controller could interleave writes to the command
register and reads from the FIFO.

Could this unpredictably corrupt commands and steal words from each other's
streams during concurrent access by different subsystems?

[Severity: High]
This isn't a bug introduced by this patch, but the wait function called
before this read loop appears to sleep in atomic contexts.

Every SPMI read/write operation calls apple_spmi_wait_rx_not_empty(), which 
uses poll_reg() wrapped around readl_poll_timeout().

Since readl_poll_timeout() delegates to usleep_range(), wouldn't this trigger
a kernel BUG when SPMI client drivers issue commands from atomic contexts?

>  
>  	/* Read SPMI data reply */
>  	while (len_read < len) {

[Severity: High]
This is a pre-existing issue, but earlier in this function the status word is
discarded before the read loop:

drivers/spmi/spmi-apple-controller.c:spmi_read_cmd() {
	/* Discard SPMI reply status */
	readl(spmi->regs + SPMI_RSP_REG);
	...
}

Does this silently ignore command NACKs and parity errors on the wire?

> +		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
> +			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
> +			return -EIO;
> +		}
>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
>  		i = 0;

[ ... ]

> @@ -86,6 +90,9 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  		}
>  	}
>  
> +	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY))
> +		dev_warn(&ctrl->dev, "FIFO has extra data\n");

[Severity: High]
This isn't a bug introduced by this patch, but if the FIFO is left with extra
data here, will the next SPMI transaction's wait function return immediately?

If the driver pops the stale leftover data instead of waiting for its own reply,
it seems like this would permanently shift the data stream and break all
subsequent communication with the PMIC.

Should the extra data be drained here to prevent this?

> +
>  	return 0;
>  }
>  
> @@ -115,6 +122,9 @@ 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(), does
spmi_write_cmd() also lack mutual exclusion for stateful bus transactions?

>  	/* Discard */
>  	readl(spmi->regs + SPMI_RSP_REG);

[Severity: High]
This isn't a bug introduced by this patch, but discarding the status word
here appears to hide write failures and parity errors.

> +	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY))
> +		dev_warn(&ctrl->dev, "FIFO has extra data\n");

[Severity: High]
This is a pre-existing issue, but just like in spmi_read_cmd(), failing to
drain the extra data detected here could permanently desynchronize subsequent
SPMI transactions.

> +
>  	return 0;
>  }

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