Re: [PATCH v5 2/2] thermal: amlogic: Add support for A9 thermal controller

On 2026/9/10 15:06, sashiko-bot@kernel.org wrote:

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

New issues:
- [Critical] The A9-specific hardware over-temperature reset mechanism is enabled unconditionally on all SoCs, causing unclocked MMIO accesses and undefined register writes that will crash existing platforms (e.g., G12A) on resume.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the thermal zone and hwmon sysfs interfaces are registered before the hardware and clocks are fully initialized, creating a race condition that leads to unclocked MMIO reads.
- [High] This problem wasn't introduced by this patch, but incorrect sign extension of a bitwise NOT operation on a 32-bit unsigned mask causes massively inflated negative trim values on 32-bit architectures, breaking thermal calibration.
- [High] This problem wasn't introduced by this patch, but disabling hardware clocks in `remove()` before the devres-managed thermal zone is unregistered introduces a use-after-disable race condition that can crash the system.
--

commit c409169ce8a3938d784d6682dd656715a5d6d606
Author: Xianwei Zhao[off-list ref]

thermal: amlogic: Add support for A9 thermal controller

Add support for the Amlogic A9 thermal controller.

The A9 thermal controller uses different calibration parameters, requires
separate core and bus clocks, and supports a hardware over-temperature
reset mechanism. It also restores the hardware reset configuration after
resume.

quoted

diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
index a0b530624b60c..e7cedb9443151 100644
--- a/drivers/thermal/amlogic_thermal.c
+++ b/drivers/thermal/amlogic_thermal.c

[ ... ]

quoted

@@ -138,6 +158,51 @@ static int amlogic_thermal_code_to_millicelsius(struct amlogic_thermal *pdata,
       return temp;
  }

+/*
+ * Calculate a temperature code from a temperature value .
+ * The unit of the temperature is degree milliCelsius.
+ */
+static u32 amlogic_thermal_millicelsius_to_code(struct amlogic_thermal *pdata, int millicelsius)
+{
+     const struct amlogic_thermal_soc_calib_data *param =
+                                     pdata->data->calibration_parameters;
+     s64 factor, uptat, uefuse;
+     u32 temp_code;
+
+     uefuse = pdata->trim_info & TSENSOR_TRIM_SIGN_MASK ?
+                          ~(pdata->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
+                          (pdata->trim_info & TSENSOR_TRIM_TEMP_MASK);

[Severity: High]
This is a pre-existing issue, but by copying this logic from
amlogic_thermal_code_to_millicelsius(), the new function inherits a sign
extension bug.

When the hardware provides a negative trim value (bit 15 set in trim_info),
the bitwise NOT operation on the 32-bit unsigned TSENSOR_TRIM_TEMP_MASK mask
evaluates to a large positive value on 32-bit systems (e.g., 0xFFFFFFFF).

When this is assigned to the s64 uefuse variable, C promotion rules
zero-extend it to 4294967295 rather than sign-extending to -1.

This breaks thermal calibration for hardware instances with negative factory
calibration offsets, rendering those devices unusable.

Could we fix this integer conversion so that the negative value is properly
sign-extended into the s64 variable?

[ ... ]

quoted

@@ -346,6 +440,17 @@ static int amlogic_thermal_probe(struct platform_device *pdev)

       devm_thermal_add_hwmon_sysfs(&pdev->dev, pdata->tzd);

[Severity: High]
This isn't a bug introduced by this patch, but registering the thermal zone
device here before the hardware clocks are fully initialized creates a race
condition.

The thermal core invokes the .get_temp callback immediately upon registration
to establish the initial temperature state.

If the driver is built as a loadable module and loaded after boot, when
bootloader-enabled clocks have already been disabled by the clock framework's
late init, the .get_temp call will attempt to read from the hardware before
its clock is prepared and enabled.

Will this result in an unclocked MMIO read and subsequent bus fault?

quoted

+     if (pdata->data->reset_temp) {
+             reset_temp = pdata->data->reset_temp;
+     } else {
+             dev_dbg(dev, "using default hardware reset temperature\n");
+             reset_temp = TSENSOR_HW_RESET_DEFAULT_TEMP;
+     }
+
+     pdata->temp_code = amlogic_thermal_millicelsius_to_code(pdata, reset_temp);
+
+     amlogic_tsensor_setup_hw_reset(pdata);
+
       ret = amlogic_thermal_enable(pdata);

[Severity: Critical]
Does unconditionally applying the A9-specific hardware reset setup here
cause issues for older non-A9 platforms?

Non-A9 HW is support too, just that this function wasn't added before. 
Here, the default value is used.

For existing SoCs where has_sysclk is false, the peripheral clock (data->clk)
isn't enabled until amlogic_thermal_enable() is called on the next line.

The existing SoCs sys clock is not control by this driver, it is enabled 
by default.

Since the clock is physically off during probe, does this regmap_update_bits()
write inside amlogic_tsensor_setup_hw_reset() trigger a bus fault?

Also, could writing to TSENSOR_CFG_REG2 cause undefined register modification
on hardware that doesn't support this over-temperature reset mechanism?

The Non-SoC has TSENSOR_CFG_REG2 regiter, the register was hidden before 
because the related functions were not supported.

[ ... ]

quoted

@@ -370,7 +477,15 @@ static int amlogic_thermal_suspend(struct device *dev)
  static int amlogic_thermal_resume(struct device *dev)
  {
       struct amlogic_thermal *data = dev_get_drvdata(dev);
+     int ret;
+
+     if (data->data->has_sysclk) {
+             ret = clk_prepare_enable(data->sysclk);
+             if (ret)
+                     return ret;
+     }

+     amlogic_tsensor_setup_hw_reset(data);
       return amlogic_thermal_enable(data);

[Severity: Critical]
Similarly, does this execute an MMIO write before the peripheral clock is
enabled during the resume path for non-A9 platforms?

On non-A9 platforms, data->clk is only enabled inside
amlogic_thermal_enable().

The sysclock is keeping openning all time for other SoC.

If this write occurs while the clock is still physically disabled during
system resume, could this cause a synchronous external abort and break power
management completely?

Non A9 clock is aways_on ,so for A9 if enable sysclock failed, it will 
be return.

[Severity: High]
This is a pre-existing issue, but is there a use-after-disable race condition
in the driver's remove path?

drivers/thermal/amlogic_thermal.c:amlogic_thermal_remove() {
     struct amlogic_thermal *data = platform_get_drvdata(pdev);

     amlogic_thermal_disable(data);
}

Disabling the hardware clocks in remove() before the devres-managed thermal
zone is cleanly unregistered could crash the system.

Because pdata->tzd was registered with devm_thermal_of_zone_register(), it
remains fully active and exposed to the thermal core until the devres cleanup
phase, which executes after remove() returns.

If the thermal core's polling timer fires, or if a user reads the temperature
sysfs attribute during this window, amlogic_thermal_get_temp() will attempt
to read from the unclocked hardware.