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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Usage of `memcpy()` with `readl()`/`writel()` for FIFO data transfer introduces an endianness bug on big-endian architectures.
--

--- Patch [4]: [PATCH v6 4/6] spmi: apple: Implement remaining commands ---
commit 44a0f7f784234d8f3fc75ad0bec2d3f544e9850c
Author: Alba Mendez <[email protected]>

spmi: apple: Implement remaining commands

This commit adds support for zero write and power management commands to
the Apple SPMI controller driver. It also consolidates the read and write
paths into a single spmi_raw_cmd() function.

> 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]
Does the use of memcpy() with writel() corrupt the outgoing payload byte
order on big-endian CPUs?

The previous code used bitwise shifts which are endian-agnostic. By switching
to memcpy(), the bytes from buf_wr are written sequentially into the host
memory layout of spmi_cmd.

On big-endian systems, the first byte of buf_wr will be placed in the most
significant byte of spmi_cmd. When writel() applies its endian conversion
via cpu_to_le32(), does this result in the device receiving the bytes in
reverse order?

[ ... ]
> @@ -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]
Similarly, does the use of memcpy() with a u32 value returned from readl()
corrupt the incoming payload byte order on big-endian CPUs?

readl() applies le32_to_cpu() to the value read from the register. On big-endian
systems, the least significant byte of the original little-endian register will
end up at the highest memory address of the local rsp variable.

Does memcpy() then copy these bytes into buf_rd in the reverse of their intended
order?

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