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]

Message-ID: <20240617183949.GO382677@ragnatech.se>
Date: Mon, 17 Jun 2024 20:39:49 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: 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()

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.

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?

>  			goto error_unregister;
>  
>  		dev_info(dev, "Sensor %u: Loaded %d trip points\n", i, ret);
> Index: linux-pm/drivers/thermal/thermal_trip.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_trip.c
> +++ linux-pm/drivers/thermal/thermal_trip.c
> @@ -55,11 +55,11 @@ int thermal_zone_for_each_trip(struct th
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_for_each_trip);
>  
> -int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
> +bool thermal_zone_is_tripless(struct thermal_zone_device *tz)
>  {
> -	return tz->num_trips;
> +	return tz->num_trips == 0;
>  }
> -EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
> +EXPORT_SYMBOL_GPL(thermal_zone_is_tripless);
>  
>  /**
>   * thermal_zone_set_trips - Computes the next trip points for the driver
> Index: linux-pm/include/linux/thermal.h
> ===================================================================
> --- linux-pm.orig/include/linux/thermal.h
> +++ linux-pm/include/linux/thermal.h
> @@ -210,7 +210,7 @@ int for_each_thermal_trip(struct thermal
>  int thermal_zone_for_each_trip(struct thermal_zone_device *tz,
>  			       int (*cb)(struct thermal_trip *, void *),
>  			       void *data);
> -int thermal_zone_get_num_trips(struct thermal_zone_device *tz);
> +bool thermal_zone_is_tripless(struct thermal_zone_device *tz);
>  void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
>  				struct thermal_trip *trip, int temp);
>  
> 
> 
> 

-- 
Kind Regards,
Niklas Söderlund