Re: [PATCH v5 5/6] spmi: apple: lock around FIFOs

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver uses register I/O accessors (`writel()` and `readl()`) with `memcpy()` to stream payload bytes to/from the SPMI FIFOs instead of using stream accessors (`writesl()` and `readsl()`), which causes endianness portability bugs on big-endian architectures.
--

--- Patch [5]: [PATCH v5 5/6] spmi: apple: lock around FIFOs ---
commit 79f5d9aebdc66fbf9df1c1b4f401429d674f9b42
Author: Alba Mendez <[email protected]>

spmi: apple: lock around FIFOs

This commit adds a mutex to protect FIFO register interactions in the Apple
SPMI controller driver, preventing concurrent access issues.

> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 9843dc871d6c7..fabccd25aa0da 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c

[ ... ]

> @@ -78,6 +80,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u16 param,
>  	size_t i = 0, j;
>  	int ret;
>  
> +	guard(mutex)(&spmi->fifo_lock);
> +

[Severity: High]
This is a pre-existing issue, but does using writel() and readl() to transfer
byte buffers to the FIFO cause endianness portability bugs on big-endian
architectures?

Further down in spmi_raw_cmd(), the driver streams payload bytes to the
hardware FIFO using writel() with memcpy():

	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;
	}

And similarly for reads:

		rsp = readl(spmi->regs + SPMI_RSP_REG);
		j = min_t(size_t, sizeof(spmi_cmd), len_rd - i);
		memcpy(buf_rd + i, &rsp, j);

Using writel() and readl() instead of stream accessors like writesl() and
readsl() performs an implicit CPU-to-LE conversion.

Could this cause the data to be unexpectedly byte-swapped on big-endian
systems, corrupting the payload sent to the device on the bus?

>  	if (spmi->prev_fail) {
>  		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG);
>  		spmi->prev_fail = false;

[ ... ]

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