Re: [PATCH v10 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] spmi: apple: endianness regression via memcpy of IO registers
--

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

spmi: apple: Implement remaining commands

This commit adds support for zero write and power management commands to
the Apple SPMI controller driver.

> 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]
Does this cause an endianness regression on Big Endian architectures?

Using memcpy() to copy a u8 array into a u32 variable before passing it
to writel() can reverse the byte order. On Big Endian hosts, the first
byte of the array is placed in the most significant byte of the u32 in
memory, which writel() will then implicitly swap to Little Endian for
the bus, effectively reversing the byte stream.

While Apple SoCs currently operate in Little Endian, does this break
driver endianness-cleanliness and architectural portability?

> +		i += j;
> +	}
> +
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);

[ ... ]

> @@ -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]
Similarly, does destructuring the readl() return value via memcpy() misplace
the bytes on Big Endian hosts?

The original code used bitwise operations to safely pack and unpack the
u32 words byte-by-byte, which inherently abstracts away host endianness.

> +		i += j;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-t603x-spmi-v10-0-ff815c66c73f@chaosmail.tech?part=4