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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Initialization order race condition and Use-After-Free in probe failure and removal paths.
- [High] Incorrect asynchronous PM put during device removal and system suspend.
- [High] Thermal sensor permanently disabled on threshold update failure.
- [Medium] Unconditional IRQ_HANDLED masks spurious interrupts.
- [Medium] Time-Of-Check to Time-Of-Use race condition in sensor state evaluation.
- [Medium] Incomplete cleanup on sensor disable failure.
- [Low] Potential uninitialized stack memory leak.
--

commit ad2d3d6566ebe8d41ffa801ff227d2afbb7dfb15
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/acpm-tmu.c b/drivers/thermal/samsung/acpm-tmu.c
> new file mode 100644
> index 0000000000000..d4e42b23c0c13
> --- /dev/null
> +++ b/drivers/thermal/samsung/acpm-tmu.c

[ ... ]

> +static int acpm_tmu_control(struct acpm_tmu_priv *priv, bool on)
> +{
> +	struct device *dev = priv->dev;
> +	int i, ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < priv->num_sensors; i++) {
> +		struct acpm_tmu_sensor *sensor = &priv->sensors[i];
> +
> +		/* Skip sensors that weren't found in DT */
> +		if (!sensor->tzd)
> +			continue;
> +
> +		mutex_lock(&sensor->lock);
> +		ret = acpm_tmu_op_tz_control(sensor, on);
> +		mutex_unlock(&sensor->lock);
> +		if (ret)
> +			break;

[Severity: Medium]
When called with on = false during driver removal or system suspend, if
disabling one sensor fails, will this break leave the remaining sensors
active?

> +	}
> +
> +	pm_runtime_put_autosuspend(dev);
> +	return ret;
> +}

[Severity: High]
When acpm_tmu_control() is called during driver removal via
acpm_tmu_remove(), this uses an asynchronous put. Will this pending
asynchronous suspend work be cancelled by pm_runtime_disable() during the
subsequent devres cleanup, leaving the hardware in an active power state?

[ ... ]

> +static int acpm_tmu_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct acpm_tmu_sensor *sensor = thermal_zone_device_priv(tz);
> +	struct acpm_tmu_priv *priv = sensor->priv;
> +	struct acpm_handle *handle = priv->handle;
> +	const struct acpm_tmu_ops *ops = &handle->ops->tmu;
> +	struct device *dev = priv->dev;
> +	int acpm_temp, ret;

[Severity: Low]
Can acpm_temp be used uninitialized here? 

If ops->read_temp() returns 0 without writing to acpm_temp, will this expose
uninitialized stack memory to user-space?

> +
> +	if (!sensor->enabled)
> +		return -EAGAIN;

[ ... ]

> +static int acpm_tmu_update_thresholds(struct acpm_tmu_sensor *sensor,
> +				      u8 thresholds[2], u8 inten)
> +{
> +	struct acpm_tmu_priv *priv = sensor->priv;
> +	struct acpm_handle *handle = priv->handle;
> +	const struct acpm_tmu_ops *ops = &handle->ops->tmu;
> +	unsigned int mbox_chan_id = priv->mbox_chan_id;
> +	u8 acpm_sensor_id = sensor->group->id;
> +	bool was_enabled = sensor->enabled;
> +	int ret;
> +
> +	guard(mutex)(&sensor->lock);

[Severity: Medium]
Could there be a race condition evaluating sensor->enabled before acquiring
the lock? 

If a concurrent thermal callback modifies the state, might was_enabled cache
a stale value and restore the sensor incorrectly at the end of the function?

> +
> +	if (was_enabled) {
> +		ret = acpm_tmu_op_tz_control(sensor, false);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = ops->set_threshold(handle, mbox_chan_id, acpm_sensor_id,
> +				 thresholds, 2);
> +	if (ret)
> +		return ret;

[Severity: High]
If set_threshold() or set_interrupt_enable() fails and returns early here,
will the thermal sensor remain permanently disabled? 

Should the sensor be restored based on was_enabled even on error paths to
ensure thermal mitigation continues?

> +
> +	ret = ops->set_interrupt_enable(handle, mbox_chan_id, acpm_sensor_id,
> +					inten);
> +	if (ret)
> +		return ret;
> +
> +	/* Restore based on cached state. */
> +	if (was_enabled)
> +		ret = acpm_tmu_op_tz_control(sensor, true);
> +
> +	return ret;
> +}

[ ... ]

> +static irqreturn_t acpm_tmu_thread_fn(int irq, void *id)
> +{
> +	struct acpm_tmu_priv *priv = id;
> +	struct acpm_handle *handle = priv->handle;
> +	const struct acpm_tmu_ops *ops = &handle->ops->tmu;
> +	struct device *dev = priv->dev;
> +	int i, ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to resume: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	for (i = 0; i < priv->num_sensors; i++) {
> +		struct acpm_tmu_sensor *sensor = &priv->sensors[i];
> +		bool pending_irq = false;
> +
> +		if (!sensor->tzd)
> +			continue;
> +
> +		ret = acpm_tmu_has_pending_irq(sensor, &pending_irq);
> +		if (ret || !pending_irq)
> +			continue;
> +
> +		thermal_zone_device_update(sensor->tzd,
> +					   THERMAL_EVENT_UNSPECIFIED);
> +
> +		scoped_guard(mutex, &sensor->lock) {
> +			ret = ops->clear_tz_irq(handle, priv->mbox_chan_id,
> +						sensor->group->id);
> +			if (ret)
> +				dev_err(priv->dev, "Sensor %d: failed to clear IRQ (%d)\n",
> +					i, ret);
> +		}
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Will returning IRQ_HANDLED unconditionally defeat the spurious interrupt
detection? 

If no pending interrupts were found for any sensor, should this return
IRQ_NONE to allow the core IRQ subsystem to detect and handle a potential
IRQ storm?

[ ... ]

> +static int acpm_tmu_probe(struct platform_device *pdev)
> +{
> +	const struct acpm_tmu_driver_data *data = &acpm_tmu_gs101;
> +	struct acpm_handle *acpm_handle;
> +	struct device *dev = &pdev->dev;
> +	struct acpm_tmu_priv *priv;
> +	struct regmap *regmap;
> +	void __iomem *base;
> +	int i, ret;
> +
> +	acpm_handle = devm_acpm_get_by_phandle(dev);

[ ... ]

> +	ret = devm_request_threaded_irq(dev, priv->irq, NULL,
> +					acpm_tmu_thread_fn, IRQF_ONESHOT,
> +					dev_name(dev), priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 100);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to resume device\n");
> +
> +	ret = acpm_handle->ops->tmu.init(acpm_handle, priv->mbox_chan_id);
> +	if (ret) {
> +		ret = dev_err_probe(dev, ret, "Failed to init TMU\n");
> +		goto err_pm_put;
> +	}
> +
> +	for (i = 0; i < priv->num_sensors; i++) {
> +		struct acpm_tmu_sensor *sensor = &priv->sensors[i];
> +
> +		mutex_init(&sensor->lock);
> +		sensor->group = &data->sensor_groups[i];
> +		sensor->priv = priv;
> +
> +		sensor->tzd = devm_thermal_of_zone_register(dev, i, sensor,
> +							    &acpm_tmu_sensor_ops);

[Severity: High]
Since devres executes cleanups in reverse order of registration, the
thermal zones will be unregistered and freed before the IRQ is disabled.

If an interrupt fires during this window (either on probe failure or driver
removal), will acpm_tmu_thread_fn() access the freed sensor->tzd pointer,
causing a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-acpm-tmu-v5-0-85fde739752e@linaro.org?part=2