Re: [PATCH v2 2/2] thermal/drivers/imx: Add calibration offset support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Premature thermal zone registration exposes sysfs before the IRQ is requested, allowing unbalanced IRQ enable/disable operations.
- [High] Data race on `data->irq_enabled` between process context and hardirq context leads to unbalanced IRQ disable depths and permanently disabled interrupts.
- [High] Use-After-Free of `data->tz` in the thermal alarm IRQ thread during driver removal due to skipped `disable_irq()` synchronization.
--

--- Patch [2]: [PATCH v2 2/2] thermal/drivers/imx: Add calibration offset support ---
commit 7cfd21023fa90494618a836d7b36a5a78f4f8447
Author: HaoNing Cheng <Haoning.CHENG@cn.bosch.com>

thermal/drivers/imx: Add calibration offset support
    
Some boards need a small per-design correction to align the reported CPU
temperature with board-level measurements.

Read the optional fsl,temp-calibration-offset-millicelsius property from
DT and apply it to the i.MX6/6SX/7D calibration formulas.

> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 5aaacbc534786..971d6b4737c68 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c

[ ... ]

> @@ -274,7 +278,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp)
>  
>  	/* See imx_init_calib() for formula derivation */
>  	if (data->socdata->version == TEMPMON_IMX7D)
> -		*temp = (n_meas - data->c1 + 25) * 1000;
> +		*temp = (n_meas - data->c1 + 25) * 1000 + data->calibration_offset;
>  	else
>  		*temp = data->c2 - n_meas * data->c1;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential race
condition involving the sysfs exposure of this temperature read.

In imx_thermal_probe(), the thermal zone is registered before the IRQ is
requested:

imx_thermal_probe() {
    data->tz = thermal_zone_device_register_with_trips(...)
    ...
    ret = devm_request_threaded_irq(...)
}

Could userspace trigger a read through sysfs immediately after registration,
calling into imx_get_temp() before the IRQ is fully requested?

If so, and if it reaches this path:

imx_get_temp() {
    if (!data->irq_enabled && *temp < data->alarm_temp) {
        data->irq_enabled = true;
        enable_irq(data->irq);
    }
}

Would it call enable_irq() on an unrequested IRQ, leading to unbalanced IRQ
enable/disable operations?

Furthermore, if devm_request_threaded_irq() fails, the error path
unregisters the thermal zone, which calls
imx_change_mode(THERMAL_DEVICE_DISABLED).  Since data->irq_enabled was set
to true, would it deterministically call disable_irq() on the unrequested IRQ?

[ ... ]

> @@ -629,6 +633,10 @@ static int imx_thermal_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, data);
>  
> +	of_property_read_s32(dev->of_node,
> +			     "fsl,temp-calibration-offset-millicelsius",
> +			     &data->calibration_offset);
> +

[Severity: High]
This is a pre-existing issue, but while reviewing the IRQ handling in this
driver, I noticed a potential data race on data->irq_enabled between process
context and hardirq context.

The flag is accessed and modified in imx_change_mode() without
synchronization:

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

It is also concurrently modified in hardirq context:

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

If imx_change_mode(THERMAL_DEVICE_DISABLED) reads data->irq_enabled as true,
and an alarm interrupt fires immediately after on another CPU, the hardirq
handler will run, call disable_irq_nosync(), and set the flag to false.

When the hardirq completes, would imx_change_mode() resume and
unconditionally call disable_irq(), incrementing the IRQ disable depth to 2?

Would subsequent reenables only decrement it by 1, leaving the hardware IRQ
permanently disabled?

[Severity: High]
This is another pre-existing issue related to the IRQ handling, but I
noticed a potential use-after-free of data->tz in the thermal alarm IRQ
thread during driver removal.

During imx_thermal_remove(), thermal_zone_device_unregister() is called,
which internally sets the mode to disabled by calling
imx_change_mode(THERMAL_DEVICE_DISABLED).

If a thermal alarm interrupt fired just prior, the hardirq handler will have
set data->irq_enabled = false and woken up the IRQ thread.  Because
data->irq_enabled is now false, imx_change_mode() skips calling disable_irq():

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

By skipping disable_irq(), does it fail to provide the required
synchronization barrier to wait for the IRQ thread to complete?

If thermal_zone_device_unregister() proceeds to free data->tz, could the
still-running imx_thermal_alarm_irq_thread() then dereference the freed
pointer:

imx_thermal_alarm_irq_thread() {
    thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED);
}

Would this lead to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-b4-symana21-11221-imx-thermal-support-upstream-6-18-v2-0-00ff72495e24@cn.bosch.com?part=2