Re: [PATCH v1 13/14] thermal: trip: Replace thermal_zone_get_num_trips()

Rafael J. Wysocki
2024-06-17T19:02:52Z

On Mon, Jun 17, 2024 at 8:55 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Hi Niklas,
>
> On Mon, Jun 17, 2024 at 8:39 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.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@intel.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@intel.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!