linux-kernel - Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point updates

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: <b3c86caf-635d-416b-af98-9e26f2a68948@linaro.org>
Date:   Mon, 4 Dec 2023 17:55:24 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Lukasz Luba <lukasz.luba@....com>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point
 updates


Hi Rafael,

On 04/12/2023 15:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> Both trip_point_temp_store() and trip_point_hyst_store() use
> thermal_zone_set_trip() to update a given trip point, but none of them
> actually needs to change more than one field in struct thermal_trip
> representing it.  However, each of them effectively calls
> __thermal_zone_get_trip() twice in a row for the same trip index value,
> once directly and once via thermal_zone_set_trip(), which is not
> particularly efficient, and the way in which thermal_zone_set_trip()
> carries out the update is not particularly straightforward.
> 
> Moreover, input processing need not be done under the thermal zone lock
> in any of these functions.
> 
> Rework trip_point_temp_store() and trip_point_hyst_store() to address
> the above, move the part of thermal_zone_set_trip() that is still
> useful to a new function called thermal_zone_trip_updated() and drop
> the rest of it.
> 
> While at it, make trip_point_hyst_store() reject negative hysteresis
> values.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
> 
> v2 -> v3: No changes
> 
> v1 -> v2: Still check device_is_registered() under the zone lock
> 
> ---
>   drivers/thermal/thermal_core.h  |    2 +
>   drivers/thermal/thermal_sysfs.c |   75 ++++++++++++++++++++++++++++------------
>   drivers/thermal/thermal_trip.c  |   45 ++++--------------------
>   include/linux/thermal.h         |    4 --
>   4 files changed, 64 insertions(+), 62 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_sysfs.c
> +++ linux-pm/drivers/thermal/thermal_sysfs.c
> @@ -78,6 +78,19 @@ mode_store(struct device *dev, struct de
>   	return count;
>   }
>   
> +static int check_thermal_zone_and_trip_id(struct device *dev,
> +					  struct thermal_zone_device *tz,
> +					  int trip_id)
> +{
> +	if (!device_is_registered(dev))
> +		return -ENODEV;
> +
> +	if (trip_id < 0 || trip_id >= tz->num_trips)
> +		return -EINVAL;

I'm not sure if this check is useful. The function is called from 
trip_point_*_store() which is providing the trip id from the file name 
parsing which is in turn built from an existing trip id. There is no 
reason the trip id is going to be wrong.

[ ... ]


-- 
<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