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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Double disable of 'sysclk' due to manual disable of devm-managed clock in remove callback.
- [High] Peripheral hardware registers are accessed via MMIO before their backing clocks are enabled.
- [High] Incorrect hardware reset temperature calculation for negative calibration trims due to flawed bitwise masking.
- [High] A9-specific hardware over-temperature reset configuration is applied unconditionally to all legacy SoCs.
- [High] The return value of `clk_prepare_enable(data->sysclk)` is ignored in the resume path.
- [High] An unvalidated device tree property can cause a divide-by-zero kernel panic during probe.
- [Medium] Possible truncation of `temp_code` during hardware reset configuration due to out-of-bounds bitwise masking.

Pre-existing issues:
- [High] The thermal zone is registered and unregistered in an unsafe order relative to hardware clock state.
--

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

thermal: amlogic: Add support for A9 thermal controller

Add support for the Amlogic A9 thermal controller, utilizing different
calibration parameters, separate core and bus clocks, and a hardware
over-temperature reset mechanism. The hardware reset configuration is
also restored after resume.

> diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
> index a0b530624b60c..c9c3514966533 100644
> --- a/drivers/thermal/amlogic_thermal.c
> +++ b/drivers/thermal/amlogic_thermal.c
[ ... ]
> @@ -138,6 +154,46 @@ 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);
> +
> +	factor = param->B + div_s64(millicelsius, 100);
> +	factor = BIT(16) * factor;
> +	factor = div_s64(factor, param->A);
> +	factor = factor - (uefuse & TSENSOR_TRIM_TEMP_MASK);

[Severity: High]
Does this bitwise mask strip the negative sign extension?

The uefuse variable can hold a negative calibration trim, but applying
TSENSOR_TRIM_TEMP_MASK here seems to destroy the negative offset, which
could cause extreme positive values and corrupt the reset temperature logic.

> +
> +	uptat = param->n * factor;
> +	uptat = div_s64(uptat, BIT(16));
> +	uptat = param->m - uptat;
> +
> +	factor = factor * 100;
> +	factor = div_s64(factor, uptat);

[Severity: High]
Is there a risk of a divide-by-zero kernel panic here?

If the 'amlogic,hw-reset-temp' device tree property is set to a value around
981,000 mC, it looks like uptat can evaluate to zero. Should we validate the
reset temperature bounds before using it as a divisor?

> +
> +	temp_code = ((factor >> 0x4) & TSENSOR_TEMP_MASK) + TSENSOR_TEMP_CAL;

[Severity: Medium]
Could this bitwise mask truncate valid temperature codes?

If temp_code evaluates to 13 bits, it would exceed the 12-bit bounds of
TSENSOR_TEMP_MASK. This might lead to a zeroed reset configuration at high
temperatures.

> +
> +	return temp_code;
> +}
[ ... ]
> @@ -323,9 +398,24 @@ static int amlogic_thermal_probe(struct platform_device *pdev)
[ ... ]
>  	pdata->tzd = devm_thermal_of_zone_register(&pdev->dev,
>  						   0,
>  						   pdata,
>  						   &amlogic_thermal_ops);

[Severity: High]
This isn't a bug introduced by this patch, but does registering the thermal
zone here expose it prematurely?

Using devm_thermal_of_zone_register() before clocks are fully enabled allows
the core to poll the device immediately, which could cause a bus hang on
unclocked hardware access. Cleanup will also happen after clocks are disabled.

>  	if (IS_ERR(pdata->tzd)) {
>  		ret = PTR_ERR(pdata->tzd);
>  		dev_err(dev, "Failed to register tsensor: %d\n", ret);
>  		return ret;
>  	}
>  
>  	devm_thermal_add_hwmon_sysfs(&pdev->dev, pdata->tzd);
>  
> +	pdata->temp_code = amlogic_thermal_millicelsius_to_code(pdata, reset_temp);
> +
> +	amlogic_tsensor_setup_hw_reset(pdata);

[Severity: High]
Does this unconditionally apply A9-specific hardware over-temperature
configuration to all legacy SoCs?

Since this function writes to TSENSOR_CFG_REG2, which appears to be
A9-specific, applying it across all platforms without checking the SoC type
might cause undefined behavior on legacy hardware.

> +
>  	ret = amlogic_thermal_enable(pdata);
[ ... ]
> @@ -355,6 +449,8 @@ static void amlogic_thermal_remove(struct platform_device *pdev)
>  {
>  	struct amlogic_thermal *data = platform_get_drvdata(pdev);
>  
> +	if (data->data->has_sysclk)
> +		clk_disable_unprepare(data->sysclk);

[Severity: High]
Can this cause a reference count underflow for sysclk?

The sysclk was acquired in amlogic_thermal_probe() using
devm_clk_get_enabled(). Manually disabling a devm-managed clock here in the
remove callback could cause a double disable when the devm cleanup runs.

>  	amlogic_thermal_disable(data);
>  }
[ ... ]
> @@ -371,6 +469,9 @@ static int amlogic_thermal_resume(struct device *dev)
>  {
>  	struct amlogic_thermal *data = dev_get_drvdata(dev);
>  
> +	amlogic_tsensor_setup_hw_reset(data);

[Severity: High]
Will this cause a bus fault on unclocked MMIO access?

amlogic_tsensor_setup_hw_reset() accesses peripheral hardware registers via
regmap, but the backing clocks haven't been enabled yet. The clock enabling
seems to happen on the next line.

> +	if (data->data->has_sysclk)
> +		clk_prepare_enable(data->sysclk);

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

Ignoring the return value could lead to unbalanced clock references or
hardware access failures if enabling the sysclk fails during resume.

>  	return amlogic_thermal_enable(data);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-a9-thermal-v1-0-d786d37cba58@amlogic.com?part=2

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