Re: [PATCH v2] thermal/drivers/rcar_thermal: fail probe when no sensors are probed

[Niklas Söderlund]

Hi Geert,

On 2026-11-04 14:14:20 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Wed, 4 Nov 2026 at 13:08, Niklas Söderlund
> <niklas.soderlund@xxxxxxxxxxxx> wrote:
> > On 2026-11-03 16:46:04 +0800, Li Ximo wrote:
> > > From: Wang Haoran <haoranwangsec@xxxxxxxxx>
> > >
> > > rcar_thermal_probe() returns success even when zero sensors were probed
> > > (the per-sensor loop simply falls through with i == 0), leaving
> > > common->head empty. rcar_thermal_suspend()/rcar_thermal_resume() then
> > > call list_first_entry(&common->head, ...) unconditionally, and
> > > list_first_entry() on an empty list does not return NULL: it returns
> > > container_of() of the list head, i.e. a wild pointer into
> > > struct rcar_thermal_common, which is dereferenced at
> > > priv->chip->needs_suspend_resume and in the MMIO accesses that follow.
> > > A malformed DT node with no MEM resource triggers this at every
> > > suspend/resume cycle.
> > >
> > > As suggested by Niklas Söderlund, fix this at the root instead of
> > > tolerating an empty list in the PM callbacks: fail probe with -ENODEV
> > > when no sensor was probed, since a successfully bound driver with zero
> > > sensors is meaningless. The error path through rcar_thermal_remove()
> > > already tolerates the empty list (it iterates with
> > > rcar_thermal_for_each_priv()).
> > >
> > > This issue was found by static analysis; we do not have R-Car hardware,
> > > so this is compile-tested only.
> > >
> > > Reported-by: Wang Haoran <haoranwangsec@xxxxxxxxx>
> > > Reported-by: Li Ximo <lixicaicai@xxxxxxxxx>
> > > Signed-off-by: Wang Haoran <haoranwangsec@xxxxxxxxx>
> > > ---
> > > v1: https://lore.kernel.org/r/20261026115018.3227372-1-lixicaicai@xxxxxxxxx
> > > v2: completely rework the fix per Niklas Söderlund's review: fail probe
> > > with -ENODEV when no sensors are probed instead of guarding
> > > suspend/resume.
> > >
> > >  drivers/thermal/renesas/rcar_thermal.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/thermal/renesas/rcar_thermal.c b/drivers/thermal/renesas/rcar_thermal.c
> > > index 9a9038671..6a1e977d8 100644
> > > --- a/drivers/thermal/renesas/rcar_thermal.c
> > > +++ b/drivers/thermal/renesas/rcar_thermal.c
> > > @@ -514,6 +514,12 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> > >                       enr_bits |= 3 << (i * 8);
> > >       }
> > >
> > > +     if (!i) {
> >
> > While this works I think it's fragile as it depends on the loop variable
> > of the last loop. If the loop is changed, or a new one added, this check
> > can break. Why not make it robust and check if any sensors where added
> > to the list containing sensors (common->head)?
> 
> Yeah, then we could move the loop variable declarations inside the
> for-statements...
> 
> > > +             dev_err(dev, "no thermal sensor probed\n");
> > > +             ret = -ENODEV;
> > > +             goto error_unregister;
> > > +     }
> > > +
> > >       if (common->base && enr_bits)
> > >               rcar_thermal_common_write(common, ENR, enr_bits);
> 
> ... if not for the next line:
> 
>     dev_info(dev, "%d sensor probed\n", i);

I like the idea of being able to move the loop variable to the 
declarations. This could be addressed with list_count_nodes(), right?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Kind Regards,
Niklas Söderlund