Re: [PATCH 18/18] thermal/drivers/imx91: Remove redundant dev_err_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Driver unbind causes a fatal hardware access and double-disable of the TMU clock if the device is suspended.
- [Critical] Hardware is accessed before asynchronous PM resume completes, and PM usage count leaks on error.
- [Critical] Reading the temperature via sysfs when the thermal zone is disabled causes a kernel crash due to unclocked hardware access.
- [High] PM runtime usage counter is permanently leaked on error paths in `imx91_tmu_probe()`.
- [High] The IRQ handler accesses hardware registers without verifying the device is runtime-active, risking a bus error.
--

--- Patch [18]: [PATCH 18/18] thermal/drivers/imx91: Remove redundant dev_err_probe() ---
commit 49fc5692bf44b8731b85be1cb5d7f3ecfbbc5b8d
Author: Pan Chuang <panchuang@vivo.com>

thermal/drivers/imx91: Remove redundant dev_err_probe()

The devm_request_threaded_irq() now automatically logs detailed error
messages on failure. This eliminates the need for driver-specific
dev_err_probe() calls that previously printed generic messages.

> diff --git a/drivers/thermal/imx91_thermal.c b/drivers/thermal/imx91_thermal.c
> index 25915bb702bea..274eee3031429 100644
> --- a/drivers/thermal/imx91_thermal.c
> +++ b/drivers/thermal/imx91_thermal.c
> @@ -331,7 +331,7 @@ static int imx91_tmu_probe(struct platform_device *pdev)
>  					IRQF_ONESHOT, "imx91_thermal", tmu);
>  
>  	if (ret < 0)
> -		return dev_err_probe(dev, ret, "failed to request alarm irq\n");
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path permanently
leak the PM runtime usage counter?

If an error occurs during probe (e.g., in devm_thermal_of_zone_register(),
platform_get_irq(), or devm_request_threaded_irq()), the function returns
without dropping the usage counter.

The usage counter incremented by pm_runtime_get_noresume(dev) earlier in
probe is never decremented, preventing the device and its parent power
domain from entering low-power states.

>  
>  	pm_runtime_put(dev);
>  

[Severity: Critical]
This is a pre-existing issue, but does driver unbind cause a fatal hardware
access if the device is suspended?

drivers/thermal/imx91_thermal.c:imx91_tmu_action_remove() {
	struct imx91_tmu *tmu = data;

	/* disable tmu */
	imx91_tmu_enable(tmu, false);
}

During driver unbind, devres executes cleanup actions in reverse order:
pm_runtime_disable() (which does not wake the device), followed by
imx91_tmu_action_remove(), and finally clk_disable_unprepare() from
devm_clk_get_enabled().

If the device is suspended (its normal idle state), its clock is already
disabled.

Calling imx91_tmu_enable() writes to hardware registers without a clock, which
can cause a fatal bus fault that crashes the system.

Will the devres clock cleanup also underflow the clock reference count?

[Severity: High]
This is a pre-existing issue, but does the IRQ handler access hardware
registers without verifying the device is runtime-active?

drivers/thermal/imx91_thermal.c:imx91_tmu_alarm_irq() {
	struct imx91_tmu *tmu = data;
	u32 val;

	val = readl_relaxed(tmu->base + IMX91_TMU_STAT0);
...
}

The interrupt handler reads IMX91_TMU_STAT0 unconditionally. The driver does
not disable the IRQ during imx91_tmu_runtime_suspend().

If a spurious interrupt fires, or if an interrupt is raised mid-flight
concurrently with the device entering runtime suspend, the handler will
access unclocked hardware, resulting in a fatal bus fault.

Should the handler use pm_runtime_get_if_active() to verify the device is
powered before touching registers?

[Severity: Critical]
This is a pre-existing issue, but is hardware accessed here before the
asynchronous PM resume completes?

drivers/thermal/imx91_thermal.c:imx91_tmu_change_mode() {
...
	if (mode == THERMAL_DEVICE_ENABLED) {
		ret = pm_runtime_get(tmu->dev);
		if (ret < 0)
			return ret;

		writel_relaxed(IMX91_TMU_CTRL0_THR1_IE | IMX91_TMU_CTRL0_THR1_MASK,
			       tmu->base + IMX91_TMU_CTRL0 + REG_CLR);
...
}

The driver uses pm_runtime_get(), which queues an asynchronous resume and
returns 0 if the device was suspended.

The driver then immediately accesses hardware registers, racing ahead of the
actual clock enablement in imx91_tmu_runtime_resume(). Accessing unclocked
hardware causes a fatal bus fault.

Additionally, if pm_runtime_get() returns a negative error code, the driver
returns directly without dropping the usage counter, causing a PM leak.

Should pm_runtime_resume_and_get() be used instead?

[Severity: Critical]
This is a pre-existing issue, but does reading the temperature via sysfs when
the thermal zone is disabled cause a kernel crash?

drivers/thermal/imx91_thermal.c:imx91_tmu_get_temp() {
	struct imx91_tmu *tmu = thermal_zone_device_priv(tz);
	s16 data;

	/* DATA0 is 16bit signed number */
	data = readw_relaxed(tmu->base + IMX91_TMU_DATA0);
...
}

When the thermal zone is disabled via sysfs, imx91_tmu_change_mode() calls
pm_runtime_put(), suspending the device and disabling its clock.

The thermal core does not block sysfs reads of the temp attribute while the
zone is disabled.

When any user reads the temperature, imx91_tmu_get_temp() directly accesses
the IMX91_TMU_DATA0 register without ensuring the device is resumed.

Accessing unclocked registers triggers a bus fault, allowing an unprivileged
user to cause a Denial of Service.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709023048.599150-1-panchuang@vivo.com?part=18