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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Global state mutation due to modifying shared match data.
- [High] Uninitialized stack variables used for critical temperature calculation due to ignored regmap_read() errors.
- [Medium] Off-by-one error in ADC bitmask definition results in an 11-bit mask for a 10-bit ADC.
- [Medium] Speculative: ADC configuration write is immediately followed by output read without hardware delay.
- [Low] Inefficient repeated fuse parsing and dead store of the calibration offset.
--

--- Patch [4]: [PATCH v2 4/4] thermal/drivers/loongson2: Add support for Loongson-2K0300 ---
commit 7a3c65aba38521ce4ea3dd10556d3932b4554449
Author: Binbin Zhou <[email protected]>

thermal/drivers/loongson2: Add support for Loongson-2K0300
    
Add support for the thermal sensor found on the Loongson-2K0300 SoC.
The Loongson-2K0300 thermal sensor uses a 10-bit ADC and requires
per-chip calibration via syscon.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index d7d221c3136d4..e8d11e592a1f2 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c
[ ... ]
> @@ -23,28 +28,48 @@
>  #define LOONGSON2_THSENS_CTRL_LOW_REG	0x8
>  #define LOONGSON2_THSENS_STATUS_REG	0x10
>  #define LOONGSON2_THSENS_OUT_REG	0x14
> +#define LOONGSON2_THSENS_CFG_REG	0x18
>  
>  #define LOONGSON2_THSENS_INT_LO		BIT(0)
>  #define LOONGSON2_THSENS_INT_HIGH	BIT(1)
>  #define LOONGSON2_THSENS_INT_EN		(LOONGSON2_THSENS_INT_LO | \
>  					 LOONGSON2_THSENS_INT_HIGH)
> -#define LOONGSON2_THSENS_OUT_MASK	0xFF
> +#define LOONGSON2_THSENS_OUT_8B_MASK	0xFF
> +#define LOONGSON2_THSENS_OUT_10B_MASK	GENMASK(10, 0)

[Severity: Medium]
The commit message mentions a 10-bit ADC, but doesn't GENMASK(10, 0) define
an 11-bit mask (bits 10 down to 0)? Would GENMASK(9, 0) be the correct mask
for a 10-bit value to avoid including potentially reserved bits in the ADC
output?

[ ... ]
> @@ -71,13 +96,55 @@ 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, chip_id1;
> +
> +	writel(0xff03, tdata->ctrl_reg + LOONGSON2_THSENS_CFG_REG);
> +	raw_adc = FIELD_GET(LOONGSON2_THSENS_OUT_10B_MASK,
> +			    readl(tdata->ctrl_reg + LOONGSON2_THSENS_OUT_REG));

[Severity: Medium]
Is a hardware delay or ready-bit poll needed between configuring the ADC and
reading its output register? 

Reading the output immediately after the configuration write might return
incomplete or stale conversion data if the hardware requires time to process
the new configuration.

> +
> +	if (tdata->chip_data->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);
> +
> +	if (chip_id0 & LS2K0300_EXTERN_ID) {

[Severity: High]
Can these regmap_read() calls fail? If an I/O error or bus failure causes
regmap_read() to return an error code, chip_id0 and chip_id1 would remain
uninitialized.

Using these uninitialized stack values for bitwise mask operations in the
following checks could cause the driver to compute incorrect calibration
offsets and corrupt temperature readings.

> +		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);
> +	}
> +
> +	tdata->calib_offset = calib_offset;

[Severity: Low]
It looks like tdata->calib_offset is assigned here but never read anywhere
else in the driver.

Since the calibration data is based on fixed hardware fuses, would it be
better to perform this parsing once during driver probe and store the result,
rather than re-reading the syscon registers on every temperature poll?

> +	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);
> +		tdata->chip_data->flags |= LS2K0300_OLD_FUSE_FLAG;

[Severity: High]
Is it safe to modify the flags field of the statically defined shared match
data?

By forcibly casting away the const qualifier from device_get_match_data() in
loongson2_thermal_probe() and then modifying tdata->chip_data->flags here,
the driver alters the global state for all current and future bound instances
of this device.

If one thermal sensor encounters an invalid temperature and sets
LS2K0300_OLD_FUSE_FLAG, this flag will be permanently set for all instances
sharing this match data, incorrectly triggering the fallback formula for
healthy sensors.

> +		temp_mc = raw_adc * 569 - 394700;
> +	}
> +	*temp = temp_mc;
> +
> +	return 0;
> +}

[ ... ]
> @@ -134,7 +205,8 @@ static int loongson2_thermal_probe(struct platform_device *pdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> -	data->chip_data = device_get_match_data(dev);
> +	data->dev = dev;
> +	data->chip_data = (struct loongson2_thermal_chip_data *)device_get_match_data(dev);

[Severity: High]
This cast drops the const qualifier from the match data, which leads to the
global state modification mentioned in loongson2_2k0300_get_temp().

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