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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using `memcpy()` to pack/unpack byte streams into a 32-bit integer variable combined with `writel()`/`readl()` causes byte-swapping data corruption on big-endian architectures.
--

--- Patch [4]: [PATCH v5 4/6] spmi: apple: Implement remaining commands ---
commit 133f8b2358af9160fedbf5fcf84ef152c33022e4
Author: Alba Mendez <[email protected]>
    
spmi: apple: Implement remaining commands
    
This patch adds support for zero write and power management commands to the
Apple SPMI controller driver. It achieves this by introducing a unified
spmi_raw_cmd function for executing SPMI transactions.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> -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_wr, size_t len_wr, u8 *buf_rd, size_t len_rd)
>  {
[ ... ]
> +	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);
> +		i += j;
> +	}

[Severity: High]
Will this sequence cause byte-swapping data corruption on big-endian
architectures?

When len_wr is greater than zero, memcpy copies the byte stream into the
u32 spmi_cmd. On big-endian systems, memcpy places the first byte in the
most-significant byte position.

writel then performs CPU-to-LE conversion (byte-swapping), which causes
the hardware to receive the bytes in reverse order.

[ ... ]
>  		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);
> +		i += j;
>  	}

[Severity: High]
Does the read path in spmi_raw_cmd suffer from the exact inverse problem?

readl will byte-swap the incoming data on big-endian hosts, and memcpy
will then save it into buf_rd in reverse order.

While Apple SoCs are primarily little-endian, Linux drivers are expected to
be endian-portable. Would it be better to use stream accessors like writesl
and readsl, or manually pack the bytes using shifts (as the original read
code did) to ensure consistent behavior across architectures?

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