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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch introduces `fsl,temp-calibration-offset-millicelsius` without bounding its value. For non-IMX7D platforms, an accidentally or maliciously large calibration offset will cause `alarm_value` (and `critical_value`) to exceed their bitmask, corrupting adjacent register fields during the unbounded shift and `REG_SET` operation.

Pre-existing issues:
- [High] If `pm_runtime_resume_and_get()` fails in probe, the error path leaks the thermal zone device and triggers a double-put of the runtime PM usage count. Additionally, `imx_thermal_remove` has an unconditional PM usage count underflow.
- [High] Early error returns in `imx_get_temp` fail to drop the PM runtime reference, causing a resource leak.
- [High] A data race on the boolean flag `data->irq_enabled` between `imx_change_mode` and the hardirq handler can cause the thermal alarm IRQ to become permanently disabled.
- [High] The thermal alarm IRQ thread can execute after the thermal zone has been unregistered and freed during driver removal.
--

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

thermal/drivers/imx: Add calibration offset support

This patch introduces a device tree property to apply a small calibration 
offset to the reported CPU temperature. It reads the property and applies it 
to the i.MX6/6SX/7D calibration formulas while preserving the default behavior.

> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -240,10 +241,13 @@ static void imx_set_alarm_temp(struct imx_thermal_data *data,
>  
>  	data->alarm_temp = alarm_temp;
>  
> -	if (data->socdata->version == TEMPMON_IMX7D)
> -		alarm_value = alarm_temp / 1000 + data->c1 - 25;
> -	else
> +	if (data->socdata->version == TEMPMON_IMX7D) {
> +		alarm_value = DIV_ROUND_UP(alarm_temp - data->calibration_offset,
> +					   1000) + data->c1 - 25;
> +		alarm_value = clamp(alarm_value, 0, 0x1ff);
> +	} else {
>  		alarm_value = (data->c2 - alarm_temp) / data->c1;
> +	}
>  
>  	regmap_write(map, soc_data->high_alarm_ctrl + REG_CLR,
>  		     soc_data->high_alarm_mask);

[Severity: Medium]
For non-IMX7D platforms, does this code allow an unbounded calibration offset
from the device tree to overflow the alarm_value?

If a large offset is provided, data->c2 will be skewed. This could cause
alarm_value to exceed its bitmask since it is not clamped in the else branch.
Could this corrupt adjacent register fields during the shifted REG_SET
operation below?

> @@ -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 is a pre-existing issue, but looking earlier in imx_get_temp(), if the
sensor is busy, the function returns early without dropping the PM runtime
reference:

drivers/thermal/imx_thermal.c:imx_get_temp() {
    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;
    ...
}

Does this cause a permanent resource leak of the device PM runtime usage
counter when -EAGAIN is returned?

> @@ -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);
> +
>  	if (of_property_present(dev->of_node, "nvmem-cells")) {
>  		ret = imx_init_from_nvmem_cells(pdev);

[Severity: High]
This is a pre-existing issue, but further down in imx_thermal_probe(), if
pm_runtime_resume_and_get() fails, the error path jumps to disable_runtime_pm:

drivers/thermal/imx_thermal.c:imx_thermal_probe() {
    ...
    ret = pm_runtime_resume_and_get(data->dev);
    if (ret < 0)
        goto disable_runtime_pm;
    ...
thermal_zone_unregister:
    thermal_zone_device_unregister(data->tz);
disable_runtime_pm:
    pm_runtime_put_noidle(data->dev);
    pm_runtime_disable(data->dev);
    ...
}

Since the jump bypasses thermal_zone_device_unregister(), and data is devres
managed, does this leave an orphaned thermal zone registered that will cause a
use-after-free when polled? Additionally, since the usage count is already 0
on resume failure, does the pm_runtime_put_noidle() cause an underflow?

[Severity: High]
This is a pre-existing issue, but there appears to be a use-after-free during
driver removal.

In imx_thermal_remove(), thermal_zone_device_unregister(data->tz) is explicitly
called, but the IRQ thread is devres-managed and continues to be active until
after the remove function returns:

drivers/thermal/imx_thermal.c:imx_thermal_alarm_irq_thread() {
    ...
    thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED);
    ...
}

Can the devm-managed IRQ fire during module teardown and access the freed
data->tz pointer here?

[Severity: High]
This is a pre-existing issue, but there seems to be a data race on the
data->irq_enabled flag that can permanently disable the thermal alarm IRQ.

If the thermal zone mode is changed via sysfs concurrently with a hardware
thermal alarm interrupt, both paths check and modify the flag locklessly:

drivers/thermal/imx_thermal.c:imx_change_mode() {
    if (data->irq_enabled) {
        disable_irq(data->irq);
        data->irq_enabled = false;
    }
    ...
}

drivers/thermal/imx_thermal.c:imx_thermal_alarm_irq() {
    ...
    disable_irq_nosync(irq);
    data->irq_enabled = false;
    ...
}

Could both paths execute their disable function, incrementing the IRQ depth
twice, such that subsequent enables leave the IRQ physically disabled?

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