Re: [PATCH v6 2/7] thermal/drivers: airoha: fix copy paste error for sen internal

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The thermal interrupt handler ignores events if multiple interrupt status bits are set simultaneously, causing lost thermal events.
- [High] The thermal interrupt is requested before the thermal zone device is initialized, risking a NULL pointer dereference.
- [High] Ignored return values from `regmap_read()` lead to the use of uninitialized stack variables, causing garbage temperature readings or hardware corruption.
- [High] Ignored return value from `of_address_to_resource()` leads to configuring the hardware with a bogus uninitialized physical address.
- [High] The `set_trips` callback fails to disable hardware interrupts or update registers when a trip point is removed.
- [High] The driver checks the entire 32-bit EFUSE register instead of the masked calibration field, potentially misinterpreting unrelated bits as calibration data.
--

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

thermal/drivers: airoha: fix copy paste error for sen internal

In airoha_thermal_setup_monitor there is a copy paste error on configuring
the internval for temp monitor. Fix the error and use the correct mask for
the sen interval for the EN7581_TEMPMONCTL2 register.

> diff --git a/drivers/thermal/airoha_thermal.c b/drivers/thermal/airoha_thermal.c
> index 439aa011b75c7..829a7327fc403 100644
> --- a/drivers/thermal/airoha_thermal.c
> +++ b/drivers/thermal/airoha_thermal.c
> @@ -403,7 +403,7 @@ static void airoha_thermal_setup_monitor(struct airoha_thermal_priv *priv)
>  	 * sen interval is 379 * 52.715us = 19.97ms
>  	 */
>  	writel(FIELD_PREP(EN7581_FILT_INTERVAL, 1) |
> -	       FIELD_PREP(EN7581_FILT_INTERVAL, 379),
> +	       FIELD_PREP(EN7581_SEN_INTERVAL, 379),
>  	       priv->base + EN7581_TEMPMONCTL2);
>  
>  	/* AHB poll is set to 146 * 68.64 = 10.02us */

[Severity: High]
This isn't a bug introduced by this patch, but could this switch statement
miss events if multiple interrupt status bits are set simultaneously?

drivers/thermal/airoha_thermal.c:en7581_thermal_irq() {
        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;
        }
}

If the hardware asserts both EN7581_HOFSINTSTS0 and EN7581_LOFSINTSTS0
concurrently, the bitwise AND evaluates to both bits being set, which matches
neither case. Execution would fall through to default, discarding the events.

[Severity: High]
This is a pre-existing issue, but does this driver request the interrupt
before the thermal zone device is initialized?

drivers/thermal/airoha_thermal.c:en7581_thermal_probe() {
        ...
        ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
                                        en7581_thermal_irq, IRQF_ONESHOT,
                                        pdev->name, priv);
        ...
}

If the bootloader leaves the thermal interrupt enabled and asserted, the
handler could execute immediately:

drivers/thermal/airoha_thermal.c:en7581_thermal_irq() {
        ...
        if (update)
                thermal_zone_device_update(priv->tz, event);
        ...
}

Because the tz pointer is populated later in the probe sequence:

drivers/thermal/airoha_thermal.c:airoha_thermal_probe() {
        ...
        priv->tz = devm_thermal_of_zone_register(dev, 0, priv,
                                                 soc_data->thdev_ops);
        ...
}

Could this result in a NULL pointer dereference if the interrupt fires early?

[Severity: High]
This isn't a bug introduced by this patch, but are the return values of
these regmap reads intentionally ignored?

drivers/thermal/airoha_thermal.c:airoha_get_thermal_ADC() {
        ...
        regmap_field_read(priv->chip_scu_fields[AIROHA_THERMAL_DOUT_TADC],
                          &val);
        return val;
}

And also here:

drivers/thermal/airoha_thermal.c:airoha_set_thermal_mux() {
        ...
        regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);
        ...
}

If the underlying regmap read fails, the uninitialized stack variables
val and pllrg could be used directly, potentially returning invalid
temperatures or writing garbage back to the hardware.

[Severity: High]
This is a pre-existing issue, but what happens if the device tree node lacks
a valid reg property?

drivers/thermal/airoha_thermal.c:en7581_thermal_probe() {
        ...
        of_address_to_resource(chip_scu_np, 0, &priv->scu_adc_res);
        ...
}

The return value is ignored here. If the parse fails, priv->scu_adc_res.start
remains 0. The driver then configures the AHB bus master to poll from this
address:

drivers/thermal/airoha_thermal.c:en7581_thermal_setup_monitor() {
        ...
        regmap_write(priv->map, EN7581_TEMPADCVALIDADDR,
                     priv->scu_adc_res.start + EN7581_DOUT_TADC);
        ...
}

Could this asynchronous read from an uninitialized address cause a bus fault?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver handle the
removal of all trip points correctly?

drivers/thermal/airoha_thermal.c:en7581_thermal_set_trips() {
        ...
        /* Enable sensor 0 monitor after trip are set */
        if (enable_monitor)
                regmap_write(priv->map, EN7581_TEMPMONCTL0, EN7581_SENSE0_EN);
        
        return 0;
}

If the thermal core removes a trip point and enable_monitor becomes false,
there is no else branch to disable the monitor in hardware. Would the monitor
continue firing interrupts for the stale thresholds?

[Severity: High]
This is a pre-existing issue, but does this correctly check if the
calibration field is programmed?

drivers/thermal/airoha_thermal.c:en7581_thermal_setup_adc_val() {
        ...
        regmap_read(priv->map, EN7581_EFUSE_TEMP_OFFSET_REG, &efuse_calib_info);
        if (efuse_calib_info) {
                priv->default_offset = FIELD_GET(EN7581_EFUSE_TEMP_OFFSET, efuse_calib_info);
        ...
}

If the lower bits of the register contain non-zero data (outside the
EN7581_EFUSE_TEMP_OFFSET mask), but the calibration field itself is
unprogrammed (0), the condition still evaluates to true. 

Would this trap the driver in the calibrated path with a 0 offset, preventing
the use of the non-calibrated fallback parameters?

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