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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing Error Rollback in `acpm_tmu_suspend` leaves thermal sensors permanently disabled if system suspend fails.
- [High] Clock and power leak during probe error path and system sleep due to autosuspend preventing synchronous runtime suspend.
- [Medium] Kconfig dependency allows COMPILE_TEST without EXYNOS_ACPM_PROTOCOL, leading to a linker error.
--

commit 72d6aa9ba0b741ec961e0a99b3827f24785a2e52
Author: Tudor Ambarus <tudor.ambarus@linaro.org>

thermal: samsung: Add Exynos ACPM TMU driver GS101

Add driver for the Thermal Management Unit (TMU) managed via the Alive
Clock and Power Manager (ACPM), found on Samsung Exynos SoCs such as
Google GS101 (and Exynos850, autov920, etc.).

> diff --git a/drivers/thermal/samsung/Kconfig b/drivers/thermal/samsung/Kconfig
> index f4eff5a41a84c..383ae3f56cbb2 100644
> --- a/drivers/thermal/samsung/Kconfig
> +++ b/drivers/thermal/samsung/Kconfig
[ ... ]
> @@ -9,3 +9,22 @@ config EXYNOS_THERMAL
>  	  the TMU, reports temperature and handles cooling action if defined.
>  	  This driver uses the Exynos core thermal APIs and TMU configuration
>  	  data from the supported SoCs.
> +
> +config EXYNOS_ACPM_THERMAL
> +	tristate "Exynos ACPM thermal management unit driver"
> +	depends on THERMAL_OF
> +	depends on HAS_IOMEM
> +	depends on EXYNOS_ACPM_PROTOCOL || (COMPILE_TEST && !EXYNOS_ACPM_PROTOCOL)

[Severity: Medium]
Does this dependency rule permit compilation when the underlying protocol
is not built, leading to a linker error?

The Kconfig option explicitly allows compilation when COMPILE_TEST is
enabled and EXYNOS_ACPM_PROTOCOL is disabled. However, the driver
unconditionally calls devm_acpm_get_by_phandle() in acpm_tmu_probe().

Since include/linux/firmware/samsung/exynos-acpm-protocol.h does not
provide empty inline stub implementations for these APIs when the
protocol is disabled, building the driver in this configuration might
result in an unresolved symbol error at link time.

> diff --git a/drivers/thermal/samsung/acpm-tmu.c b/drivers/thermal/samsung/acpm-tmu.c
> new file mode 100644
> index 0000000000000..2de34b03b9eab
> --- /dev/null
> +++ b/drivers/thermal/samsung/acpm-tmu.c
[ ... ]
> +err_rollback:
> +	acpm_tmu_control_rollback(priv, i - 1);
> +err_pm_put:
> +	pm_runtime_put_sync(dev);
> +	return ret;
> +}

[Severity: High]
Will this leak the clock and power during the probe error path?

The driver enables runtime PM autosuspend with a 100ms delay. On error paths
in acpm_tmu_probe(), pm_runtime_put_sync() is called. Because autosuspend
is active, this drops the usage count but only schedules an asynchronous
suspend, leaving the device active.

When the probe function returns the error, devres cleanup automatically
invokes pm_runtime_disable(), which cancels the pending autosuspend timer.
The device gets permanently stuck in an active state, the runtime suspend
callback is never invoked, and the clock is leaked.

[ ... ]
> +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;
> +
> +	ret = acpm_tmu_control(priv, false);
> +	if (ret)
> +		return ret;

[Severity: High]
Should this roll back the partially disabled thermal sensors on failure?

If acpm_tmu_control() fails, the driver immediately returns the error.
Because acpm_tmu_control() is designed to continue processing even if an
individual sensor fails to disable, a failure return implies that some
sensors may have been successfully disabled while others failed.

Returning an error aborts the system suspend transition, meaning the PM
core assumes the device is still active and will not invoke the resume
callback. The successfully disabled sensors are thus left disabled
indefinitely, which silently breaks thermal mitigation on an active system.

Should we call acpm_tmu_control(priv, true) on failure, similar to the
existing rollback logic for ops->suspend()?

[Severity: High]
Additionally, will this leak the clock during system sleep due to autosuspend?

acpm_tmu_control() calls pm_runtime_put_sync(), which schedules an
autosuspend. During the subsequent suspend_late phase, the PM core calls
pm_runtime_disable(), cancelling the timer. The device remains active, and
its clock remains enabled throughout the entire system sleep duration.

It looks like acpm_tmu_remove() mitigates this by calling
pm_runtime_dont_use_autosuspend() before dropping the reference. Could a
similar fix be applied here and in the probe error paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-acpm-tmu-v7-0-8047dd54b0e9@linaro.org?part=2