* Re: [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support
2026-07-13 10:22 ` [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support Haoning CHENG via B4 Relay
@ 2026-07-13 10:38 ` sashiko-bot
2026-07-13 14:39 ` Frank Li
2026-07-13 14:43 ` Lukasz Luba
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-07-13 10:38 UTC (permalink / raw)
To: Haoning CHENG; +Cc: Frank.Li, imx, robh, devicetree, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] An unbounded DT property causes integer underflow in the alarm calculation, leading to out-of-bounds register bit manipulation.
Pre-existing issues:
- [High] Use-after-free of `data->tz` in the thermal alarm IRQ thread during device removal.
--
--- Patch [2]: [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support ---
commit 09bf5f606220dc9c50fce508974c8440db8631c3
Author: Haoning CHENG <Haoning.CHENG@cn.bosch.com>
thermal/drivers/imx: Add calibration offset support
This patch introduces support for a per-design temperature calibration
offset to align reported CPU temperatures with board-level measurements.
It reads the fsl,temp-calibration-offset-millicelsius property from the
device tree and applies it to the calibration formulas.
> 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 subtracting an unvalidated calibration_offset allow critical_value to
exceed the register bitfield width?
If the device tree provides an extremely large positive or negative value,
critical_value could overflow or underflow. When shifted and written to
REG_SET:
regmap_write(map, soc_data->panic_alarm_ctrl + REG_SET,
critical_value << soc_data->panic_alarm_shift);
Could this out-of-bounds value pollute adjacent register fields, since REG_SET
applies any bits that are 1? Should data->calibration_offset be clamped before
use?
> @@ -239,11 +241,14 @@ static void imx_set_alarm_temp(struct imx_thermal_data *data,
> int alarm_temp)
> 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,
> soc_data->high_alarm_mask);
[Severity: High]
Can alarm_value overflow the register bitfield for non-IMX7D variants?
The IMX7D variant uses clamp(), but the else branch does not:
} else {
alarm_value = (data->c2 - alarm_temp) / data->c1;
}
Since alarm_temp is adjusted by an unbounded calibration_offset, this could
result in an out-of-bounds alarm_value that pollutes adjacent fields when
written to soc_data->high_alarm_ctrl.
[ ... ]
> @@ -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")) {
> ret = imx_init_from_nvmem_cells(pdev);
> if (ret)
[Severity: High]
This is a pre-existing issue, but does this driver have a use-after-free
during device removal?
The driver requests its interrupt via devm_request_threaded_irq() in
imx_thermal_probe(). Because it uses devres, the IRQ is freed after
imx_thermal_remove() returns.
However, imx_thermal_remove() manually unregisters the thermal zone:
imx_thermal_remove()
...
thermal_zone_device_unregister(data->tz);
This frees data->tz. If the hardware thermal alarm triggers after this
unregister but before the devres cleanup, the IRQ handler
imx_thermal_alarm_irq_thread() will execute:
imx_thermal_alarm_irq_thread()
...
thermal_zone_device_update(data->tz, THERMAL_EVENT_UNSPECIFIED);
Could this dereference the freed data->tz pointer?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-b4-symana21-11221-imx-thermal-support-upstream-6-18-v7-0-09b6b7669be1@cn.bosch.com?part=2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support
2026-07-13 10:22 ` [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support Haoning CHENG via B4 Relay
2026-07-13 10:38 ` sashiko-bot
@ 2026-07-13 14:39 ` Frank Li
2026-07-13 14:43 ` Lukasz Luba
2 siblings, 0 replies; 6+ messages in thread
From: Frank Li @ 2026-07-13 14:39 UTC (permalink / raw)
To: Haoning.CHENG
Cc: Rafael J. Wysocki, Daniel Lezcano, Zhang Rui, Lukasz Luba,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, linux-pm,
devicetree, imx, linux-arm-kernel, linux-kernel
On Mon, Jul 13, 2026 at 06:22:57PM +0800, Haoning CHENG via B4 Relay wrote:
> From: Haoning CHENG <Haoning.CHENG@cn.bosch.com>
>
> 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. When the
> property is not present, the default offset remains 0, preserving the
> current behaviour.
>
> Signed-off-by: Haoning Cheng <Haoning.CHENG@cn.bosch.com>
> ---
> drivers/thermal/imx_thermal.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 38c993d1bcb3..0a443e608957 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -207,6 +207,7 @@ struct imx_thermal_data {
> struct regmap *tempmon;
> u32 c1, c2; /* See formula in imx_init_calib() */
> int temp_max;
> + s32 calibration_offset;
> int alarm_temp;
> int last_temp;
> bool irq_enabled;
> @@ -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,
> @@ -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);
Please use new patch update alarm_temp / 1000 to DIV_DOUND_UP() and clamp()
only and show reason why change this.
Frank
> + } else {
> alarm_value = (data->c2 - alarm_temp) / data->c1;
> + }
>
> regmap_write(map, soc_data->high_alarm_ctrl + REG_CLR,
> soc_data->high_alarm_mask);
> @@ -277,6 +282,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) {
> @@ -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")) {
> ret = imx_init_from_nvmem_cells(pdev);
> if (ret)
>
> --
> 2.43.0
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support
2026-07-13 10:22 ` [PATCH v7 2/2] thermal/drivers/imx: Add calibration offset support Haoning CHENG via B4 Relay
2026-07-13 10:38 ` sashiko-bot
2026-07-13 14:39 ` Frank Li
@ 2026-07-13 14:43 ` Lukasz Luba
2 siblings, 0 replies; 6+ messages in thread
From: Lukasz Luba @ 2026-07-13 14:43 UTC (permalink / raw)
To: Haoning.CHENG
Cc: linux-pm, Fabio Estevam, Sascha Hauer, Shawn Guo, devicetree,
Krzysztof Kozlowski, Rob Herring, Zhang Rui,
Pengutronix Kernel Team, Daniel Lezcano, Rafael J. Wysocki,
Conor Dooley, imx, linux-arm-kernel, linux-kernel
On 7/13/26 11:22, Haoning CHENG via B4 Relay wrote:
> From: Haoning CHENG <Haoning.CHENG@cn.bosch.com>
>
> 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. When the
> property is not present, the default offset remains 0, preserving the
> current behaviour.
s/behaviour/behavior/
Also, you can add a description in this patch header with an example
how this is going to be calculated. The full mechanism is split
into two components: this driver and the thermal framework.
When the IRQ (which was programmed for e.g. 'panic temp' value)
is triggered then thermal fwk is kicked. In that the 'get_temp'
will be called.
What this change effectively does is:
- program the IRQ firing temp level higher (based on dt example)
- provide the temp value lower to thermal fwk
then you don't touch other stuff which is generic in this
thermal scope (like trip points).
It's kind of tricky to grasp and I would suggest to describe it
somewhere. These are popular chips and many boards use them,
so many engineers might miss this bit.
>
> Signed-off-by: Haoning Cheng <Haoning.CHENG@cn.bosch.com>
> ---
> drivers/thermal/imx_thermal.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 38c993d1bcb3..0a443e608957 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -207,6 +207,7 @@ struct imx_thermal_data {
> struct regmap *tempmon;
> u32 c1, c2; /* See formula in imx_init_calib() */
> int temp_max;
> + s32 calibration_offset;
> int alarm_temp;
> int last_temp;
> bool irq_enabled;
> @@ -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,
> @@ -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);
This doesn't belong to the $subject. Needs extra patch.
> + } else {
> alarm_value = (data->c2 - alarm_temp) / data->c1;
> + }
>
> regmap_write(map, soc_data->high_alarm_ctrl + REG_CLR,
> soc_data->high_alarm_mask);
> @@ -277,6 +282,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) {
> @@ -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);
> +
No bounds check here or debug message?
Playing with raw registers which handle thermal safety net it's kind of
risky IMO (a typo by one digit and the board dies).
Done once in the setup code won't harm performance and something which
goes to the DT schema is quite stable to reuse.
Regards,
Lukasz
^ permalink raw reply [flat|nested] 6+ messages in thread