linux-kernel - Re: [PATCH v1 13/14] thermal: trip: Replace thermal_zone_get_num

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 17 Jun 2024 21:02:40 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Lukasz Luba <lukasz.luba@....com>, Daniel Lezcano <daniel.lezcano@...aro.org>, 
	Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH v1 13/14] thermal: trip: Replace thermal_zone_get_num_trips()

On Mon, Jun 17, 2024 at 8:55 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> Hi Niklas,
>
> On Mon, Jun 17, 2024 at 8:39 PM Niklas Söderlund
> <niklas.soderlund+renesas@...natech.se> wrote:
> >
> > Hi Rafael,
> >
> > Thanks for your patch.
> >
> > On 2024-06-17 20:11:30 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > >
> > > The only existing caller of thermal_zone_get_num_trips(), which is
> > > rcar_gen3_thermal_probe(), uses this function to check whether or
> > > not the number of trips in the given thermal zone is nonzero.
> > >
> > > Because it really only needs to know whether or not the given
> > > thermal zone is tripless, replace thermal_zone_get_num_trips() with
> > > thermal_zone_is_tripless() that can tell rcar_gen3_thermal_probe()
> > > exactly what it needs to know and make it call that function.
> > >
> > > No intentional functional impact.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > ---
> > >  drivers/thermal/renesas/rcar_gen3_thermal.c |    3 +--
> > >  drivers/thermal/thermal_trip.c              |    6 +++---
> > >  include/linux/thermal.h                     |    2 +-
> > >  3 files changed, 5 insertions(+), 6 deletions(-)
> > >
> > > Index: linux-pm/drivers/thermal/renesas/rcar_gen3_thermal.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/thermal/renesas/rcar_gen3_thermal.c
> > > +++ linux-pm/drivers/thermal/renesas/rcar_gen3_thermal.c
> > > @@ -563,8 +563,7 @@ static int rcar_gen3_thermal_probe(struc
> > >               if (ret)
> > >                       goto error_unregister;
> > >
> > > -             ret = thermal_zone_get_num_trips(tsc->zone);
> > > -             if (ret < 0)
> > > +             if (thermal_zone_is_tripless(tsc->zone))
> >
> > There are two issues with this change.
> >
> > 1. The original code only jumped to error_unregister if there where a
> >    negative number of trip points, presumably to allow for an error to
> >    be returned when reading the number of trip points.
> >
> >    If an negative error was found it was stored in ret, and this was
> >    then returned from the probe function after jumping to
> >    error_unregister. This change jumps to the error out path, but do not
> >    fail probe.
> >
> >    However it is valid to complete probe without any trip points found.
> >    So failing probe on thermal_zone_is_tripless() is not desired.
> >
> > 2. The value returned from thermal_zone_get_num_trips() and stored in
> >    ret is used in a dev_info() below, logging how many trip points (if
> >    any) where found.
> >
> >    With this change that is no longer true and it will always log 0 trip
> >    points found.
>
> You are right, I've overlooked the above.
>
> > As there is no long (if there ever where) a reason to check for errors
> > when reading the number of trip points, and no real use to log the
> > number of trip points found maybe a modified patch can do what you want
> > (not tested).
> >
> > - ret = thermal_zone_get_num_trips(tsc->zone);
> > - if (ret < 0)
> > -    goto error_unregister;
> > -
> > - dev_info(dev, "Sensor %u: Loaded %d trip points\n", i, ret);
> > + dev_info(dev, "Sensor %u: Loaded %s trip points\n", i,
> > +       thermal_zone_is_tripless(tsc->zone) ? "with" : "without");
> >
> > What do you think?
>
> I would rather first update the driver to stop failing when the zone
> is tripless, in a separate patch.

Well, it actually never fails when the zone is tripless because
thermal_zone_get_num_trips() returns a non-negative value.

So something like the above patch, but using
thermal_zone_get_num_trips() or maybe just

- ret = thermal_zone_get_num_trips(tsc->zone);
- if (ret < 0)
-    goto error_unregister;
-
- dev_info(dev, "Sensor %u: Loaded %d trip points\n", i, ret);
+ dev_info(dev, "Sensor %u: Loaded", i);

because the number of trips (and whether or not there are any) can be
checked via sysfs.

> Fortunately, the $subject patch is not really needed in the series, so
> please regard it as withdrawn for now and we can get back to this
> later.

But the above still holds.

Thanks!