Re: [PATCH v7 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:
- [High] Replacing bitwise shifts with `memcpy()` into/from a `u32` variable combined with `writel()` and `readl()` for FIFO transfers introduces byte-swapping data corruption on big-endian architectures.
--

--- Patch [4]: [PATCH v7 4/6] spmi: apple: Implement remaining commands ---
commit 0955662fb587593fe5c8969de75412be27a01533
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);
> +		i += j;
> +	}
> +

[Severity: High]
Will this combination of memcpy() and writel() cause data corruption on
big-endian architectures?

On big-endian systems, memcpy() will store the first byte of the stream into
the lowest memory address of the u32, making it the most-significant byte.
When writel() is called, it will apply cpu_to_le32(), swapping the bytes and
sending them to the hardware in reverse order. Does this loop need to use
writesl() or explicit bitwise shifts instead?

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

[Severity: High]
Can this sequence of readl() and memcpy() read zero-padding instead of valid
data for partial reads on big-endian systems?

When readl() byte-swaps the incoming hardware register value on big-endian
CPUs, the lowest bytes end up at the highest memory addresses. Since memcpy()
copies from the lowest memory address first, a partial read will copy the
zero-padded upper bytes instead of the actual data. Should this use readsl()
or the previous bitwise extraction logic to preserve portability?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-t603x-spmi-v7-0-dafebe6e7739@chaosmail.tech?part=4