Re: [PATCH v1 2/6] thermal: ACPI: Discard trip table after zone registration

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Mon, Feb 05, 2024 at 10:15:50PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because the thermal core creates and uses its own copy of the trips
> table passed to thermal_zone_device_register_with_trips(), it is not
> necessary to hold on to a local copy of it any more after the given
> thermal zone has been registered.
> 
> Accordingly, modify the ACPI thermal driver to store the trips table
> passed to thermal_zone_device_register_with_trips() in a local variable
> which is automatically discarded after the zone registration.
> 
> Also make some additional code simplifications unlocked by the above
> change.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/acpi/thermal.c |   57 +++++++++++++++++--------------------------------
>  1 file changed, 20 insertions(+), 37 deletions(-)
> 
> Index: linux-pm/drivers/acpi/thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/thermal.c
> +++ linux-pm/drivers/acpi/thermal.c
> @@ -47,6 +47,8 @@
>  
>  #define ACPI_THERMAL_TRIP_PASSIVE	(-1)
>  
> +#define ACPI_THERMAL_MAX_NR_TRIPS	(ACPI_THERMAL_MAX_ACTIVE + 3)
> +
>  /*
>   * This exception is thrown out in two cases:
>   * 1.An invalid trip point becomes invalid or a valid trip point becomes invalid
> @@ -112,7 +114,6 @@ struct acpi_thermal {
>  	unsigned long polling_frequency;
>  	volatile u8 zombie;
>  	struct acpi_thermal_trips trips;
> -	struct thermal_trip *trip_table;
>  	struct thermal_zone_device *thermal_zone;
>  	int kelvin_offset;	/* in millidegrees */
>  	struct work_struct thermal_check_work;
> @@ -451,26 +452,19 @@ fail:
>  	return false;
>  }
>  
> -static int acpi_thermal_get_trip_points(struct acpi_thermal *tz)
> +static void acpi_thermal_get_trip_points(struct acpi_thermal *tz)
>  {
> -	unsigned int count = 0;
>  	int i;
>  
> -	if (acpi_thermal_init_trip(tz, ACPI_THERMAL_TRIP_PASSIVE))
> -		count++;
> +	acpi_thermal_init_trip(tz, ACPI_THERMAL_TRIP_PASSIVE);
>  
>  	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
> -		if (acpi_thermal_init_trip(tz, i))
> -			count++;
> -		else
> +		if (!acpi_thermal_init_trip(tz, i))
>  			break;
> -
>  	}
>  
>  	while (++i < ACPI_THERMAL_MAX_ACTIVE)
>  		tz->trips.active[i].trip.temp_dk = THERMAL_TEMP_INVALID;
> -
> -	return count;
>  }
>  
>  /* sys I/F for generic thermal sysfs support */
> @@ -662,13 +656,14 @@ static void acpi_thermal_zone_sysfs_remo
>  }
>  
>  static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz,
> +					      const struct thermal_trip *trip_table,
>  					      unsigned int trip_count,
>  					      int passive_delay)
>  {
>  	int result;
>  
>  	tz->thermal_zone = thermal_zone_device_register_with_trips("acpitz",
> -								   tz->trip_table,
> +								   trip_table,
>  								   trip_count,
>  								   0, tz,
>  								   &acpi_thermal_zone_ops,
> @@ -823,10 +818,10 @@ static void acpi_thermal_free_thermal_zo
>  
>  static int acpi_thermal_add(struct acpi_device *device)
>  {
> +	struct thermal_trip trip_table[ACPI_THERMAL_MAX_NR_TRIPS] = { 0 };
>  	struct acpi_thermal_trip *acpi_trip;
>  	struct thermal_trip *trip;
>  	struct acpi_thermal *tz;
> -	unsigned int trip_count;
>  	int crit_temp, hot_temp;
>  	int passive_delay = 0;
>  	int result;
> @@ -848,21 +843,10 @@ static int acpi_thermal_add(struct acpi_
>  	acpi_thermal_aml_dependency_fix(tz);
>  
>  	/* Get trip points [_CRT, _PSV, etc.] (required). */
> -	trip_count = acpi_thermal_get_trip_points(tz);
> +	acpi_thermal_get_trip_points(tz);
>  
>  	crit_temp = acpi_thermal_get_critical_trip(tz);
> -	if (crit_temp != THERMAL_TEMP_INVALID)
> -		trip_count++;
> -
>  	hot_temp = acpi_thermal_get_hot_trip(tz);
> -	if (hot_temp != THERMAL_TEMP_INVALID)
> -		trip_count++;
> -
> -	if (!trip_count) {
> -		pr_warn(FW_BUG "No valid trip points!\n");
> -		result = -ENODEV;
> -		goto free_memory;
> -	}
>  
>  	/* Get temperature [_TMP] (required). */
>  	result = acpi_thermal_get_temperature(tz);
> @@ -881,13 +865,7 @@ static int acpi_thermal_add(struct acpi_
>  
>  	acpi_thermal_guess_offset(tz, crit_temp);
>  
> -	trip = kcalloc(trip_count, sizeof(*trip), GFP_KERNEL);
> -	if (!trip) {
> -		result = -ENOMEM;
> -		goto free_memory;
> -	}
> -
> -	tz->trip_table = trip;
> +	trip = trip_table;
>  
>  	if (crit_temp != THERMAL_TEMP_INVALID) {
>  		trip->type = THERMAL_TRIP_CRITICAL;
> @@ -923,9 +901,17 @@ static int acpi_thermal_add(struct acpi_
>  		trip++;
>  	}
>  
> -	result = acpi_thermal_register_thermal_zone(tz, trip_count, passive_delay);
> +	if (trip == trip_table) {
> +		pr_warn(FW_BUG "No valid trip points!\n");
> +		result = -ENODEV;
> +		goto free_memory;
> +	}
> +
> +	result = acpi_thermal_register_thermal_zone(tz, trip_table,
> +						    trip - trip_table,
> +						    passive_delay);
>  	if (result)
> -		goto free_trips;
> +		goto free_memory;
>  
>  	refcount_set(&tz->thermal_check_count, 3);
>  	mutex_init(&tz->thermal_check_lock);
> @@ -944,8 +930,6 @@ static int acpi_thermal_add(struct acpi_
>  flush_wq:
>  	flush_workqueue(acpi_thermal_pm_queue);
>  	acpi_thermal_unregister_thermal_zone(tz);
> -free_trips:
> -	kfree(tz->trip_table);
>  free_memory:
>  	acpi_thermal_free_thermal_zone(tz);
>  
> @@ -966,7 +950,6 @@ static void acpi_thermal_remove(struct a
>  
>  	flush_workqueue(acpi_thermal_pm_queue);
>  	acpi_thermal_unregister_thermal_zone(tz);
> -	kfree(tz->trip_table);
>  	acpi_thermal_free_thermal_zone(tz);
>  }
>  
> 
> 
>