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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] spmi: massive undocumented behavioral changes
- [Low] spmi: memcpy reverses payload byte order on BE hosts
--

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

spmi: apple: lock around FIFOs

This commit adds a mutex to the apple_spmi structure to serialize register
interactions. The new fifo_lock is initialized during probe and guards the
FIFO operations within spmi_raw_cmd().

> spmi: apple: lock around FIFOs
>
> The driver was missing locking around register interactions.

[Severity: Medium]
Does this patch bundle undocumented behavioral regressions?

The commit message claims to only add locking around register interactions,
but it appears to silently bundle major refactorings and logic changes.

For example, does it introduce new hardware ACK checking logic in
spmi_raw_cmd()?

drivers/spmi/spmi-apple-controller.c:spmi_raw_cmd() {
    ...
    if (!len_rd && !FIELD_GET(SPMI_REPLY_ACK, reply)) {
        ...
    }
}

Does it also alter how standard write commands are packed?

drivers/spmi/spmi-apple-controller.c:spmi_write_cmd() {
    ...
    return spmi_raw_cmd_ack(ctrl, opc | saddr, sid,
                            buf[0] << 8 | saddr, NULL, 0);
}

And does it implement a new callback for sleep/wakeup/shutdown commands?

drivers/spmi/spmi-apple-controller.c:
static int spmi_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
{
    ...
}

Could the commit message be updated to describe these changes, or should they
be split into separate commits?

> 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: Low]
Does this code introduce a regression by reversing the payload byte order on
big-endian hosts?

In spmi_raw_cmd(), memcpy() is used to copy byte arrays into a host-endian
u32 variable, which is then written to the bus using writel():

drivers/spmi/spmi-apple-controller.c:spmi_raw_cmd() {
    ...
        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;
        }
    ...
}

On big-endian hosts, writel() will byte-swap this, resulting in the bytes
being written in reverse order compared to the original bitwise logic.

A similar reversal happens when reading the received payload:

drivers/spmi/spmi-apple-controller.c:spmi_raw_cmd() {
    ...
        while (i < len_rd) {
                ...
                rsp = readl(spmi->regs + SPMI_RSP_REG);
                j = min_t(size_t, sizeof(spmi_cmd), len_rd - i);
                memcpy(buf_rd + i, &rsp, j);
                i += j;
        }
    ...
}

While Apple SoCs are little-endian and this may have no visible effect in
practice, could this unconditional byte swapping manifest as a regression on
big-endian architectures under COMPILE_TEST?

[ ... ]

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