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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware Logic Bug: Incorrect hardware trip point thresholds programmed for Loongson-2K0300.
- [High] Logic Error: Asymmetric handling and missing sign check for chip_id0 calibration data.
- [Medium] Logic Error: Off-by-one bitmask creates an 11-bit mask for a 10-bit ADC.
- [Medium] Performance Regression: Inefficient polling of static hardware calibration fuses.
- [Medium] Missing Error Handling: Ignored regmap_read return values silently use default zero offset.
--

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

Add support for the thermal sensor found on the Loongson-2K0300 SoC.

> 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
[ ... ]
> @@ -23,18 +28,34 @@
>  #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_11B_MASK	GENMASK(10, 0)

[Severity: Medium]
The commit message states the thermal sensor uses a 10-bit ADC, but this macro
creates an 11-bit mask. Does applying this 11-bit mask risk pulling in adjacent
status bits and corrupting the raw_adc value?

> +
> +#define LS2K0300_CHIP_ID0		0x10
> +#define LS2K0300_CHIP_ID1		0x14
> +#define LS2K0300_EXTERN_ID		BIT(4)
> +#define LS2K0300_ID0_VAL_MASK		GENMASK(31, 20)

[Severity: High]
Does this 12-bit extraction mask incorrectly truncate the data if ID0 was
intended to hold 16 bits like ID1?

> +#define LS2K0300_ID1_VAL_MASK		GENMASK(15, 0)
> +
> +#define LS2K0300_COMP_VAL_MASK		GENMASK(14, 0)
> +#define LS2K0300_COMP_SIGN_BIT		BIT(15)
> +
> +#define LS2K0300_LOWEST_VALID_TEMP	(-55000)
> +#define LS2K0300_HIGHEST_VALID_TEMP	(125000)
[ ... ]
> @@ -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]
Do we need to re-read the static calibration fuses on every temperature poll?

Since chip_id0 and chip_id1 don't change at runtime, does polling them
continuously introduce unnecessary MMIO read overhead and lock contention on
the shared syscon regmap?

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

If the reads fail, chip_id0 and chip_id1 will remain 0, leading to a silent
uncalibrated calculation. If the resulting temperature falls within the -55C
to 125C range, will this silently return inaccurate temperature data without
triggering the fallback mechanism or reporting the 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: High]
When extracting calibration data from chip_id0, are we incorrectly applying a
15-bit mask to a 12-bit value and omitting the sign bit check?

If ID0 contains signed offsets similar to ID1, will negative offsets be
treated as large positive values?

> +	}
> +
[ ... ]
> @@ -113,6 +178,11 @@ static int loongson2_thermal_set_trips(struct thermal_zone_device *tz, int low,
>  	return loongson2_thermal_set(data, low/MILLI, high/MILLI, true);
>  }
>  
> +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 assigning the legacy loongson2_thermal_set_trips function to the 2K0300
ops structure program the hardware comparators with the wrong values?

loongson2_thermal_set_trips() calls loongson2_set_ctrl_regs() which uses the
legacy formula:
    reg_ctrl = ctrl_data + HECTO;

But the 2K0300 uses a different formula:
    temp = (raw_adc + calib_offset) * 570 - 394700;

Will this cause the hardware interrupts to trigger at incorrect ADC values and
break thermal protection for the new SoC?

> +};
> +

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