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 therm=
al emergencies.
- [Medium] The return values of `regmap_read()` for chip ID registers are i=
gnored, potentially leading to silent failures and miscalculation of temper=
atures.
- [Medium] Suspicious manual sign-magnitude parsing and missing sign handli=
ng for `chip_id0` calibration data.
--

--- Patch [5]: [PATCH v4 5/5] thermal/drivers/loongson2: Add support for Lo=
ongson-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/loongs=
on2_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_the=
rmal_data *data,
>  	return 0;
>  }
> =20
> +static int loongson2_2k0300_get_temp(struct thermal_zone_device *tz, int=
 *temp)
> +{
> +	struct loongson2_thermal_data *tdata =3D thermal_zone_device_priv(tz);
> +	int calib_data, calib_offset, temp_mc, raw_adc;
> +	u32 chip_id0 =3D 0, chip_id1 =3D 0;
> +
> +	writel(0xff03, tdata->ctrl_reg + LOONGSON2_THSENS_CFG_REG);
> +	raw_adc =3D FIELD_GET(LOONGSON2_THSENS_OUT_11B_MASK,
> +			    readl(tdata->ctrl_reg + LOONGSON2_THSENS_OUT_REG));
> +
> +	if (tdata->flags & LS2K0300_OLD_FUSE_FLAG) {
> +		*temp =3D 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 =3D FIELD_GET(LS2K0300_ID1_VAL_MASK, chip_id1);
> +		calib_offset =3D FIELD_GET(LS2K0300_COMP_VAL_MASK, calib_data);
> +		if (calib_data & LS2K0300_COMP_SIGN_BIT)
> +			calib_offset =3D -calib_offset;
> +	} else {
> +		calib_data =3D FIELD_GET(LS2K0300_ID0_VAL_MASK, chip_id0);
> +		calib_offset =3D 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 =3D (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 overh=
eat?

If the physical CPU temperature genuinely exceeds 125 degrees Celsius during
a thermal emergency, this check will incorrectly flag the valid hardware fu=
se
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 shutdow=
n.

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

[ ... ]

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