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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The new `calibration_offset` is added to `data->c2`. This unvalidated offset can cause `alarm_value` and `critical_value` in `imx_set_alarm_temp()` and `imx_set_panic_temp()` to exceed the 12-bit hardware mask for i.MX6. Since these values are left-shifted and written using `REG_SET` without clamping, it will corrupt adjacent register fields (e.g., powering down the sensor unintentionally). Additionally, negative values left-shifted trigger Undefined Behavior.
- [Medium] The macro `DIV_ROUND_UP(n, d)` expands to `((n) + (d) - 1) / (d)`. Because C integer division truncates towards zero, this macro produces mathematically incorrect results when the numerator is negative. In `imx_set_alarm_temp`, if `alarm_temp` is smaller than `data->calibration_offset` (e.g., a low trip threshold combined with a large DT offset), the numerator becomes negative. This results in an off-by-one error (1°C) when calculating the hardware alarm threshold for i.MX7D. A safe division approach that handles negative values should be used.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but both `imx_get_temp()` and `imx_set_trip_temp()` acquire a runtime PM reference using `pm_runtime_resume_and_get()`. However, if an early return condition is met (e.g., the sensor measurement is not yet valid returning `-EAGAIN`, or the trip temperature is out of bounds returning `-EINVAL`), the functions return directly without dropping the runtime PM reference. This permanently leaks the reference.
- [High] This problem wasn't introduced by this patch, but the thermal clock `data->thermal_clk` is successfully prepared and enabled in `imx_thermal_probe()`. However, the driver's removal function, `imx_thermal_remove()`, completely lacks the corresponding `clk_disable_unprepare()` call. This leads to a clock reference leak every time the driver module is unloaded.
--

--- Patch [2]: [PATCH v4 2/2] thermal/drivers/imx: Add calibration offset support ---
commit 0856324278f99d619114b6ca18250e57e7dbf3ca
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

[ ... ]

> @@ -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;

[Severity: Medium]
Since C integer division truncates toward zero, does DIV_ROUND_UP() produce
mathematically incorrect results if alarm_temp is smaller than
data->calibration_offset and the numerator becomes negative?

> +		alarm_value = clamp(alarm_value, 0, 0x1ff);
> +	} else {
>  		alarm_value = (data->c2 - alarm_temp) / data->c1;
> +	}

[Severity: High]
With the unvalidated calibration_offset now added to data->c2, can
alarm_value exceed the hardware mask for i.MX6 or become negative?

If so, does the left-shift and REG_SET occurring just below this block:

	regmap_write(map, soc_data->high_alarm_ctrl + REG_SET,
		     alarm_value << soc_data->high_alarm_shift);

corrupt adjacent register fields? A similar sequence exists in
imx_set_panic_temp() with critical_value.

[ ... ]

> @@ -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 does this function leak the runtime PM
reference if the sensor measurement is not yet valid?

Earlier in imx_get_temp(), pm_runtime_resume_and_get() is called, but the
error path 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;

This same pattern also appears in imx_set_trip_temp() when returning -EINVAL.

[ ... ]

> @@ -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);
>  		if (ret)

[Severity: High]
This is a pre-existing issue, but does the driver leak the thermal clock
reference when removed?

imx_thermal_probe() prepares and enables data->thermal_clk:

	ret = clk_prepare_enable(data->thermal_clk);
	if (ret) {
		dev_err(dev, "failed to enable thermal clk: %d\n", ret);

However, imx_thermal_remove() does not appear to call clk_disable_unprepare()
when the driver module is unloaded.

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