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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Copying raw bytes to/from u32 variables for writel/readl causes byte swapping and data corruption on big-endian architectures.
- [High] The FIFO flush command is written to the response register (SPMI_RSP_REG) instead of the action register (SPMI_ACT_REG).
--

--- Patch [5]: [PATCH v4 5/6] spmi: apple: lock around FIFOs ---
commit 69b70a7086a8bbc17681a468c0f31368df078c1d
Author: Alba Mendez <[email protected]>

spmi: apple: lock around FIFOs

The driver was missing locking around register interactions.

> 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

[ ... ]

> @@ -79,6 +81,8 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	size_t i = 0, j;
>  	int ret;
>  
> +	guard(mutex)(&spmi->fifo_lock);
> +
>  	if (spmi->prev_fail) {
>  		writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);

[Severity: High]
Does this flush command write to the correct register?

Looking at the error recovery path in spmi_raw_cmd(), the FIFO flush is
written to the response register:

    writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_RSP_REG);

Should this be written to SPMI_ACT_REG instead? Writing the flush command
to the response register might fail to flush the hardware FIFO and could
corrupt the response register state.

[Severity: High]
Can this I/O access pattern corrupt data on big-endian architectures?

Further down in spmi_raw_cmd(), the command and response byte streams are
accessed using writel() and readl():

    memcpy(&spmi_cmd, buf + i, j);
    writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

and during the read phase:

    rsp = readl(spmi->regs + SPMI_RSP_REG);
    i = min_t(size_t, sizeof(spmi_cmd), ilen - len_read);
    memcpy(ibuf + len_read, &rsp, i);

Copying a byte stream into a local u32 variable and then using writel() or
readl() performs CPU-to-device endianness conversion, which will swap the
bytes on big-endian systems.

Should these accesses use stream-based accessors like writesl() and readsl()
instead to preserve the expected byte order?

>  		spmi->prev_fail = false;
>  	}

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