Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <b3c86caf-635d-416b-af98-9e26f2a68948@linaro.org>
Date:   Mon, 4 Dec 2023 17:55:24 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/2] thermal: sysfs: Rework the handling of trip point
 updates
Content-Language: en-US
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Lukasz Luba <lukasz.luba@arm.com>
Cc:     Linux PM <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Zhang Rui <rui.zhang@intel.com>
References: <12338384.O9o76ZdvQC@kreacher> <4883151.31r3eYUQgx@kreacher>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <4883151.31r3eYUQgx@kreacher>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Hi Rafael,

On 04/12/2023 15:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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