Re: [PATCH v8 2/5] thermal: samsung: Add Exynos ACPM TMU driver GS101

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Potential IRQ storm and permanent interrupt shutdown during system suspend due to missing `disable_irq()`.
- [Medium] Device clock leak during module removal or probe error due to concurrent sysfs access thwarting synchronous PM suspend.
--

commit b26c120c7a5d402216fea8bcbd596b1277417d90
Author: Tudor Ambarus <tudor.ambarus@linaro.org>
Subject: thermal: samsung: Add Exynos ACPM TMU driver GS101

This adds a new Thermal Management Unit (TMU) driver for Samsung Exynos SoCs 
like the GS101, which uses the Alive Clock and Power Manager (ACPM). The 
driver handles direct memory-mapped access for interrupts but delegates 
functional tasks to ACPM firmware via IPC.

> diff --git a/drivers/thermal/samsung/acpm-tmu.c b/drivers/thermal/samsung/acpm-tmu.c
> new file mode 100644
> index 0000000000000..f9802080acd7c
> --- /dev/null
> +++ b/drivers/thermal/samsung/acpm-tmu.c

[ ... ]

> +static void acpm_tmu_remove(struct platform_device *pdev)
> +{
> +	struct acpm_tmu_priv *priv = platform_get_drvdata(pdev);
> +
> +	/* Stop IRQ first to prevent race with thread_fn */
> +	disable_irq(priv->irq);
> +
> +	/*
> +	 * Disable autosuspend to force the subsequent pm_runtime_put_sync()
> +	 * inside acpm_tmu_control() to synchronously suspend the device
> +	 * immediately, preventing clock leaks when the driver is removed.
> +	 */
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +	acpm_tmu_control(priv, false, false);
> +}

[Severity: Medium]
Since devm_thermal_of_zone_register() is called after devm_pm_runtime_enable()
in acpm_tmu_probe(), the thermal zones are still active here and devres will 
unregister them only after acpm_tmu_remove() returns.

If userspace is concurrently reading sysfs via acpm_tmu_get_temp(), it will 
hold a PM runtime reference. Will the synchronous pm_runtime_put_sync() inside 
acpm_tmu_control() fail to actually suspend the device because the usage count
is greater than zero?

When the concurrent read finishes, it will call pm_runtime_put_autosuspend(). 
Because autosuspend was disabled above, this queues an asynchronous suspend. 
Would devres then continue cleanup and call pm_runtime_disable(), cancelling 
this pending asynchronous PM work and leaving the device permanently active?

[ ... ]

> +static int acpm_tmu_suspend(struct device *dev)
> +{
> +	struct acpm_tmu_priv *priv = dev_get_drvdata(dev);
> +	struct acpm_handle *handle = priv->handle;
> +	const struct acpm_tmu_ops *ops = &handle->ops->tmu;
> +	int ret, restore_ret;
> +
> +	ret = acpm_tmu_control(priv, false, false);
> +	if (ret)
> +		goto err_restore_sensors;
> +
> +	/* APB clock not required for this specific msg */
> +	ret = ops->suspend(handle, priv->mbox_chan_id);
> +	if (ret)
> +		goto err_restore_sensors;
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		goto err_resume_acpm;
> +
> +	return 0;

[Severity: High]
Unlike acpm_tmu_remove(), this suspend sequence does not call disable_irq().

If a thermal interrupt fires just before or during system suspend, it will 
wake the IRQ thread. When acpm_tmu_thread_fn() runs, it calls 
pm_runtime_resume_and_get(), which will fail with -EACCES because runtime PM 
is disabled by pm_runtime_force_suspend() here.

Since the thread will return IRQ_NONE without clearing the hardware interrupt,
could this lead to an IRQ storm when the IRQ core unmasks the line? If the 
spurious interrupt detector permanently disables the interrupt line, this might
break thermal monitoring entirely upon system resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-acpm-tmu-v8-0-0f1810a356e6@linaro.org?part=2