Re: [PATCH v8 3/7] thermal/drivers/loongson2: Fix Loongson-2K2000 trip point programming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] thermal: loongson2: torn gate write order in loongson2_2k2000_set_trips_regs()
- [Low] thermal: loongson2: 16-bit gate in 8-bit FIELD_PREP() in set_trips_regs()
--

commit 8811a8d93840088ed85385aa42b6210e17fd97b7
Author: Binbin Zhou <zhoubinbin@loongson.cn>

thermal/drivers/loongson2: Fix Loongson-2K2000 trip point programming

This commit introduces a per-SoC set_trips_regs callback in
drivers/thermal/loongson2_thermal.c and implements trip point programming for
the Loongson-2K2000 using its 16-bit gate formula and split control/HI_GATE
registers.

> diff --git a/drivers/thermal/loongson2_thermal.c b/drivers/thermal/loongson2_thermal.c
> index 9256cd3663e60..8be0c6a29efec 100644
> --- a/drivers/thermal/loongson2_thermal.c
> +++ b/drivers/thermal/loongson2_thermal.c

[ ... ]

> @@ -46,52 +68,77 @@ struct loongson2_thermal_data {

[ ... ]

> +static void loongson2_2k2000_set_trips_regs(struct loongson2_thermal_data *data,
> +					    int temp, bool low, bool enable)
> +{
> +	unsigned int sel = data->chip_data->thermal_sensor_sel;
> +	int hi_reg, ctrl_reg, val, gate;
> +
> +	gate = (temp + LS2K2000_TEMP_OFFSET) * LS2K2000_TEMP_DIV / LS2K2000_TEMP_MUL;
> +
> +	/* Low byte of the gate value goes to the control register */
> +	ctrl_reg = low ? LOONGSON2_THSENS_CTRL_LOW_REG : LOONGSON2_THSENS_CTRL_HI_REG;
> +	val = FIELD_PREP(LOONGSON2_THSENS_CTRL_GATE_MASK, gate) |
> +	      FIELD_PREP(LOONGSON2_THSENS_CTRL_INTEN, enable) |
> +	      FIELD_PREP(LOONGSON2_THSENS_CTRL_SEL_MASK, sel);

[Severity: Low]
As reported by Sashiko on v7 [1], in loongson2_2k2000_set_trips_regs(), can
passing the unmasked 16-bit gate value into
FIELD_PREP(LOONGSON2_THSENS_CTRL_GATE_MASK, gate) trigger a compile-time
BUILD_BUG_ON_MSG failure?

Across the clamped temperature range [-40, 125] C, gate is in the range
[5413, 8709] (0x1525 to 0x2205), so bits 15:8 are always non-zero and never
fit within LOONGSON2_THSENS_CTRL_GATE_MASK (GENMASK(7, 0)).

When loongson2_thermal_probe() calls loongson2_thermal_set(data, 0, 0, false)
with a constant temperature of 0, interprocedural constant propagation or LTO
can resolve loongson2_2k2000_set_trips_regs() with gate = 0x1844 at compile
time, tripping the field overflow check in __BF_FIELD_CHECK_MASK().

Should the low byte be extracted first (for example via
FIELD_GET(GENMASK(7, 0), gate) or lower_8_bits(gate)), matching how the high
byte is extracted with FIELD_GET(GENMASK(15, 8), gate) below?

> +	writew(val, data->ctrl_reg + ctrl_reg + sel * 2);
> +
> +	/* High byte of the gate value goes to the low byte of the HI_GATE register */
> +	hi_reg = low ? LS2K2000_THSENS_HI_GATE_LOW_REG : LS2K2000_THSENS_HI_GATE_HI_REG;
> +	val = FIELD_GET(GENMASK(15, 8), gate);
> +	writeb(val, data->ctrl_reg + hi_reg + sel);
> +}

[Severity: Medium]
In loongson2_2k2000_set_trips_regs(), could writing the control register
before the HI_GATE register expose a torn 16-bit threshold while the
interrupt is enabled and latch a spurious thermal interrupt?

During loongson2_thermal_probe(), loongson2_thermal_set() first initializes
both thresholds to 0 C (gate = 0x1844) with enable = false:

loongson2_thermal_probe()
  loongson2_thermal_set(data, 0, 0, false)
    loongson2_2k2000_set_trips_regs()

which leaves 0x18 in LS2K2000_THSENS_HI_GATE_HI_REG and
LS2K2000_THSENS_HI_GATE_LOW_REG.

When devm_thermal_of_zone_register() then programs the initial trip window
via loongson2_thermal_set_trips() -> loongson2_thermal_set() with
enable = true (for example, the 40 C trip point where gate = 0x1b65),
writew() writes the new low byte (0x65) together with
LOONGSON2_THSENS_CTRL_INTEN = 1 while LS2K2000_THSENS_HI_GATE_HI_REG still
holds 0x18. Until writeb() updates HI_GATE_HI_REG to 0x1b, the hardware
comparator is active with a composite threshold of 0x1865 (around 1.6 C),
which is already exceeded at normal operating temperatures. A similar
intermediate {old_hi, new_lo} window occurs during runtime trip updates
whenever the threshold crosses a 256-count boundary with INTEN enabled.

Would it be safer to disable INTEN (or write HI_GATE before enabling INTEN in
the control register) so the hardware comparator never evaluates a half-updated
16-bit gate?

[1] https://lore.kernel.org/r/1375bd12403bffc496261953772a45833f9b5a3e.1791512067.git.zhoubinbin@loongson.cn

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