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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Endianness bug in `spmi_raw_cmd` due to improper mixing of `memcpy()` and `writel()`/`readl()` for FIFO stream accesses.
--

commit cb6fc258a7a80697550d0e3d62f761cda4b89087
Author: Alba Mendez <me@alba.sh>

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 b1c127cf5f441..9843dc871d6c7 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -86,6 +85,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_wr) {
> +		j = min_t(size_t, sizeof(spmi_cmd), len_wr - i);
> +		spmi_cmd = 0;
> +		memcpy(&spmi_cmd, buf_wr + i, j);
> +		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: Medium]
Will using memcpy() to write into a u32 followed by writel() cause an
endianness regression on big-endian systems?

On big-endian systems, memcpy() places the first byte at the most
significant byte of the u32. writel() then applies a byte-swap, placing
that first byte at the highest bus address (byte 3) instead of byte 0,
which would corrupt the data stream going into the FIFO.

The pre-patch code avoided this by using bitwise shifts to pack the u32
in a CPU-endian-independent manner. Mixing memcpy() and writel() violates
subsystem rules for FIFO I/O accessors.

> +		i += j;
> +	}
> +
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);
>  	if (ret)
>  		return ret;
[ ... ]
> @@ -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;
> -		}
> +		j = min_t(size_t, sizeof(spmi_cmd), len_rd - i);
> +		memcpy(buf_rd + i, &rsp, j);

[Severity: Medium]
Can this readl() followed by memcpy() cause the inverse data corruption on
the read path?

Like the write path, readl() performs a byte swap on big-endian systems,
meaning the memcpy() will extract the stream bytes in reverse order,
corrupting the responses received from the FIFO.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-t603x-spmi-v9-0-09e702e3b153@chaosmail.tech?part=4