Re: [PATCH v5 3/7] thermal/drivers: airoha: Convert to regmap API

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Simultaneous temperature trip interrupts are dropped instead of being handled.
- [High] The IRQ is requested before the thermal zone is registered, allowing a potential NULL pointer dereference in the IRQ handler.
- [High] Non-atomic read-modify-restore sequence on a shared syscon regmap without external locking.
--

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

thermal/drivers: airoha: Convert to regmap API

In preparation for support of Airoha AN7583, convert the driver to
regmap API. This is needed as Airoha AN7583 will be based on syscon
regmap.

> diff --git a/drivers/thermal/airoha_thermal.c b/drivers/thermal/airoha_thermal.c
> index 829a7327fc403..b63893a8997a3 100644
> --- a/drivers/thermal/airoha_thermal.c
> +++ b/drivers/thermal/airoha_thermal.c

[ ... ]

> @@ -300,9 +300,9 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
>  	struct airoha_thermal_priv *priv = data;
>  	enum thermal_notify_event event;
>  	bool update = false;
> -	u32 status;
> +	u32 status = 0;
>  
> -	status = readl(priv->base + EN7581_TEMPMONINTSTS);
> +	regmap_read(priv->map, EN7581_TEMPMONINTSTS, &status);
>  	switch (status & (EN7581_HOFSINTSTS0 | EN7581_LOFSINTSTS0)) {
>  	case EN7581_HOFSINTSTS0:
>  		event = THERMAL_TRIP_VIOLATED;

[Severity: High]
This is a pre-existing issue, but what happens if both high and low
temperature interrupt status bits are set simultaneously?

If both EN7581_HOFSINTSTS0 and EN7581_LOFSINTSTS0 are set, the bitwise AND
will yield a combination of both bits. Since there are only case statements
for the individual bits, the code will fall through to the default case.

The status will be written back to clear the interrupts, but the events
will be silently dropped without notifying the thermal core.

Would it be safer to replace this switch statement with independent if
checks for each interrupt flag?

> @@ -329,18 +329,19 @@ static irqreturn_t airoha_thermal_irq(int irq, void *data)
>  static void airoha_thermal_setup_adc_val(struct device *dev,
>  					 struct airoha_thermal_priv *priv)
>  {
> -	u32 efuse_calib_info, cpu_sensor;
> +	u32 efuse_calib_info = 0;
> +	u32 cpu_sensor = 0;
>  
>  	/* Setup thermal sensor to ADC mode and setup the mux to DIODE1 */
>  	airoha_init_thermal_ADC_mode(priv);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
chip_scu regmap sequence to race with other drivers?

Looking at airoha_init_thermal_ADC_mode(), it performs a read-modify-restore
sequence on a shared syscon regmap:

static void airoha_init_thermal_ADC_mode(struct airoha_thermal_priv *priv)
{
    ...
    regmap_read(priv->chip_scu, EN7581_PLLRG_PROTECT, &pllrg);
    regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT,
                 EN7581_SCU_THERMAL_PROTECT_KEY);
    ...
    regmap_write(priv->chip_scu, EN7581_PLLRG_PROTECT, pllrg);
}

Because the internal regmap lock is only held during individual register
accesses, could another driver (like a clock or pinctrl driver) accessing
the SCU concurrently cause the protect key to be incorrectly overwritten or
prematurely removed?

[ ... ]

> @@ -462,8 +475,8 @@ static int airoha_thermal_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, priv);
>  
>  	/* Enable LOW and HIGH interrupt */
> -	writel(EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0,
> -	       priv->base + EN7581_TEMPMONINT);
> +	regmap_write(priv->map, EN7581_TEMPMONINT,
> +		     EN7581_HOFSINTEN0 | EN7581_LOFSINTEN0);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but could we encounter a NULL pointer
dereference if an interrupt fires before the thermal zone is registered?

Looking earlier in 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);

If an interrupt is already pending (for example, left active by the
bootloader) when the IRQ is requested, the handler will run immediately.

If an event is detected, airoha_thermal_irq() will call
thermal_zone_device_update(priv->tz, event). Since priv->tz is still NULL
at that point, it will crash.

Should the IRQ be requested after the thermal zone registration is complete?

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