Re: [PATCH] thermal/drivers/imx9: Constify struct thermal_zone_device_ops

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

Pre-existing issues:
- [High] Unsafe asynchronous `pm_runtime_get()` used immediately prior to device register access.
--

--- Patch [1]: [PATCH] thermal/drivers/imx9: Constify struct thermal_zone_device_ops ---
commit a5aeff30f3e13d6747636801e7e678ac7b06d113
Author: Christophe JAILLET <[email protected]>

thermal/drivers/imx9: Constify struct thermal_zone_device_ops

Constify the thermal_zone_device_ops structure since it is not modified in
the driver. This moves the data to a read-only section, improving overall
security by protecting the function pointers.

> diff --git a/drivers/thermal/imx91_thermal.c b/drivers/thermal/imx91_thermal.c
> index 25915bb702bea..892a25440e06e 100644
> --- a/drivers/thermal/imx91_thermal.c
> +++ b/drivers/thermal/imx91_thermal.c
> @@ -225,7 +225,7 @@ static int imx91_tmu_change_mode(struct thermal_zone_device *tz, enum thermal_de
>  	return 0;
>  }
>  
> -static struct thermal_zone_device_ops tmu_tz_ops = {
> +static const struct thermal_zone_device_ops tmu_tz_ops = {
>  	.get_temp = imx91_tmu_get_temp,
>  	.change_mode = imx91_tmu_change_mode,

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with the
imx91_tmu_change_mode function referenced here.

Can this result in a bus fault if the device clocks are not yet enabled?

Looking at imx91_tmu_change_mode(), it uses the asynchronous
pm_runtime_get():

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);
...
}

Because pm_runtime_get() queues an asynchronous resume and returns
immediately, writel_relaxed() could access memory-mapped registers before
the PM workqueue has completed the resume and enabled the device's clocks.

Should this use pm_runtime_resume_and_get() instead to ensure the device
is synchronously resumed before register access?

>  	.set_trips = imx91_tmu_set_trips,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5cce9ea713edcf2aa3c0efd5d31e9d9fd9f40a88.1786220585.git.christophe.jaillet@wanadoo.fr?part=1