Re: [PATCH v3 3/6] spmi: apple: check transaction status

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

New issues:
- [Low] Missing explicit header includes for bitfield and bitwise macros.

Pre-existing issues:
- [High] Leftover FIFO data on error paths permanently desynchronizes subsequent transactions.
- [High] Unbounded `while` loop polling hardware status can cause a kernel hard lockup.
--

--- Patch [3]: [PATCH v3 3/6] spmi: apple: check transaction status ---
commit c52cd20ea1913f6b44118c7681235dcecf7cc8dc
Author: Alba Mendez <[email protected]>

spmi: apple: check transaction status

This commit adds checks for parity errors and missing command ACKs in the
Apple SPMI controller driver. It extracts the reply status from the response
register instead of discarding it and returns an error if checks fail.

> 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
> @@ -22,6 +22,12 @@
>  #define SPMI_CMD_REG 0x4
>  #define SPMI_RSP_REG 0x8
>  
> +/* SPMI_RSP_REG reply word */
> +#define SPMI_REPLY_FRAME_PARITY_STATUS GENMASK(31, 16)
> +#define SPMI_REPLY_ACK BIT(15)
> +#define SPMI_REPLY_SLAVE_ID GENMASK(14, 8)
> +#define SPMI_REPLY_CMD GENMASK(7, 0)
> +

[Severity: Low]
Are we missing explicit header includes for linux/bitfield.h and
linux/bits.h?

This patch introduces the use of GENMASK and FIELD_GET, but appears to rely
on transitive inclusions from other headers, which might cause future build
breakages if those headers change.

[ ... ]

> @@ -87,8 +93,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (ret)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does this error path leave stale data in
the RX FIFO?

If apple_spmi_wait_rx_not_empty() times out, it returns an error and aborts
immediately without draining the RX FIFO:

drivers/spmi/spmi-apple-controller.c:apple_spmi_wait_rx_not_empty() {
    ...
    ret = poll_reg(spmi, SPMI_STATUS_REG, status, !(status & SPMI_RX_FIFO_EMPTY));
    if (ret) {
        dev_err(&ctrl->dev, "failed to wait for RX FIFO not empty\n");
        return ret;
    }
}

The hardware might subsequently complete the transaction and push the reply
into the FIFO. Later commands will begin by writing to the command register
without first asserting that the RX FIFO is drained:

drivers/spmi/spmi-apple-controller.c:spmi_raw_cmd() {
    guard(mutex)(&spmi->fifo_lock);
    reinit_completion(&spmi->fifo_rx);
    writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
    ...
}

Could this permanently desynchronize subsequent transactions by causing them
to read the stale reply data left by the aborted transaction?

> -	/* Discard SPMI reply status */
> -	readl(spmi->regs + SPMI_RSP_REG);
> +	reply = readl(spmi->regs + SPMI_RSP_REG);

[ ... ]

> @@ -107,6 +112,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	flush_extra_data(ctrl);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for this call
to cause a kernel hard lockup?

The flush_extra_data() function implements an unbounded while loop polling
the hardware status:

drivers/spmi/spmi-apple-controller.c:flush_extra_data() {
    while (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY)) {
        ...
        readl(spmi->regs + SPMI_RSP_REG);
    }
}

If the hardware enters a faulty state or continuously reports that the FIFO
is not empty, could the CPU loop infinitely here without yielding or timing
out?

> +	if (~FIELD_GET(SPMI_REPLY_FRAME_PARITY_STATUS, reply) & ((1 << len) - 1)) {
> +		dev_err(&ctrl->dev, "some frames failed parity check\n");
> +		return -EIO;
> +	}
>  	return 0;
>  }

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