Re: [PATCH v3 2/6] peci: controller: Add StarFive JHB100 PECI driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] peci: missing hardware abort/reset on transfer timeout causes spurious completion
--

commit 362c32d18ca6b1082855225b6845fe8810b06db0
Author: Changhuang Liang <changhuang.liang@starfivetech.com>

peci: controller: Add StarFive JHB100 PECI driver

This commit adds the PECI controller driver for the StarFive JHB100 SoC,
providing PECI protocol communication support for CPU thermal management.

> diff --git a/drivers/peci/controller/peci-starfive.c b/drivers/peci/controller/peci-starfive.c
> new file mode 100644
> index 0000000000000..93aca2413f5ca
> --- /dev/null
> +++ b/drivers/peci/controller/peci-starfive.c

[ ... ]

> +static int starfive_peci_xfer(struct peci_controller *controller,
> +			      u8 addr, struct peci_request *req)
> +{

[ ... ]

> +	priv->status = 0;
> +
> +	regmap_write_bits(priv->regmap, STARFIVE_PECI_CTRL,
> +			  STARFIVE_PECI_CTRL_START, STARFIVE_PECI_CTRL_START);
> +
> +	spin_unlock_irq(&priv->lock);

[Severity: High]
Does initiating a new transfer here require clearing the interrupt status
first?

If a previous transaction timed out, the hardware might eventually finish
and set STARFIVE_PECI_STS_XFER_DONE in STARFIVE_PECI_INT_STS, leaving an
interrupt pending. When spin_unlock_irq() executes, the delayed IRQ
handler could fire immediately.

Since the handler reads the stale XFER_DONE bit and calls
complete(&priv->xfer_complete), could this falsely complete the new
transfer prematurely and read in-progress or garbage hardware FIFO data?

> +
> +	ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete,
> +							timeout);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret == 0) {
> +		dev_dbg(priv->dev, "Timeout waiting for a response\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
Should the hardware be reset or the pending interrupts cleared when this
transfer times out or is interrupted?

When wait_for_completion_interruptible_timeout() returns 0 or a negative
error code, starfive_peci_xfer() bails out early without aborting the
active transaction in the controller.

This leaves the hardware executing and capable of asserting the IRQ line
later, potentially desynchronizing the protocol state for the next transfer.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008100623.7085-1-changhuang.liang@starfivetech.com?part=2