Re: [PATCH v9 3/3] thermal/drivers/imx: Add temperature offset support

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Temperature calibration offset compromises hardware die protection limits.

Pre-existing issues:
- [High] Use-After-Free of thermal zone device in the alarm IRQ thread during driver removal.
- [High] Race condition between imx_change_mode and the hardirq handler permanently disables the thermal alarm IRQ.
- [High] Missing pm_runtime_put calls in error paths leak runtime PM usage counters.
--

--- Patch [3]: [PATCH v9 3/3] thermal/drivers/imx: Add temperature offset support ---
commit 481f612125b42a851957ea2af55ad414681bb494
Author: Haoning CHENG <Haoning.CHENG@cn.bosch.com>

thermal/drivers/imx: Add temperature offset support
    
Some boards need a small per-design offset to make the TEMPMON reading
better correspond to the SoC package-surface temperature. This reads the
optional fsl,temp-calibration-offset-millicelsius property from DT and
applies it uniformly to the i.MX6/6SX/7D formulas.

> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 168069a16e930..87ae2e40cc6fe 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c

[ ... ]

> @@ -223,6 +228,7 @@ static void imx_set_panic_temp(struct imx_thermal_data *data,
>  	struct regmap *map = data->tempmon;
>  	int critical_value;
>  
> +	panic_temp -= data->calibration_offset;

[Severity: Critical]
Can a negative calibration offset cause the hardware panic limit to exceed
the safe maximum die temperature?

If a negative offset is specified in the device tree for a cooler package,
subtracting it here shifts the hardware panic limit higher. For example,
if the OCOTP die limit is 95C and the offset is -20C, does this program
the hardware to alarm at 115C, which exceeds the safe threshold specified in
the SoC fuses?

[ ... ]

> @@ -283,6 +290,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
>  		*temp = (n_meas - data->c1 + 25) * 1000;
>  	else
>  		*temp = data->c2 - n_meas * data->c1;
> +	*temp += data->calibration_offset;
>  
>  	/* Update alarm value to next higher trip point for TEMPMON_IMX6Q */
>  	if (data->socdata->version == TEMPMON_IMX6Q) {

[Severity: High]
This is a pre-existing issue, but does this function leak runtime PM usage
counters on error paths?

Looking at imx_get_temp(), if the sensor data is invalid, it returns
without dropping the reference:

    ret = pm_runtime_resume_and_get(data->dev);
    if (ret < 0)
        return ret;

    regmap_read(map, soc_data->temp_data, &val);

    if ((val & soc_data->temp_valid_mask) == 0)
        return -EAGAIN;

Similarly, in imx_set_trip_temp(), if the requested temperature is out of
bounds, it returns -EINVAL without calling pm_runtime_put(). Do these
paths permanently prevent the device from suspending?

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
of the thermal zone device during driver removal?

When the driver is removed, thermal_zone_device_unregister() is called
which frees data->tz:

imx_thermal_remove()
    ...
    thermal_zone_device_unregister(data->tz);

However, the devm-managed IRQ is still registered and active until after
remove completes. If the thermal alarm fires in this window, will the IRQ
thread dereference the freed pointer here:

imx_thermal_alarm_irq_thread()
    ...
    thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED);

[Severity: High]
This is a pre-existing issue, but can a race condition permanently disable
the thermal alarm IRQ?

If imx_change_mode() is called to disable the thermal device, it performs
a check-then-act without locking against the hardirq:

imx_change_mode()
    ...
    if (data->irq_enabled) {
        disable_irq(data->irq);
        data->irq_enabled = false;
    }

If the thermal alarm fires concurrently, the hardirq handler will also
disable the IRQ:

imx_thermal_alarm_irq()
    ...
    disable_irq_nosync(irq);
    data->irq_enabled = false;

Does this cause the IRQ disable depth to increment to 2, meaning a
subsequent enable_irq() will leave the hardware alarm permanently disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-b4-symana21-11221-imx-thermal-support-upstream-6-18-v9-0-75f4af8974f4@cn.bosch.com?part=3