| Message ID | 20230328031037.1361048-1-wenst@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | thermal/drivers/mediatek/lvts_thermal: only update registered thermal zones | expand |
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; > } > > /*
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 >
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 ?
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; } /*
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(-)