Re: [PATCH v6 4/7] thermal/drivers: airoha: Generalize probe function

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing NULL check for the result of `device_get_match_data()` leads to a NULL pointer dereference.

Pre-existing issues:
- [High] Interrupt handler silently drops thermal events if both High and Low threshold interrupts fire simultaneously.
- [High] Incorrect devres initialization order for IRQ causes race conditions (early NULL pointer dereference) and Use-After-Free.
- [High] Missing return value check for `of_address_to_resource()` allows usage of an uninitialized resource address.
--

(Note: The standard inline diff quoting format has been altered to a free-form
summary due to automated recitation restrictions. The original patch code is
not quoted below.)

Commit: a66125bc561d9323e2561d461f2be68491469827
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: thermal/drivers: airoha: Generalize probe function

This commit generalizes the probe function in preparation for supporting
the Airoha AN7583 SoC. It introduces a match_data struct to define specific
probe and post_probe functions, thermal ops, and pllrg protect values.

[Severity: High]
Could device_get_match_data() return NULL in airoha_thermal_probe()? 
If the driver is bound manually via sysfs by matching the driver name
without an underlying Device Tree node, the match data would be NULL.
Dereferencing soc_data->pllrg_protect immediately after without validation
would cause a NULL pointer dereference. Is there a need to check if
soc_data is valid before accessing it?

[Severity: High]
This is a pre-existing issue, but I noticed a potential flaw in how the
interrupt status is handled in en7581_thermal_irq(). If the temperature
fluctuates rapidly and both EN7581_HOFSINTSTS0 and EN7581_LOFSINTSTS0
are set simultaneously in the status register before the IRQ is serviced,
the bitwise AND yields the combination of both bits.

The switch statement fails to match this combined value and falls through to
the default case, dropping the event entirely. Would it be safer to process
these bits individually rather than using a switch statement?

[Severity: High]
This isn't a bug introduced by this patch, but does the devres initialization
order leave a window for a race condition? devm_request_threaded_irq() is
called in en7581_thermal_probe() before priv->tz is initialized via
devm_thermal_of_zone_register() in airoha_thermal_probe().

If an interrupt is already pending from the bootloader, it can fire
immediately, passing a NULL priv->tz to thermal_zone_device_update().
Furthermore, on driver unbind, devres frees the thermal zone before disabling
the IRQ, potentially leading to a use-after-free if an interrupt fires.
Could moving the IRQ request after the thermal zone registration resolve this?

[Severity: High]
This is a pre-existing issue, but what happens if of_address_to_resource()
fails in en7581_thermal_probe()? The return value is currently ignored.

If it fails, for example due to a malformed device tree,
priv->scu_adc_res.start would remain 0. Later, en7581_thermal_setup_monitor()
unconditionally uses this value to configure EN7581_TEMPADCVALIDADDR,
instructing the ADC controller to read from an invalid physical address
instead of the SCU block. Should the return value of
of_address_to_resource() be validated?

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