diff mbox series

[v1,02/13] ACPI: thermal: Collapse trip devices update functions

Message ID 3534976.iIbC2pHGDl@kreacher
State New
Headers
Series thermal: ACPI: More ACPI thermal improvements and modification of thermal instances |

Commit Message

Rafael J. Wysocki Sept. 21, 2023, 5:49 p.m. UTC 
  From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In order to reduce code duplication, merge update_passive_devices() and
update_active_devices() into one function called update_trip_devices()
that will be used for updating both the passive and active trip points.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/thermal.c |   53 ++++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

Comments

Daniel Lezcano Sept. 26, 2023, 5:18 p.m. UTC | #1 
On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In order to reduce code duplication, merge update_passive_devices() and
> update_active_devices() into one function called update_trip_devices()
> that will be used for updating both the passive and active trip points.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/thermal.c |   53 ++++++++++++++++++-------------------------------
>   1 file changed, 20 insertions(+), 33 deletions(-)
> 
> Index: linux-pm/drivers/acpi/thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/thermal.c
> +++ linux-pm/drivers/acpi/thermal.c
> @@ -43,6 +43,8 @@
>   #define ACPI_THERMAL_MAX_ACTIVE		10
>   #define ACPI_THERMAL_MAX_LIMIT_STR_LEN	65
>   
> +#define ACPI_THERMAL_TRIP_PASSIVE	(-1)
> +
>   /*
>    * This exception is thrown out in two cases:
>    * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> @@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
>   		ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
>   }
>   
> -static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
> +static bool update_trip_devices(struct acpi_thermal *tz,
> +				struct acpi_thermal_trip *acpi_trip,
> +				int index, bool compare)
>   {
> -	struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
>   	struct acpi_handle_list devices;
> +	char method[] = "_PSL";
>   	acpi_status status;
>   
> +	if (index != ACPI_THERMAL_TRIP_PASSIVE) {
> +		method[1] = 'A';
> +		method[2] = 'L';
> +		method[3] = '0' + index;
> +	}

Could be index > 9 ?
Rafael J. Wysocki Sept. 26, 2023, 5:56 p.m. UTC | #2 
On Tue, Sep 26, 2023 at 7:18 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In order to reduce code duplication, merge update_passive_devices() and
> > update_active_devices() into one function called update_trip_devices()
> > that will be used for updating both the passive and active trip points.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/acpi/thermal.c |   53 ++++++++++++++++++-------------------------------
> >   1 file changed, 20 insertions(+), 33 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/thermal.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/thermal.c
> > +++ linux-pm/drivers/acpi/thermal.c
> > @@ -43,6 +43,8 @@
> >   #define ACPI_THERMAL_MAX_ACTIVE             10
> >   #define ACPI_THERMAL_MAX_LIMIT_STR_LEN      65
> >
> > +#define ACPI_THERMAL_TRIP_PASSIVE    (-1)
> > +
> >   /*
> >    * This exception is thrown out in two cases:
> >    * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> > @@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
> >               ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
> >   }
> >
> > -static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
> > +static bool update_trip_devices(struct acpi_thermal *tz,
> > +                             struct acpi_thermal_trip *acpi_trip,
> > +                             int index, bool compare)
> >   {
> > -     struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
> >       struct acpi_handle_list devices;
> > +     char method[] = "_PSL";
> >       acpi_status status;
> >
> > +     if (index != ACPI_THERMAL_TRIP_PASSIVE) {
> > +             method[1] = 'A';
> > +             method[2] = 'L';
> > +             method[3] = '0' + index;
> > +     }
>
> Could be index > 9 ?

I can add a check, but it will never be called with index > 9 anyway.
Rafael J. Wysocki Sept. 26, 2023, 6:04 p.m. UTC | #3 
On Tue, Sep 26, 2023 at 7:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Sep 26, 2023 at 7:18 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > In order to reduce code duplication, merge update_passive_devices() and
> > > update_active_devices() into one function called update_trip_devices()
> > > that will be used for updating both the passive and active trip points.
> > >
> > > No intentional functional impact.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >   drivers/acpi/thermal.c |   53 ++++++++++++++++++-------------------------------
> > >   1 file changed, 20 insertions(+), 33 deletions(-)
> > >
> > > Index: linux-pm/drivers/acpi/thermal.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/thermal.c
> > > +++ linux-pm/drivers/acpi/thermal.c
> > > @@ -43,6 +43,8 @@
> > >   #define ACPI_THERMAL_MAX_ACTIVE             10
> > >   #define ACPI_THERMAL_MAX_LIMIT_STR_LEN      65
> > >
> > > +#define ACPI_THERMAL_TRIP_PASSIVE    (-1)
> > > +
> > >   /*
> > >    * This exception is thrown out in two cases:
> > >    * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> > > @@ -202,18 +204,25 @@ static void acpi_thermal_update_passive_
> > >               ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
> > >   }
> > >
> > > -static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
> > > +static bool update_trip_devices(struct acpi_thermal *tz,
> > > +                             struct acpi_thermal_trip *acpi_trip,
> > > +                             int index, bool compare)
> > >   {
> > > -     struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
> > >       struct acpi_handle_list devices;
> > > +     char method[] = "_PSL";
> > >       acpi_status status;
> > >
> > > +     if (index != ACPI_THERMAL_TRIP_PASSIVE) {
> > > +             method[1] = 'A';
> > > +             method[2] = 'L';
> > > +             method[3] = '0' + index;
> > > +     }
> >
> > Could be index > 9 ?
>
> I can add a check, but it will never be called with index > 9 anyway.

To be more precise, update_trip_devices() is called in two places,
acpi_thermal_init_trip() and acpi_thermal_update_trip_devices().

Both of these are called either with ACPI_THERMAL_TRIP_PASSIVE passed
as index, or from a loop over indices between 0 and
ACPI_THERMAL_MAX_ACTIVE-1 inclusive.
Daniel Lezcano Sept. 26, 2023, 9:16 p.m. UTC | #4 
On 26/09/2023 20:04, Rafael J. Wysocki wrote:
> On Tue, Sep 26, 2023 at 7:56 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Tue, Sep 26, 2023 at 7:18 PM Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>>
>>> On 21/09/2023 19:49, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> In order to reduce code duplication, merge update_passive_devices() and
>>>> update_active_devices() into one function called update_trip_devices()
>>>> that will be used for updating both the passive and active trip points.
>>>>
>>>> No intentional functional impact.
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> ---
>>>>    drivers/acpi/thermal.c |   53 ++++++++++++++++++-------------------------------

[ ... ]

>>>> +     if (index != ACPI_THERMAL_TRIP_PASSIVE) {
>>>> +             method[1] = 'A';
>>>> +             method[2] = 'L';
>>>> +             method[3] = '0' + index;
>>>> +     }
>>>
>>> Could be index > 9 ?
>>
>> I can add a check, but it will never be called with index > 9 anyway.
> 
> To be more precise, update_trip_devices() is called in two places,
> acpi_thermal_init_trip() and acpi_thermal_update_trip_devices().
> 
> Both of these are called either with ACPI_THERMAL_TRIP_PASSIVE passed
> as index, or from a loop over indices between 0 and
> ACPI_THERMAL_MAX_ACTIVE-1 inclusive.

Ok, thanks for clarifying
Daniel Lezcano Sept. 26, 2023, 9:33 p.m. UTC | #5 
On 21/09/2023 19:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In order to reduce code duplication, merge update_passive_devices() and
> update_active_devices() into one function called update_trip_devices()
> that will be used for updating both the passive and active trip points.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
diff mbox series

Patch

Index: linux-pm/drivers/acpi/thermal.c
===================================================================
--- linux-pm.orig/drivers/acpi/thermal.c
+++ linux-pm/drivers/acpi/thermal.c
@@ -43,6 +43,8 @@ 
 #define ACPI_THERMAL_MAX_ACTIVE		10
 #define ACPI_THERMAL_MAX_LIMIT_STR_LEN	65
 
+#define ACPI_THERMAL_TRIP_PASSIVE	(-1)
+
 /*
  * This exception is thrown out in two cases:
  * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
@@ -202,18 +204,25 @@  static void acpi_thermal_update_passive_
 		ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
 }
 
-static bool update_passive_devices(struct acpi_thermal *tz, bool compare)
+static bool update_trip_devices(struct acpi_thermal *tz,
+				struct acpi_thermal_trip *acpi_trip,
+				int index, bool compare)
 {
-	struct acpi_thermal_trip *acpi_trip = &tz->trips.passive.trip;
 	struct acpi_handle_list devices;
+	char method[] = "_PSL";
 	acpi_status status;
 
+	if (index != ACPI_THERMAL_TRIP_PASSIVE) {
+		method[1] = 'A';
+		method[2] = 'L';
+		method[3] = '0' + index;
+	}
+
 	memset(&devices, 0, sizeof(devices));
 
-	status = acpi_evaluate_reference(tz->device->handle, "_PSL", NULL, &devices);
+	status = acpi_evaluate_reference(tz->device->handle, method, NULL, &devices);
 	if (ACPI_FAILURE(status)) {
-		acpi_handle_info(tz->device->handle,
-				 "Missing device list for passive threshold\n");
+		acpi_handle_info(tz->device->handle, "%s evaluation failure\n", method);
 		return false;
 	}
 
@@ -231,8 +240,9 @@  static void acpi_thermal_update_passive_
 	if (!acpi_thermal_trip_valid(acpi_trip))
 		return;
 
-	if (update_passive_devices(tz, true))
+	if (update_trip_devices(tz, acpi_trip, ACPI_THERMAL_TRIP_PASSIVE, true)) {
 		return;
+	}
 
 	acpi_trip->temperature = THERMAL_TEMP_INVALID;
 	ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
@@ -273,30 +283,6 @@  static void acpi_thermal_update_active_t
 		ACPI_THERMAL_TRIPS_EXCEPTION(tz, "state");
 }
 
-static bool update_active_devices(struct acpi_thermal *tz, int index, bool compare)
-{
-	char method[] = { '_', 'A', 'L', '0' + index, '\0' };
-	struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip;
-	struct acpi_handle_list devices;
-	acpi_status status;
-
-	memset(&devices, 0, sizeof(devices));
-
-	status = acpi_evaluate_reference(tz->device->handle, method, NULL, &devices);
-	if (ACPI_FAILURE(status)) {
-		acpi_handle_info(tz->device->handle,
-				 "Missing device list for active threshold %d\n",
-				 index);
-		return false;
-	}
-
-	if (compare && memcmp(&acpi_trip->devices, &devices, sizeof(devices)))
-		ACPI_THERMAL_TRIPS_EXCEPTION(tz, "device");
-
-	memcpy(&acpi_trip->devices, &devices, sizeof(devices));
-	return true;
-}
-
 static void acpi_thermal_update_active_devices(struct acpi_thermal *tz, int index)
 {
 	struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip;
@@ -304,7 +290,7 @@  static void acpi_thermal_update_active_d
 	if (!acpi_thermal_trip_valid(acpi_trip))
 		return;
 
-	if (update_active_devices(tz, index, true))
+	if (update_trip_devices(tz, acpi_trip, index, true))
 		return;
 
 	acpi_trip->temperature = THERMAL_TEMP_INVALID;
@@ -460,7 +446,8 @@  static bool acpi_thermal_init_passive_tr
 
 	tz->trips.passive.tsp = tmp;
 
-	if (!update_passive_devices(tz, false))
+	if (!update_trip_devices(tz, &tz->trips.passive.trip,
+				 ACPI_THERMAL_TRIP_PASSIVE, false))
 		goto fail;
 
 	tz->trips.passive.trip.temperature = temp;
@@ -482,7 +469,7 @@  static bool acpi_thermal_init_active_tri
 	if (temp == THERMAL_TEMP_INVALID)
 		goto fail;
 
-	if (!update_active_devices(tz, index, false))
+	if (!update_trip_devices(tz, &tz->trips.active[index].trip, index, false))
 		goto fail;
 
 	tz->trips.active[index].trip.temperature = temp;