Re: [PATCH v5 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:

New issues:
- [High] Hardware register corruption due to unmasked ALARM and PANIC values

Pre-existing issues:
- [Critical] Use-After-Free of device data due to skipped thermal zone unregistration on probe error path
- [High] TOCTOU race on data->irq_enabled leads to permanent IRQ disablement
--

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

thermal/drivers/imx: Add calibration offset support

This patch reads an optional fsl,temp-calibration-offset-millicelsius
property from the device tree and applies it to the calibration formulas
for i.MX6/6SX/7D devices.

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

[ ... ]

> @@ -223,6 +224,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;
>  	critical_value = (data->c2 - panic_temp) / data->c1;
>  
>  	regmap_write(map, soc_data->panic_alarm_ctrl + REG_CLR,

[Severity: High]
Does this code risk corrupting the panic_alarm_ctrl register if a large
calibration offset is provided?

The calibration_offset is subtracted from panic_temp, and critical_value
is not masked before being shifted and written to the REG_SET register:

drivers/thermal/imx_thermal.c:imx_set_panic_temp() {
    critical_value = (data->c2 - panic_temp) / data->c1;
    ...
    regmap_write(map, soc_data->panic_alarm_ctrl + REG_SET,
                 critical_value << soc_data->panic_alarm_shift);
}

If the offset causes critical_value to exceed its bitfield width, could the
overhanging bits corrupt adjacent control bits?

> @@ -239,11 +241,14 @@ static void imx_set_alarm_temp(struct imx_thermal_data *data,
>  	int alarm_value;
>  
>  	data->alarm_temp = alarm_temp;
> +	alarm_temp -= data->calibration_offset;
>  
> -	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, 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,

[Severity: High]
Can similar register corruption occur here for IMX6/IMX6SX devices?

While a clamp() was added for TEMPMON_IMX7D, the else branch for
other versions calculates alarm_value without masking or clamping:

drivers/thermal/imx_thermal.c:imx_set_alarm_temp() {
    } else {
        alarm_value = (data->c2 - alarm_temp) / data->c1;
    }
    ...
    regmap_write(map, soc_data->high_alarm_ctrl + REG_SET,
                 alarm_value << soc_data->high_alarm_shift);
}

Could an excessively large positive or negative calibration offset result
in unmasked bits corrupting the high_alarm_ctrl register?

[ ... ]

> @@ -629,6 +635,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")) {

[Severity: Critical]
This is a pre-existing issue, but does this function leave the thermal
zone registered if pm_runtime_resume_and_get() fails later in the probe?

If pm_runtime_resume_and_get() fails near the end of probe, 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);
    ...
}

Because disable_runtime_pm is located after thermal_zone_unregister,
it skips unregistering the thermal zone. Since the device data is managed
by devm_kzalloc(), it will be freed when probe fails.

Could subsequent polling by the thermal core invoke imx_get_temp() and
dereference the freed data pointer, leading to a use-after-free?

[Severity: High]
This is a pre-existing issue, but is there a race condition that can
lead to permanent loss of thermal interrupts?

If a user-space process toggles the thermal zone mode via sysfs, it
calls imx_change_mode() which performs a non-atomic check:

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

If a hardware interrupt fires between the check and the call to
disable_irq(), the hardirq handler modifies the same flag and disables
the interrupt:

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

Would imx_change_mode() then call disable_irq() a second time,
increasing the disable depth to 2? Since future enable attempts will only
decrement the depth to 1, could this leave the interrupt permanently
disabled and force the system to rely on slower polling?

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