Re: [PATCH v6 7/7] thermal/drivers/loongson2: Add Loongson-2K0300 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] thermal/loongson2: hardware race in ADC initialization triggers false fallback
- [Medium] thermal/loongson2: missing sign bit check for ID0 calibration data
--

commit 0cea5de8d422086175b271d663470bc3da490430
Author: Binbin Zhou <zhoubinbin@loongson.cn>

thermal/drivers/loongson2: Add Loongson-2K0300 support
    
This commit adds support for the Loongson-2K0300 thermal sensor by reading
calibration offsets from syscon, initializing the sensor, and calculating
the temperature. It also includes logic to detect early fuse revisions and
fall back to an older calibration formula if the initial reading is out of
bounds.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index 17aa1d80a2e67..a53d70b824f32 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c
[ ... ]
> +static int ls2k0300_thermal_read_calib(struct loongson2_thermal_data *data)
> +{
> +	u32 chip_id0, chip_id1;
> +	int calib_data, calib_offset, ret;
> +
> +	ret = regmap_read(data->regmap_cfg, LS2K0300_CHIP_ID0, &chip_id0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap_cfg, LS2K0300_CHIP_ID1, &chip_id1);
> +	if (ret)
> +		return ret;
> +
> +	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]
Is there a missing sign bit check for the chip_id0 branch?

The chip_id1 branch explicitly checks LS2K0300_COMP_SIGN_BIT to negate the
offset, but the chip_id0 branch does not. Furthermore, the extracted 
LS2K0300_ID0_VAL_MASK is 12 bits, but it is masked against 
LS2K0300_COMP_VAL_MASK which is 15 bits. 

Could negative calibration offsets provided via chip_id0 be incorrectly 
treated as large positive offsets, leading to completely wrong temperature 
readings?

> +
> +	data->calib_offset = calib_offset;
> +	return 0;
> +}
> +
> +static int ls2k0300_thermal_init_extend_resource(struct platform_device *pdev,
> +						 struct loongson2_thermal_data *data)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	data->regmap_cfg =
> +		syscon_regmap_lookup_by_phandle(dev->of_node, "loongson,chipid");
> +	if (IS_ERR(data->regmap_cfg))
> +		return PTR_ERR(data->regmap_cfg);
> +
> +	ret = ls2k0300_thermal_read_calib(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to read calibration data\n");
> +
> +	writel(LS2K0300_THSENS_INIT_MASK, data->ctrl_reg + LS2K0300_THSENS_CFG_REG);
> +
> +	ls2k0300_thermal_detect_old_fuse(dev, data);

[Severity: Medium]
Could this trigger a false fallback to the old fuse formula due to a hardware
initialization race?

Here, the sensor is initialized by writing LS2K0300_THSENS_INIT_MASK to the
configuration register. Immediately after, ls2k0300_thermal_detect_old_fuse() 
is called, which reads the ADC value via ls2k0300_adc_to_temp_mc() without any
readiness poll or sleep delay.

If the hardware ADC requires time to complete its first conversion after being
configured, this immediate read might yield an artificially low or invalid
temperature. This would cause the out-of-bounds check in 
ls2k0300_thermal_detect_old_fuse() to fail, permanently falling back to the 
incorrect old fuse formula.

> +
> +	return 0;
> +}

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