Re: [PATCH v5 1/7] thermal/drivers: airoha: fix copy paste error on clamp_t low temp

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver fails to clear old thresholds or disable the thermal monitor when trip points are disabled, potentially causing an interrupt storm.
- [High] The interrupt handler uses a `switch` statement on a bitmask, silently dropping events if both hot and cold interrupts are asserted simultaneously.
- [High] The thermal interrupt is requested before the thermal zone is registered, which can lead to a NULL pointer dereference.
--

commit e9247122d90d0d1f57f8075a6a171215a771b2d6
Author: Christian Marangi <ansuelsmth@gmail.com>

thermal/drivers: airoha: fix copy paste error on clamp_t low temp

In airoha_thermal_set_trips, there is a copy paste error on clamping the
value for the low trip temp point. Fix it to the correct value and actually
clamp for the low variable.

> diff --git a/drivers/thermal/airoha_thermal.c b/drivers/thermal/airoha_thermal.c
> index b9fd6bfc88e5e..439aa011b75c7 100644
> --- a/drivers/thermal/airoha_thermal.c
> +++ b/drivers/thermal/airoha_thermal.c
> @@ -273,7 +273,7 @@ static int airoha_thermal_set_trips(struct thermal_zone_device *tz, int low,
>  
>  	if (low != -INT_MAX) {
>  		/* Validate low and clamp it to a supported value */
> -		low = clamp_t(int, high, RAW_TO_TEMP(priv, 0),
> +		low = clamp_t(int, low, RAW_TO_TEMP(priv, 0),
>  			      RAW_TO_TEMP(priv, FIELD_MAX(EN7581_DOUT_TADC_MASK)));
>  
>  		/* We offset the low temp of 1°C to trigger correct event */

[Severity: High]
This is a pre-existing issue, but does this function properly handle the case
where trip points are disabled?

If a trip point boundary is disabled (e.g., passing INT_MAX for the high trip),
its corresponding threshold register (EN7581_TEMPOFFSETH) is not updated. If
both boundaries are disabled, enable_monitor is set to false, and the monitor is
never explicitly disabled via EN7581_TEMPMONCTL0.

This leaves stale thresholds active in hardware. If the temperature crosses
these stale thresholds, the hardware will continue asserting interrupts.
Because the trip is disabled in software, the thermal core will not take
action to adjust the boundaries, which could lead to an unhandled interrupt
storm that can render the system unusable.


[Severity: High]
This is another pre-existing issue, but could the interrupt handler silently
drop events if both hot and cold interrupts assert simultaneously?

Looking at airoha_thermal_irq():

drivers/thermal/airoha_thermal.c:airoha_thermal_irq() {
	...
	status = readl(priv->base + EN7581_TEMPMONINTSTS);
	switch (status & (EN7581_HOFSINTSTS0 | EN7581_LOFSINTSTS0)) {
	case EN7581_HOFSINTSTS0:
		event = THERMAL_TRIP_VIOLATED;
		update = true;
		break;
	case EN7581_LOFSINTSTS0:
		event = THERMAL_EVENT_UNSPECIFIED;
		update = true;
		break;
	default:
		/* Should be impossible as we enable only these Interrupt */
		break;
	}

	/* Reset Interrupt */
	writel(status, priv->base + EN7581_TEMPMONINTSTS);

	if (update)
		thermal_zone_device_update(priv->tz, event);
	...
}

If the hardware asserts both the hot and cold interrupt bits simultaneously
(e.g., due to a delayed IRQ handler combined with temperature fluctuations,
or a hardware glitch), the bitmask value matches neither case.

It will fall through to default, leaving update = false, and clear both
interrupts in hardware without notifying the thermal core.


[Severity: High]
This is also a pre-existing issue, but could requesting the interrupt before
registering the thermal zone lead to a NULL pointer dereference?

Looking at airoha_thermal_probe():

drivers/thermal/airoha_thermal.c:airoha_thermal_probe() {
	...
	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
					airoha_thermal_irq, IRQF_ONESHOT,
					pdev->name, priv);
	...
	priv->tz = devm_thermal_of_zone_register(dev, 0, priv, &thdev_ops);
	...
}

The driver does not explicitly disable or clear hardware interrupts prior to
requesting the IRQ.

If a bootloader or previous kernel left the thermal monitor running and
interrupts enabled, an interrupt can fire immediately upon request. The
handler airoha_thermal_irq() will execute and call
thermal_zone_device_update(priv->tz, event).

Because priv->tz is not yet populated, this would cause a NULL pointer
dereference during initialization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520155525.22239-1-ansuelsmth@gmail.com?part=1