Re: [PATCH v2 01/12] thermal/driver/tegra/soctherm: Use devm_ variant when registering a cooling device

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Hardware reset sequence completely bypassed during probe.
- [High] Double clock disable during driver removal and failure to fix the described race window.
- [Medium] Clock enablement and disablement order is reversed.
--

commit c2cefc8b595baea6413baca093c73c6b01f2bfd1
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>

thermal/driver/tegra/soctherm: Use devm_ variant when registering a cooling device

The driver is updated to use devm_thermal_of_cooling_device_register()
to automatically unregister the cooling device. To avoid a window where
the cooling device remains registered while the hardware is powered down,
it also switches to using devm_clk_get_enabled().

> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 5d26b52beaba7..790c4c96a9b5f 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c

[ ... ]

> @@ -2133,13 +2133,13 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>  		return PTR_ERR(tegra->reset);
>  	}
>  
> -	tegra->clock_tsensor = devm_clk_get(&pdev->dev, "tsensor");
> +	tegra->clock_tsensor = devm_clk_get_enabled(&pdev->dev, "tsensor");
>  	if (IS_ERR(tegra->clock_tsensor)) {
>  		dev_err(&pdev->dev, "can't get tsensor clock\n");
>  		return PTR_ERR(tegra->clock_tsensor);
>  	}
>  
> -	tegra->clock_soctherm = devm_clk_get(&pdev->dev, "soctherm");
> +	tegra->clock_soctherm = devm_clk_get_enabled(&pdev->dev, "soctherm");

Does this invert the clock enablement order?

The original soctherm_clk_enable() enabled the soctherm clock before the
tsensor clock:

drivers/thermal/tegra/soctherm.c:soctherm_clk_enable() {
	...
	if (enable) {
		err = clk_prepare_enable(tegra->clock_soctherm);
		if (err) {
			...
		}

		err = clk_prepare_enable(tegra->clock_tsensor);
	...
}

By retrieving and enabling tsensor first with devm_clk_get_enabled(), the
tsensor clock is now enabled before soctherm, and the devres LIFO cleanup will
disable soctherm before tsensor.

Will this cause undefined behavior if the tsensor block relies on the main
soctherm clock?

[ ... ]

> @@ -2171,10 +2171,6 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>  	if (!tegra->thermctl_tzs)
>  		return -ENOMEM;
>  
> -	err = soctherm_clk_enable(pdev, true);
> -	if (err)
> -		return err;
> -

By completely removing this call, doesn't it also bypass the hardware reset
sequence?

Looking at soctherm_clk_enable(), it performs a hardware reset along with clock
management:

drivers/thermal/tegra/soctherm.c:soctherm_clk_enable() {
	...
	reset_control_assert(tegra->reset);

	if (enable) {
		err = clk_prepare_enable(tegra->clock_soctherm);
		...
		err = clk_prepare_enable(tegra->clock_tsensor);
		...
	} else {
		...
	}

	reset_control_deassert(tegra->reset);
	...
}

Without this sequence, will the thermal controller hardware remain in an
uninitialized state?

[ ... ]

> @@ -2218,11 +2212,6 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>  	soctherm_debug_init(pdev);
>  
>  	return 0;
> -
> -disable_clocks:
> -	soctherm_clk_enable(pdev, false);
> -
> -	return err;
>  }
>  
>  static void tegra_soctherm_remove(struct platform_device *pdev)

The commit message states:

> A bug can be subtly introduced by this change because the driver still
> manually disables the hardware clocks during the probe error path and
> in tegra_soctherm_remove(). This creates a window where the cooling
> device remains registered and exposed to the thermal core and sysfs
> while the underlying hardware is powered down.
>
> In order to prevent this potential issue, use the devm_ variant for the
> clk.

Does the race window described here remain open? The manual call to
soctherm_clk_enable(pdev, false) is still present in tegra_soctherm_remove():

drivers/thermal/tegra/soctherm.c:tegra_soctherm_remove() {
	...
	soctherm_clk_enable(pdev, false);
}

Since this executes before devres automatically unregisters the cooling device,
the clocks are powered down while the cooling device is still registered.

Additionally, when devres later cleans up the clocks, will it cause a
double-disable and clock reference count underflow since the clocks were
already manually disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260422174305.2899095-1-daniel.lezcano@oss.qualcomm.com?part=1