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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The 'old fuse' validity check uses legitimate dynamic temperature bounds, which will permanently disable sensor calibration during real thermal emergencies.
- [Medium] The return values of `regmap_read()` for chip ID registers are ignored, potentially leading to silent failures and miscalculation of temperatures.
- [Medium] Suspicious manual sign-magnitude parsing and missing sign handling for `chip_id0` calibration data.
--

--- Patch [5]: [PATCH v4 5/5] thermal/drivers/loongson2: Add support for Loongson-2K0300 ---
commit 3e3a89cf673b3132da6f690e9200a7140d2ed6ae
Author: Binbin Zhou <[email protected]>

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

This commit adds support for the thermal sensor on the Loongson-2K0300 SoC.
The sensor uses a 10-bit ADC and requires per-chip calibration offsets read
from syscon CHIP ID registers. A fallback formula and validity check are
included for older fuse versions.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index 1a8ca2cd06f1b..51b29b60372a5 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c

[ ... ]

> @@ -72,13 +96,54 @@ static int loongson2_thermal_set(struct loongson2_thermal_data *data,
>  	return 0;
>  }
>  
> +static int loongson2_2k0300_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct loongson2_thermal_data *tdata = thermal_zone_device_priv(tz);
> +	int calib_data, calib_offset, temp_mc, raw_adc;
> +	u32 chip_id0 = 0, chip_id1 = 0;
> +
> +	writel(0xff03, tdata->ctrl_reg + LOONGSON2_THSENS_CFG_REG);
> +	raw_adc = FIELD_GET(LOONGSON2_THSENS_OUT_11B_MASK,
> +			    readl(tdata->ctrl_reg + LOONGSON2_THSENS_OUT_REG));
> +
> +	if (tdata->flags & LS2K0300_OLD_FUSE_FLAG) {
> +		*temp = raw_adc * 569 - 394700;
> +		return 0;
> +	}
> +
> +	regmap_read(tdata->regmap_cfg, LS2K0300_CHIP_ID0, &chip_id0);
> +	regmap_read(tdata->regmap_cfg, LS2K0300_CHIP_ID1, &chip_id1);

[Severity: Medium]
Is it safe to ignore the return values of these regmap_read() calls?

If regmap_read() fails due to an underlying bus or configuration error, it
will not modify chip_id0 or chip_id1, leaving them at 0. This could cause
the driver to silently compute an uncalibrated temperature without reporting
the read error.

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

[Severity: Medium]
Does the chip_id0 fallback path have a missing sign bit check?

The code extracts a 12-bit value into calib_data using a 12-bit mask
(LS2K0300_ID0_VAL_MASK is GENMASK(31, 20)). It then immediately applies a
15-bit mask (LS2K0300_COMP_VAL_MASK is GENMASK(14, 0)) to that 12-bit value,
which appears redundant.

Additionally, unlike the chip_id1 branch above which explicitly checks for a
sign bit and negates the offset, this branch lacks sign handling. If the
12-bit fuse data uses a sign bit, interpreting a negative value as a large
positive integer could skew the temperature calculation significantly.

> +	}
> +
> +	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) {

[Severity: High]
Could this logic permanently disable sensor calibration during a real overheat?

If the physical CPU temperature genuinely exceeds 125 degrees Celsius during
a thermal emergency, this check will incorrectly flag the valid hardware fuse
as invalid.

It then sets the LS2K0300_OLD_FUSE_FLAG, permanently disabling calibration
for all future reads, which causes the sensor to report lower uncalibrated
temperatures exactly when accurate readings are needed to trigger a shutdown.

> +		dev_warn_once(tdata->dev, "It's an old fuse, thermal %d is not right\n", temp_mc);
> +		tdata->flags |= LS2K0300_OLD_FUSE_FLAG;
> +		temp_mc = raw_adc * 569 - 394700;
> +	}
> +	*temp = temp_mc;
> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5