Re: [PATCH 2/3] thermal/drivers/sun8i: Add support for the A80 THS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional NULL pointer dereference if calibration nvmem cell is present.
- [High] Statically enabled ALARM interrupt will cause a severe interrupt storm when triggered.

Pre-existing issues:
- [High] Interrupt handler defeats spurious interrupt detection by unconditionally returning IRQ_HANDLED.
--

--- Patch [2]: [PATCH 2/3] thermal/drivers/sun8i: Add support for the A80 THS ---
commit 95ccac1aebf565b0761d7e8e612500acbb05beba
Author: Sören Hantel <fugininsane@gmail.com>

thermal/drivers/sun8i: Add support for the A80 THS

The Allwinner A80 thermal sensor is an early relative of the THS found
in the A83T and later SoCs. It shares its register block, bus gate,
4 MHz module clock and reset line with the GPADC; the THS registers
live at offset 0x40 of the shared block, with four data registers for
the four sensors (0: big cluster, 1: DRAM, 2: GPU, 3: little cluster).

> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 22674790629a7..3e7bfda621267 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c

[ ... ]

> @@ -596,6 +616,76 @@ static int sun8i_ths_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int sun9i_a80_thermal_init(struct ths_device *tmdev)
> +{
> +	int i;
> +
> +	/* clear any pending interrupt status */
> +	regmap_write(tmdev->regmap, SUN9I_THS_IS, 0xfff);
> +	/* set up the median filter, average over 8 samples */
> +	regmap_write(tmdev->regmap, SUN9I_THS_MFC, 0x5);
> +
> +	/*
> +	 * Program the protection thresholds with the values the vendor
> +	 * BSP uses (thresholds are in raw sensor units, which decrease
> +	 * with rising temperature): an alarm interrupt at ~90 degC and
> +	 * an emergency hardware shutdown at ~105 degC.
> +	 */
> +	for (i = 0; i < tmdev->chip->sensor_num; i++) {
> +		regmap_write(tmdev->regmap, SUN9I_THS_ALARM_TH(i),
> +			     (1454 << 16) | 0xfff);
> +		regmap_write(tmdev->regmap, SUN9I_THS_SHUT_TH(i),
> +			     (1231 << 16) | 0xfff);
> +	}
> +
> +	/*
> +	 * Unlike on later SoCs, the data-ready interrupt fires at the
> +	 * conversion rate (tens of kHz), so leave it disabled and let the
> +	 * thermal core poll the data registers; the interrupt line only
> +	 * serves the alarm and shutdown events, like in the vendor BSP.
> +	 */
> +	regmap_write(tmdev->regmap, SUN9I_THS_IC,
> +		     SUN9I_THS_ALARM_IRQ_EN | SUN9I_THS_SHUT_IRQ_EN);

[Severity: High]
Will this statically enabled ALARM interrupt cause an interrupt storm when
triggered?

Since the ALARM threshold is static (~90 degC) and sun8i_irq_thread() clears
the status bit but does not mask the interrupt or adjust the threshold (as
there is no set_trips callback in this driver), won't the hardware immediately
re-assert the interrupt once the IRQF_ONESHOT thread unmasks the line while
the temperature remains above the threshold?

> +	/* acquire time 0x2f, enable all four sensors */
> +	regmap_write(tmdev->regmap, SUN9I_THS_CTRL,
> +		     SUN9I_THS_CTRL_ACQ(0x2f) | SUN9I_THS_CTRL_SENSOR_EN);
> +
> +	return 0;
> +}

[ ... ]

> +static const struct ths_thermal_chip sun9i_a80_ths = {
> +	.sensor_num = 4,
> +	.has_mod_clk = true,
> +	.mod_clk_rate = 4000000,
> +	.has_bus_clk_reset = true,
> +	.scale = 688,
> +	.offset = 190000,
> +	.temp_data_base = SUN9I_THS_TEMP_DATA,
> +	.init = sun9i_a80_thermal_init,

[Severity: High]
Is it possible for the calibration nvmem cell to be present for this chip?

The sun8i_ths_calibrate() function unconditionally dereferences the .calibrate
callback if calibration data is successfully read:

drivers/thermal/sun8i_thermal.c:sun8i_ths_calibrate() {
    ...
	caldata = nvmem_cell_read(calcell, &callen);
	if (IS_ERR(caldata)) {
		ret = PTR_ERR(caldata);
		goto out;
	}

	tmdev->chip->calibrate(tmdev, caldata, callen);
    ...
}

Since the sun9i_a80_ths struct omits the .calibrate callback, if a calibration
nvmem cell is provided (e.g. via a device tree overlay), would this cause a
NULL pointer dereference during probe?

> +	.irq_ack = sun9i_a80_irq_ack,

[Severity: High]
This is a pre-existing issue, but does sun8i_irq_thread() defeat spurious
interrupt detection by unconditionally returning IRQ_HANDLED?

If the hardware triggers an interrupt but irq_ack() finds no relevant status
bits (returning an irq_bitmap of 0), the loop is skipped:

drivers/thermal/sun8i_thermal.c:sun8i_irq_thread() {
	struct ths_device *tmdev = data;
	unsigned long irq_bitmap = tmdev->chip->irq_ack(tmdev);
	int i;

	for_each_set_bit(i, &irq_bitmap, tmdev->chip->sensor_num) {
        ...
	}

	return IRQ_HANDLED;
}

Returning IRQ_HANDLED instead of IRQ_NONE when no status bits are set prevents
the core IRQ subsystem from detecting and disabling a stuck or spurious
interrupt line.

> +	.calc_temp = sun8i_ths_calc_temp,
> +};
> +
>  static const struct ths_thermal_chip sun8i_a83t_ths = {
>  	.sensor_num = 3,
>  	.scale = 705,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722203341.2917852-1-fugininsane@googlemail.com?part=2