Re: [PATCH v2 5/7] spmi: apple: lock around FIFOs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver does not drain or flush the RX FIFO during initialization or after error conditions, leading to cascading transaction failures and state desynchronization.
--

--- Patch [5]: [PATCH v2 5/7] spmi: apple: lock around FIFOs ---
commit 0e8d4d322dfd8a19a861bdcc1b9aefd06ad5b239
Author: Alba Mendez <me@alba.sh>

spmi: apple: lock around FIFOs

The driver adds a mutex to provide locking around register interactions.

> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
[ ... ]
> @@ -86,7 +90,7 @@ static int spmi_raw_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  
>  	ret = apple_spmi_wait_rx_not_empty(ctrl);
>  	if (ret)
> -		return ret;
> +		goto out;

[Severity: High]
This is a pre-existing issue, but if a transaction times out here and the
hardware eventually replies, does the unread data remain in the hardware
RX FIFO? If the FIFO isn't explicitly drained on errors, the next
transaction could incorrectly consume the stale data as its own response,
leading to persistent state desynchronization.

>  
>  	reply = readl(spmi->regs + SPMI_RSP_REG);
>  
[ ... ]
> @@ -107,6 +112,8 @@ static int spmi_raw_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");
>  
> +	mutex_unlock(&spmi->fifo_lock);
> +

[Severity: High]
This is also a pre-existing issue, but when extra data is detected here,
should the driver drain the remaining words from the FIFO? Leaving the
unexpected data inside the FIFO instead of reading it out could corrupt
the state synchronization of the next command on the bus.

[ ... ]
> @@ -181,6 +192,7 @@ static int apple_spmi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	spmi = spmi_controller_get_drvdata(ctrl);
> +	mutex_init(&spmi->fifo_lock);
>  
>  	spmi->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(spmi->regs))

[Severity: High]
This is a pre-existing issue, but does apple_spmi_probe() need to flush
or drain the RX FIFO during initialization? There could be stale data
left over from bootloader or firmware interactions before the controller
is fully registered and mapped.

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