Re: [PATCH v6 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver fails to update or disable hardware trip point thresholds when the thermal framework clears the boundaries, causing continuous spurious interrupts.
- [High] The thermal IRQ handler silently drops valid thermal events if both high and low interrupt status bits are asserted simultaneously.
--

commit c9ebb221cc728a9aaa865333e7d5b41b485e09e1
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 */
>  		writel(TEMP_TO_RAW(priv, low) >> 4,
>  		       priv->base + EN7581_TEMPOFFSETL);
>  
>  		enable_monitor = true;
>  	}
>  
>  	/* Enable sensor 0 monitor after trip are set */
>  	if (enable_monitor)
>  		writel(EN7581_SENSE0_EN, priv->base + EN7581_TEMPMONCTL0);
>
> 	return 0;
> }

[Severity: High]
This is a pre-existing issue, but I noticed the hardware monitor thresholds
aren't disabled when the boundaries are cleared.

If the thermal framework passes INT_MAX for high and -INT_MAX for low
to clear active trip points, enable_monitor will be false, but there is no
code to write 0 to disable the hardware monitor.

Could this lead to spurious interrupts since the stale thresholds remain active?


Looking at airoha_thermal_irq() in the same file:

static irqreturn_t airoha_thermal_irq(int irq, void *data)
{
	...
	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;
	}

[Severity: High]
This isn't a bug introduced by this patch, but does this switch statement
silently drop events if both high and low interrupt status bits are asserted
simultaneously?

If status & (EN7581_HOFSINTSTS0 | EN7581_LOFSINTSTS0) results in both bits 
being set, it will fall through to the default case and ignore the events.

Since the IRQ handler is a threaded IRQ, could scheduling delays allow both
bits to be latched before the thread executes and cause us to lose thermal
notifications?

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