Re: [PATCH 2/2] thermal/drivers/loongson2: Add thermal driver support for Loongson-2K0300

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect extraction mask and sign-evaluation for `chip_id0` calibration data breaks negative temperature calibration.
- [High] The loongson2_thermal_set_trips function programs invalid Celsius thresholds into the raw ADC comparator for the LS2K0300 sensor.
- [High] Dynamic 'old fuse' detection dangerously masks legitimate critical thermal events during a real thermal runaway.

Pre-existing issues:
- [High] Inverted error handling in the thermal zone registration loop passes an `ERR_PTR` to subsequent IRQ and sysfs setup.
--

--- Patch [2]: [PATCH 2/2] thermal/drivers/loongson2: Add thermal driver support for Loongson-2K0300 ---
commit 95ad3fc965f2e0fee5a82b9e68fbe20931624b5a
Author: Binbin Zhou <zhoubinbin@loongson.cn>

thermal/drivers/loongson2: Add thermal driver support for Loongson-2K0300

This commit adds support for the Loongson-2K0300 thermal sensor, introducing a
new temperature calculation formula based on a hardware version compensation
value read from a chip ID register. It also includes a fallback mechanism
for early chips with invalid fuses.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index ea4dd2fb1f473..a7eb87070aa9f 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c

[ ... ]

> +#define LS2K0300_ID0_VAL_MASK		GENMASK(31, 20)
> +#define LS2K0300_ID1_VAL_MASK		GENMASK(15, 0)
> +
> +#define LS2K0300_COMP_VAL_MASK		GENMASK(14, 0)
> +#define LS2K0300_COMP_SIGN_BIT		BIT(15)

[ ... ]

> +static int loongson2_2k0300_get_temp(struct thermal_zone_device *tz, int *temp)
> +{

[ ... ]

> +	if (chip_id0 & LS2K0300_EXTERN_ID)
> +		calib_data = FIELD_GET(LS2K0300_ID1_VAL_MASK, chip_id1);
> +	else
> +		calib_data = FIELD_GET(LS2K0300_ID0_VAL_MASK, chip_id0);
> +
> +	calib_offset = FIELD_GET(LS2K0300_COMP_VAL_MASK, calib_data);
> +	if (calib_data & LS2K0300_COMP_SIGN_BIT)
> +		calib_offset = -calib_offset;

[Severity: High]
Is this sign check mathematically possible for chip_id0?

LS2K0300_ID0_VAL_MASK extracts a 12-bit value using GENMASK(31, 20). If
calib_data is populated from chip_id0, checking against BIT(15) will always
evaluate to false. Does this mean negative calibration offsets will never be
applied for this case?

> +
> +	temp_mc = (raw_adc + calib_offset) * 570 - 394700;
> +
> +	/* For old fuse which can not read right thermal data */
> +	if (temp_mc < LS2K0300_LOWEST_VALID_TEMP || temp_mc > LS2K0300_HIGHEST_VALID_TEMP) {
> +		dev_warn_once(tdata->dev, "It's an old fuse, thermal %d is not right\n", temp_mc);
> +		temp_mc = raw_adc * 569 - 394700;
> +	}

[Severity: High]
Could this dynamic bounds check mask a legitimate thermal runaway?

If the system genuinely overheats and exceeds LS2K0300_HIGHEST_VALID_TEMP
(125 C), this check assumes an old fuse and discards the calib_offset. If
that offset was positive, discarding it will suddenly drop the reported
temperature during severe heating. Might this hide the true severity from the
OS and prevent the system from reaching the critical trip points needed for an
emergency shutdown?

> +	*temp = temp_mc;
> +
> +	return 0;
> +}

[ ... ]

> +static const struct thermal_zone_device_ops loongson2_2k0300_of_thermal_ops = {
> +	.get_temp = loongson2_2k0300_get_temp,
> +	.set_trips = loongson2_thermal_set_trips,
> +};

[Severity: High]
Does loongson2_thermal_set_trips write invalid threshold data for this new
sensor?

That function adds 100 to the Celsius threshold via loongson2_set_ctrl_regs,
which matches the older LS2K1000 scale. Since the LS2K0300 uses an 11-bit ADC
format and a completely different calculation formula, wouldn't writing
Celsius + 100 into its hardware comparator cause an immediate threshold
mismatch and a potential interrupt storm?

[ ... ]

> @@ -160,7 +219,7 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
>  	loongson2_thermal_set(data, 0, 0, false);
>  
>  	for (i = 0; i <= LOONGSON2_MAX_SENSOR_SEL_NUM; i++) {
> -		tzd = devm_thermal_of_zone_register(dev, i, data, thermal_ops);
> +		tzd = devm_thermal_of_zone_register(dev, i, data, data->chip_data->thermal_ops);
>  
>  		if (!IS_ERR(tzd))
>  			break;
>  
>  		if (PTR_ERR(tzd) != -ENODEV)
>  			continue;
>  
>  		return dev_err_probe(dev, PTR_ERR(tzd), "failed to register");
>  	}

[Severity: High]
This is a pre-existing issue, but does this error handling logic inadvertently
pass an error pointer on fatal errors?

By continuing the loop when PTR_ERR(tzd) != -ENODEV, critical errors like
-ENOMEM are ignored. If all sensors fail to register, the loop terminates
with tzd holding an error pointer. This pointer is then blindly passed to
devm_request_threaded_irq and devm_thermal_add_hwmon_sysfs right after the
loop, potentially causing a kernel oops when dereferenced.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783670011.git.zhoubinbin@loongson.cn?part=2