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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Hardware reset registers are unconditionally accessed before the peripheral clock is enabled, causing a bus fault or system hang on legacy SoCs.
- [High] The hardware over-temperature reset mechanism is unconditionally enabled for all SoCs, contradicting the commit message and potentially causing undefined behavior on unsupported platforms.
- [High] The return value of `clk_prepare_enable()` for `sysclk` is ignored in `amlogic_thermal_resume()`, potentially leading to unclocked hardware accesses and clock state leaks.
- [High] The fallback logic for a zero divisor (`!uptat`) in temperature calculation produces a code that overflows the 12-bit hardware register mask, silently setting the reboot threshold to 0.

Pre-existing issues:
- [High] The calculation of `uefuse` incorrectly zero-extends a 32-bit bitwise NOT result on 32-bit platforms, turning a negative calibration offset into a massive positive value.
- [High] The thermal zone and hwmon sysfs interfaces are exposed to userspace and the thermal core before hardware initialization completes, allowing unclocked register accesses.
--

--- Patch [2]: [PATCH v3 2/2] thermal: amlogic: Add support for A9 thermal controller ---
commit a95f34d366c7ca1c65ce5aec6e86688c00a990a8
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

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, and restore the
hardware reset configuration after resume.

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

[ ... ]

> @@ -138,6 +157,49 @@ 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 that was copied into this new function, but does
this calculation correctly handle negative calibration offsets on 32-bit
platforms?

The expression ~(pdata->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 evaluates to a
32-bit unsigned integer since TSENSOR_TRIM_TEMP_MASK is generated by GENMASK.
When assigned to the s64 uefuse variable, it is zero-extended instead of
sign-extended, turning a negative offset into a massive positive value.

> +
> +	factor = param->B + div_s64(millicelsius, 100);
> +	factor = BIT(16) * factor;
> +	factor = div_s64(factor, param->A);
> +	factor = factor - uefuse;
> +
> +	uptat = param->n * factor;
> +	uptat = div_s64(uptat, BIT(16));
> +	uptat = param->m - uptat;
> +
> +	factor = factor * 100;
> +	if (!uptat)
> +		factor = TSENSOR_TEMP_MASK << 0x4;
> +	else
> +		factor = div_s64(factor, uptat);
> +
> +	temp_code = ((factor >> 0x4) & TSENSOR_TEMP_MASK) + TSENSOR_TEMP_CAL;

[Severity: High]
If the division-by-zero fallback (!uptat) triggers, factor is set to 0xFFF0.
Does this result in a temp_code of 0x1000 here, which exceeds the 12-bit
TSENSOR_TEMP_MASK?

Later in amlogic_tsensor_setup_hw_reset(), this 0x1000 is shifted left by 4 to
produce 0x10000. When written to TSENSOR_CFG_REG2 with the GENMASK(15, 4) mask,
it truncates to 0. Could this accidentally set the hardware reboot threshold to
0 and cause an unexpected reboot?

> +
> +	return temp_code;
> +}
> +
> +static void amlogic_tsensor_setup_hw_reset(struct amlogic_thermal *data)
> +{
> +	regmap_update_bits(data->regmap, TSENSOR_CFG_REG2, TSENSOR_CFG_REG2_REBOOT_CODE,
> +			   data->temp_code << 0x4);
> +	regmap_update_bits(data->regmap, TSENSOR_CFG_REG2,
> +			   TSENSOR_CFG_REG2_ENABLE, TSENSOR_CFG_REG2_ENABLE);
> +}

[ ... ]

> @@ -346,6 +437,17 @@ static int amlogic_thermal_probe(struct platform_device *pdev)
>  
>  	devm_thermal_add_hwmon_sysfs(&pdev->dev, pdata->tzd);

[Severity: High]
This is a pre-existing issue, but does registering the thermal zone and hwmon
sysfs interfaces here expose them to userspace before hardware initialization
completes?

Since devm_thermal_of_zone_register() and devm_thermal_add_hwmon_sysfs() are
called before amlogic_thermal_enable() starts the device clock, a thermal
core or sysfs read could invoke amlogic_thermal_get_temp() concurrently,
accessing TSENSOR_STAT0 while the clock is gated, which could cause a bus
hang.

> +	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;
> +	}

[Severity: High]
The commit message states the hardware over-temperature reset is an A9
thermal controller feature.

However, doesn't this unconditionally fall back to the default reset
temperature and enable the hardware reboot feature for all SoCs? Older
platforms might not support this register or the hardware reboot feature.

> +
> +	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 amlogic_tsensor_setup_hw_reset() access TSENSOR_CFG_REG2 before
amlogic_thermal_enable() starts the device clock?

On older SoCs like G12A, there is no separate sysclk, so the hardware is
completely unclocked during this MMIO write. Could this cause a bus fault
or system hang during boot?

>  	return ret;
>  }

[ ... ]

> @@ -371,6 +475,9 @@ static int amlogic_thermal_resume(struct device *dev)
>  {
>  	struct amlogic_thermal *data = dev_get_drvdata(dev);
>  
> +	if (data->data->has_sysclk)
> +		clk_prepare_enable(data->sysclk);

[Severity: High]
Should the return value of clk_prepare_enable() be checked here?

If it fails, the driver continues to access the hardware in
amlogic_tsensor_setup_hw_reset(), causing a bus hang. If it succeeds but the
subsequent amlogic_thermal_enable() fails, the clock reference count could be
left imbalanced.

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

[Severity: Critical]
Similar to the issue in amlogic_thermal_probe(), does this perform an
unclocked hardware register access during device resume on legacy platforms?

amlogic_tsensor_setup_hw_reset() accesses TSENSOR_CFG_REG2 before
amlogic_thermal_enable() starts the main device clock, which could cause a
bus hang on older SoCs that lack a separate sysclk.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-a9-thermal-v3-0-0e7abf3941e3@amlogic.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic