Re: [PATCH v4 4/6] spmi: apple: Implement remaining commands

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

New issues:
- [High] Endianness portability bug causing data corruption on big-endian architectures due to `memcpy` and `writel()` interaction.

Pre-existing issues:
- [High] Missing lock around SPMI bus transactions allows concurrent operations to interleave, corrupting hardware state.
--

--- Patch [4]: [PATCH v4 4/6] spmi: apple: Implement remaining commands ---
commit 3757bcbae2fce4b6f6a6ec4bc0492ce9325dcd8c
Author: Alba Mendez <[email protected]>

spmi: apple: Implement remaining commands

Add support for zero write and power management commands

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 7ebf899edf173..49827e51e93e9 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -69,14 +69,14 @@ static int apple_spmi_wait_rx_not_empty(struct spmi_controller *ctrl)
>  	return 0;
>  }
>  
> -static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> -			 u16 saddr, u8 *buf, size_t len)
> +static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +			 u16 param, const u8 *buf, size_t len, u8 *ibuf, size_t ilen)

[Severity: High]
This is a pre-existing issue, but is there a missing lock around the SPMI
bus transactions here?

The SPMI subsystem core does not serialize calls to controller read/write
functions. Without a mutex protecting this transaction sequence, concurrent
requests can interleave their writes to the shared command register,
poll operations, and reads from the response FIFO.

Additionally, the unprotected shared flag spmi->prev_fail introduces a race
condition where one thread's failure can cause another thread to
unexpectedly flush the controller's FIFO mid-transaction.

>  {
>  	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> -	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
> +	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, param);
>  	u32 reply, rsp;
>  	size_t len_read = 0;
> -	u8 i;
> +	size_t i = 0, j;
>  	int ret;
>  
>  	if (spmi->prev_fail) {

[ ... ]

> @@ -86,6 +86,14 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
>  
> +	while (i < len) {
> +		j = min_t(size_t, sizeof(spmi_cmd), len - i);
> +		spmi_cmd = 0;
> +		memcpy(&spmi_cmd, buf + i, j);
> +		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: High]
Will this byte stream copy cause an endianness bug on big-endian
architectures?

Using memcpy to copy the byte stream into a local u32 spmi_cmd
variable and then calling writel() will result in byteswapping on
big-endian systems.

The memcpy call places the first byte of buf into the most significant
byte of the u32, and writel() performs a CPU-to-LE conversion.

This reverses the byte order sent to the hardware compared to the previous
bitwise-shift implementation, which was correctly endian-safe. Should this
use writesl() or continue using the previous shift-based packing?

> +		i += j;
> +	}
> +
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);
>  	if (ret)
>  		return ret;
> @@ -93,7 +101,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	reply = readl(spmi->regs + SPMI_RSP_REG);
>  
>  	/* Read SPMI data reply */
> -	while (len_read < len) {
> +	while (len_read < ilen) {
>  		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
>  			spmi->prev_fail = true;
>  			dev_err_ratelimited(&ctrl->dev,
> @@ -101,11 +109,9 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  			return -EIO;
>  		}
>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
> -		i = 0;
> -		while ((len_read < len) && (i < 4)) {
> -			buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);
> -			i += 1;
> -		}
> +		i = min_t(size_t, sizeof(spmi_cmd), ilen - len_read);
> +		memcpy(ibuf + len_read, &rsp, i);

[Severity: High]
Does unpacking the readl() result via memcpy introduce the same
endianness reversal on big-endian systems?

Since rsp is read via readl(), copying it directly to the buffer with
memcpy will reverse the byte order.

> +		len_read += i;
>  	}
>  
>  	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {

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