Re: [PATCH v4 2/5] thermal/drivers/loongson2: Move thermal_ops to chip_data for SoC scalability

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

Pre-existing issues:
- [High] The driver misuses the `clamp()` macro when setting temperature th=
resholds, failing to bound the requested trip values to the hardware's supp=
orted range `[-40, 125]`.
--

--- Patch [2]: [PATCH v4 2/5] thermal/drivers/loongson2: Move thermal_ops t=
o chip_data for SoC scalability ---
commit 1f38b1d9f1db90805ef5765872a4c9de0db11e9c
Author: Binbin Zhou <[email protected]>

thermal/drivers/loongson2: Move thermal_ops to chip_data for SoC scalability
   =20
In preparation for supporting additional Loongson SoCs, this commit
relocates the thermal zone device operations pointer from the probe
routine to the per-SoC chip_data structure. This eliminates the need to
add conditional branches in probe() when a new SoC is introduced.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongs=
on2_thermal.c
> index 5f47fa51fb907..1a8ca2cd06f1b 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c
[ ... ]
> @@ -160,7 +156,7 @@ static int loongson2_thermal_probe(struct platform_de=
vice *pdev)
>  	loongson2_thermal_set(data, 0, 0, false);
> =20

[Severity: High]
This is a pre-existing issue, but does loongson2_thermal_set() misuse the
clamp macro when setting temperature thresholds?

Looking at drivers/thermal/loongson2_thermal.c:loongson2_thermal_set(), the
arguments to clamp() appear to be in the wrong order:

	/* Set low temperature threshold */
	loongson2_set_ctrl_regs(data, clamp(-40, low, high), true, enable);

	/* Set high temperature threshold */
	loongson2_set_ctrl_regs(data, clamp(125, low, high), false, enable);

Because the clamp macro arguments are swapped (using clamp(-40, low, high)
instead of clamp(low, -40, 125) and clamp(high, -40, 125)), values like a
high trip point > 155C are not bounded to 125.

If root sets a thermal trip point > 155C via sysfs, adding HECTO (100) to
156+ results in a value > 255.

When this is written to the 16-bit hardware register, can it overflow into
bit 8 (the enable bit), overwriting the control state and truncating the
threshold to an incorrect value?

Could this cause immediate and continuous hardware thermal alarms (IRQ
storm), rendering the system unusable?

>  	for (i =3D 0; i <=3D LOONGSON2_MAX_SENSOR_SEL_NUM; i++) {
> -		tzd =3D devm_thermal_of_zone_register(dev, i, data, thermal_ops);
> +		tzd =3D devm_thermal_of_zone_register(dev, i, data, data->chip_data->t=
hermal_ops);
>  		if (!IS_ERR(tzd))
>  			break;

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/cover.1785829933.gi=
[email protected]?part=3D2