Re: [PATCH v7 2/6] spmi: apple: Validate FIFO state
From: sashiko-bot@kernel.org
Date: 2026-08-18 18:44:50
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [Medium] Instantaneous check of RX_FIFO_EMPTY randomly aborts valid multi-word reads if the hardware streams data. Pre-existing issues: - [High] Missing controller-level lock allows concurrent operations to corrupt SPMI bus transactions and race on prev_fail. --
--- Patch [2]: [PATCH v7 2/6] spmi: apple: Validate FIFO state ---commit b638b8c461a5028b5eb344197adecba7ccd922ba Author: Alba Mendez [off-list ref] spmi: apple: Validate FIFO state This commit adds checks to validate the FIFO state before reading reply data and after a command completes. It introduces a prev_fail flag and flushes the FIFO before executing new commands if previous operations failed.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -67,6 +71,11 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, u8 i; int ret; + if (spmi->prev_fail) {
[Severity: High] This isn't a bug introduced by this patch, but does this missing controller-level lock allow concurrent operations to corrupt SPMI bus transactions? Since the SPMI subsystem exposes this controller to all its children and does not natively serialize read_cmd or write_cmd operations, multiple threads could invoke spmi_read_cmd() simultaneously. This would lead to concurrent modifications of spmi->prev_fail and shared hardware registers without mutual exclusion.
+ writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG); + spmi->prev_fail = false; + } + writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -78,6 +87,12 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, /* Read SPMI data reply */ while (len_read < len) { + if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) { + spmi->prev_fail = true; + dev_err_ratelimited(&ctrl->dev, + "FIFO lacks reply data, controller stuck?\n"); + return -EIO; + }
[Severity: Medium] Could this instantaneous check of SPMI_RX_FIFO_EMPTY spuriously abort valid multi-word reads? In a multi-word read transaction, the CPU might outrun the SPMI bus after reading the first word. If the hardware streams data and hasn't buffered the next word yet, the CPU will immediately see SPMI_RX_FIFO_EMPTY and return -EIO instead of waiting for the next word to arrive.
rsp = readl(spmi->regs + SPMI_RSP_REG); i = 0;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -97,6 +117,11 @@ static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid, size_t i = 0, j; int ret; + if (spmi->prev_fail) {
[Severity: High] This is a pre-existing issue, but can concurrent calls to spmi_write_cmd() race on spmi->prev_fail and hardware state? Similar to spmi_read_cmd(), this modifies shared hardware state and spmi->prev_fail without serialization.
+ writel(SPMI_ACT_FIFO_FLUSH, spmi->regs + SPMI_ACT_REG); + spmi->prev_fail = false; + } + writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260818-t603x-spmi-v7-0-dafebe6e7739@chaosmail.tech?part=2