Re: [PATCH v2 4/7] spmi: apple: Implement remaining commands

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing bounds validation allows unmasked data in SPMI_CMD_ZERO_WRITE to corrupt the command opcode.
- [Medium] Missing NACK check on read commands triggers a false 'controller stuck' error and incorrect error handling.

Pre-existing issues:
- [High] Missing locking around shared hardware FIFO registers causes race conditions and bus state corruption.
- [High] Unhandled residual data in the RX FIFO leads to permanent transaction desynchronization.
--

--- Patch [4]: [PATCH v2 4/7] spmi: apple: Implement remaining commands ---
commit aad16a3367909482f17e1e9e1da0123341931c90
Author: Alba Mendez <me@alba.sh>
spmi: apple: Implement remaining commands

This commit adds support for zero write and power management commands to the
Apple SPMI driver. It restructures the internal command functions to handle
raw inputs and acknowledgements.

> 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

[ ... ]

> -static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> -			 u16 saddr, u8 *buf, size_t len)
> +static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +			 u16 param, const u8 *buf, size_t len, u8 *ibuf, size_t ilen)
>  {
>  	struct apple_spmi *spmi = spmi_controller_get_drvdata(ctrl);
> -	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, saddr, len);
> +	u32 spmi_cmd = apple_spmi_pack_cmd(opc, sid, param);
>  	u32 reply, rsp;
>  	size_t len_read = 0;
> -	u8 i;
> +	size_t i = 0, j;
>  	int ret;
>  
>  	writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);

[Severity: High]
This is a pre-existing issue, but does spmi_raw_cmd() need locking to serialize
access to the shared hardware FIFO registers? Since the SPMI framework does
not serialize transactions, if two threads perform concurrent commands, could
their reads and writes to SPMI_CMD_REG and SPMI_RSP_REG interleave, resulting
in hardware FIFO corruption or reading another transaction's response?

> +
> +	while (i < len) {
> +		j = 0;
> +		spmi_cmd = 0;
> +		while ((j < 4) & (i < len))
> +			spmi_cmd |= buf[i++] << (j++ * 8);
> +
> +		writel(spmi_cmd, spmi->regs + SPMI_CMD_REG);
> +	}
> +
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);
>  	if (ret)
>  		return ret;
> @@ -82,15 +91,15 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	reply = readl(spmi->regs + SPMI_RSP_REG);
>  
>  	/* Read SPMI data reply */
> -	while (len_read < len) {
> +	while (len_read < ilen) {
>  		if (readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY) {
>  			dev_err(&ctrl->dev, "FIFO lacks reply data, controller stuck?\n");
>  			return -EIO;
>  		}

[Severity: Medium]
Will this error path incorrectly trigger for unacknowledged read commands?
Because the NACK check below is restricted to commands with no input payload
(!ilen), if a read command is NACKed (e.g., when probing an empty bus address),
it bypasses the NACK check and enters this loop. Could this result in hitting
the empty FIFO check and raising a misleading "controller stuck?" error instead
of gracefully handling the normal NACK condition?

>  		rsp = readl(spmi->regs + SPMI_RSP_REG);
>  		i = 0;
> -		while ((len_read < len) && (i < 4)) {
> -			buf[len_read++] = ((0xff << (8 * i)) & rsp) >> (8 * i);
> +		while ((len_read < ilen) && (i < 4)) {
> +			ibuf[len_read++] = ((0xffU << (8 * i)) & rsp) >> (8 * i);
>  			i += 1;
>  		}
>  	}
> @@ -98,47 +107,67 @@ static int spmi_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	if (!(readl(spmi->regs + SPMI_STATUS_REG) & SPMI_RX_FIFO_EMPTY))
>  		dev_warn(&ctrl->dev, "FIFO has extra data\n");

[Severity: High]
This is a pre-existing issue, but does the RX FIFO need to be drained when
residual data is detected? If the PMIC pushes more frames than expected or the
transaction returns early, leaving this extra data could cause subsequent
transactions to read the stale residual words, permanently desynchronizing the
bus.

> -	if ((~reply >> SPMI_REPLY_FRAME_PARITY_OFFSET) & ((1 << len) - 1)) {
> +	if (!ilen && !(reply & SPMI_REPLY_ACK)) {
> +		dev_err(&ctrl->dev, "command not acknowledged\n");
> +		return -EIO;
> +	}

[ ... ]

> +static int spmi_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
> +			  u16 saddr, const u8 *buf, size_t len)
> +{
> +	switch (opc) {
> +	case SPMI_CMD_WRITE:
> +		return spmi_raw_cmd_ack(ctrl, opc | saddr, sid, buf[0] << 8 | saddr, NULL, 0);
> +	case SPMI_CMD_ZERO_WRITE:
> +		return spmi_raw_cmd_ack(ctrl, opc | buf[0], sid, buf[0] << 8 | saddr, NULL, 0);

[Severity: High]
Does this code allow unmasked user data to corrupt the command opcode? If a
caller provides a data payload for SPMI_CMD_ZERO_WRITE where bits 4-7 are set,
bitwise ORing it directly with the opcode (opc | buf[0]) without masking
(e.g., buf[0] & 0x0F) could overwrite the upper opcode bits, potentially
executing unintended commands on the PMIC.

> +	case SPMI_CMD_EXT_WRITE:
> +	case SPMI_CMD_EXT_WRITEL:
> +		return spmi_raw_cmd_ack(ctrl, opc | (len - 1), sid, saddr, buf, len);
>  	}
> -	return 0;
> +	return -EINVAL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-t603x-spmi-v2-0-f43e5f10e583@chaosmail.tech?part=4