[PATCH] thermal/drivers/mediatek/lvts_thermal: only update registered thermal zones

[PATCH] thermal/drivers/mediatek/lvts_thermal: only update registered thermal zones

[Chen-Yu Tsai]

It's possible for some sensors or thermal zones to not be registered,
either because they are unused or not fully declared in the device tree.
Nevertheless the driver enables interrupts for all sensors. If an
interrupt happens for an not-registered sensor, the driver would end up
updating a non-existent thermal zone, which leads to a NULL pointer
dereference.

Change it so that only registered thermal zones get updated.

Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/thermal/mediatek/lvts_thermal.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index d87d3847c7d0..bf59174e18d3 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -415,9 +415,14 @@ static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
 		if (!(value & masks[i]))
 			continue;
 
+		iret = IRQ_HANDLED;
+
+		/* sensor might not exist (bogus interrupt) or not be registered */
+		if (!lvts_ctrl->sensors[i].tz)
+			continue;
+
 		thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
 					   THERMAL_TRIP_VIOLATED);
-		iret = IRQ_HANDLED;
 	}
 
 	/*
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH] thermal/drivers/mediatek/lvts_thermal: only update registered thermal zones

[Daniel Lezcano]

On 28/03/2023 05:10, Chen-Yu Tsai wrote:
> It's possible for some sensors or thermal zones to not be registered,
> either because they are unused or not fully declared in the device tree.
> Nevertheless the driver enables interrupts for all sensors. If an
> interrupt happens for an not-registered sensor, the driver would end up
> updating a non-existent thermal zone, which leads to a NULL pointer
> dereference.
> 
> Change it so that only registered thermal zones get updated.

Why not change the interrupt initialization ?

> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/thermal/mediatek/lvts_thermal.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index d87d3847c7d0..bf59174e18d3 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -415,9 +415,14 @@ static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
>   		if (!(value & masks[i]))
>   			continue;
>   
> +		iret = IRQ_HANDLED;
> +
> +		/* sensor might not exist (bogus interrupt) or not be registered */
> +		if (!lvts_ctrl->sensors[i].tz)
> +			continue;
> +
>   		thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
>   					   THERMAL_TRIP_VIOLATED);
> -		iret = IRQ_HANDLED;
>   	}
>   
>   	/*

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] thermal/drivers/mediatek/lvts_thermal: only update registered thermal zones

[Chen-Yu Tsai]

On Sun, Apr 2, 2023 at 4:34 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 28/03/2023 05:10, Chen-Yu Tsai wrote:
> > It's possible for some sensors or thermal zones to not be registered,
> > either because they are unused or not fully declared in the device tree.
> > Nevertheless the driver enables interrupts for all sensors. If an
> > interrupt happens for an not-registered sensor, the driver would end up
> > updating a non-existent thermal zone, which leads to a NULL pointer
> > dereference.
> >
> > Change it so that only registered thermal zones get updated.
>
> Why not change the interrupt initialization ?

I'll send another patch for that.

However I think the part in this patch should still be fixed?

ChenYu

> > Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >   drivers/thermal/mediatek/lvts_thermal.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> > index d87d3847c7d0..bf59174e18d3 100644
> > --- a/drivers/thermal/mediatek/lvts_thermal.c
> > +++ b/drivers/thermal/mediatek/lvts_thermal.c
> > @@ -415,9 +415,14 @@ static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
> >               if (!(value & masks[i]))
> >                       continue;
> >
> > +             iret = IRQ_HANDLED;
> > +
> > +             /* sensor might not exist (bogus interrupt) or not be registered */
> > +             if (!lvts_ctrl->sensors[i].tz)
> > +                     continue;
> > +
> >               thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
> >                                          THERMAL_TRIP_VIOLATED);
> > -             iret = IRQ_HANDLED;
> >       }
> >
> >       /*
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>

Re: [PATCH] thermal/drivers/mediatek/lvts_thermal: only update registered thermal zones

[Daniel Lezcano]

On 07/04/2023 10:45, Chen-Yu Tsai wrote:
> On Sun, Apr 2, 2023 at 4:34 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 28/03/2023 05:10, Chen-Yu Tsai wrote:
>>> It's possible for some sensors or thermal zones to not be registered,
>>> either because they are unused or not fully declared in the device tree.
>>> Nevertheless the driver enables interrupts for all sensors. If an
>>> interrupt happens for an not-registered sensor, the driver would end up
>>> updating a non-existent thermal zone, which leads to a NULL pointer
>>> dereference.
>>>
>>> Change it so that only registered thermal zones get updated.
>>
>> Why not change the interrupt initialization ?
> 
> I'll send another patch for that.
> 
> However I think the part in this patch should still be fixed?

If you are receiving an unexpected interrupt, there is something wrong 
with the initialization. If only the monitored thermal zones have their 
interrupt enabled, then you should never enter in the interrupt handler 
for a thermal zone which is not monitored, or did I miss something ?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog