Re: [PATCH v3 4/4] soundwire: qcom: check clk_prepare_enable() return values

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] soundwire: qcom: enable_irq() in swrm_runtime_resume() causes
  wake IRQ storm
- [Low] soundwire: qcom: missing Fixes trailers for clk_prepare_enable()
  check
- [Low] soundwire: qcom: qcom_swrm_irq_handler() returns IRQ_HANDLED on
  clk error
--

commit 5c5235a561fc40c30680eee11304261327eab6cc
Author: Daniel Carter <yass27106@gmail.com>

> soundwire: qcom: check clk_prepare_enable() return values
>
> clk_prepare_enable() can fail; unchecked, the driver proceeds to
> touch registers with the interface clock off, which aborts on arm64.
> Check the return value in the interrupt handler, in probe and in the
> runtime resume path, re-enabling the wake IRQ on the resume error
> path.
>
> Signed-off-by: Daniel Carter <yass27106@gmail.com>

[Severity: Low]
Since this fixes unchecked clk_prepare_enable() calls introduced in two
earlier commits, should this include Fixes: tags?

  Fixes: 02efb49aa805 ("soundwire: qcom: add support for SoundWire controller")
  Fixes: 74e79da9fd46 ("soundwire: qcom: add runtime pm support")

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 0bdeb076a2e0e..ec510809766bb 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -799,7 +799,9 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>  	u32 i;
>  	int devnum;
>  	int ret = IRQ_HANDLED;
> -	clk_prepare_enable(ctrl->hclk);
> +
> +	if (clk_prepare_enable(ctrl->hclk))
> +		return IRQ_HANDLED;

[Severity: Low]
Should this return IRQ_NONE instead of IRQ_HANDLED when
clk_prepare_enable() fails?  The interrupt status is neither read nor
cleared, and returning IRQ_HANDLED causes irq_thread_fn() to mark the
interrupt as handled, preventing the genirq spurious interrupt detector
from disabling a stuck line.

[ ... ]

> @@ -1821,7 +1827,14 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
>  			disable_irq_nosync(ctrl->wake_irq);
>  	}
>  
> -	clk_prepare_enable(ctrl->hclk);
> +	ret = clk_prepare_enable(ctrl->hclk);
> +	if (ret) {
> +		if (ctrl->wake_irq > 0) {
> +			if (irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq)))
> +				enable_irq(ctrl->wake_irq);
> +		}
> +		return ret;
> +	}

[Severity: Medium]
Can calling enable_irq(ctrl->wake_irq) here cause a level-high wake IRQ
storm when swrm_runtime_resume() is invoked from
qcom_swrm_wake_irq_handler()?

ctrl->wake_irq is registered with IRQF_TRIGGER_HIGH | IRQF_ONESHOT.  During
an in-band wakeup, the slave holds the line high until the bus exits clock
stop, and qcom_swrm_wake_irq_handler() synchronously runs
swrm_runtime_resume() via pm_runtime_get_sync():

drivers/soundwire/qcom.c:qcom_swrm_wake_irq_handler() {
	ret = pm_runtime_get_sync(ctrl->dev);
	if (ret < 0 && ret != -EACCES) {
		dev_err_ratelimited(ctrl->dev,
				    "pm_runtime_get_sync failed in %s, ret %d\n",
				    __func__, ret);
		pm_runtime_put_noidle(ctrl->dev);
		return ret;
	}

	if (ctrl->wake_irq > 0) {
		if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq)))
			disable_irq_nosync(ctrl->wake_irq);
	}
    ...
}

When clk_prepare_enable() fails, enable_irq() unmasks ctrl->wake_irq at the
irqchip (even while the oneshot threaded handler is running), and
qcom_swrm_wake_irq_handler() returns early before its own
disable_irq_nosync(ctrl->wake_irq).  Because the bus never resumed to clear
the wakeup condition, the slave continues driving the level-high line and
ctrl->wake_irq immediately re-fires in a loop.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1ce6d7664e48413d9a5a8b11a986f746@gmail.com?part=4