| Message ID | 5708760.DvuYhMxLoT@kreacher |
|---|---|
| Headers | show |
| Series | ACPI: thermal: Removal of redundant data and cleanup | expand |
On Tue, Sep 12, 2023 at 8:47 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Separate the code needed to update active trips (in a response to a > notification from the platform firmware) as well as to initialize them > from the code that is only necessary for their initialization and > cleanly divide it into functions that each carry out a specific action. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/thermal.c | 197 ++++++++++++++++++++++++------------------------- > 1 file changed, 100 insertions(+), 97 deletions(-) > > Index: linux-pm/drivers/acpi/thermal.c > =================================================================== > --- linux-pm.orig/drivers/acpi/thermal.c > +++ linux-pm/drivers/acpi/thermal.c > @@ -184,94 +184,6 @@ static int acpi_thermal_temp(struct acpi > tz->kelvin_offset); > } > > -static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > -{ > - acpi_status status; > - unsigned long long tmp; > - struct acpi_handle_list devices; > - bool valid = false; > - int i; > - > - /* Active (optional) */ > - for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) { > - char name[5] = { '_', 'A', 'C', ('0' + i), '\0' }; > - valid = tz->trips.active[i].trip.valid; > - > - if (act == -1) > - break; /* disable all active trip points */ > - > - if (flag == ACPI_TRIPS_INIT || ((flag & ACPI_TRIPS_ACTIVE) && > - tz->trips.active[i].trip.valid)) { > - status = acpi_evaluate_integer(tz->device->handle, > - name, NULL, &tmp); > - if (ACPI_FAILURE(status)) { > - tz->trips.active[i].trip.valid = false; > - if (i == 0) > - break; > - > - if (act <= 0) > - break; > - > - if (i == 1) > - tz->trips.active[0].trip.temperature = > - celsius_to_deci_kelvin(act); > - else > - /* > - * Don't allow override higher than > - * the next higher trip point > - */ > - tz->trips.active[i-1].trip.temperature = > - min_t(unsigned long, > - tz->trips.active[i-2].trip.temperature, > - celsius_to_deci_kelvin(act)); > - > - break; > - } else { > - tz->trips.active[i].trip.temperature = tmp; > - tz->trips.active[i].trip.valid = true; > - } > - } > - > - name[2] = 'L'; > - if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.active[i].trip.valid) { > - memset(&devices, 0, sizeof(struct acpi_handle_list)); > - status = acpi_evaluate_reference(tz->device->handle, > - name, NULL, &devices); > - if (ACPI_FAILURE(status)) { > - acpi_handle_info(tz->device->handle, > - "Invalid active%d threshold\n", i); > - tz->trips.active[i].trip.valid = false; > - } else { > - tz->trips.active[i].trip.valid = true; > - } > - > - if (memcmp(&tz->trips.active[i].devices, &devices, > - sizeof(struct acpi_handle_list))) { > - memcpy(&tz->trips.active[i].devices, &devices, > - sizeof(struct acpi_handle_list)); > - ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > - } > - } > - if ((flag & ACPI_TRIPS_ACTIVE) || (flag & ACPI_TRIPS_DEVICES)) > - if (valid != tz->trips.active[i].trip.valid) > - ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state"); > - > - if (!tz->trips.active[i].trip.valid) > - break; > - } > - > - if (flag & ACPI_TRIPS_DEVICES) { > - memset(&devices, 0, sizeof(devices)); > - status = acpi_evaluate_reference(tz->device->handle, "_TZD", > - NULL, &devices); > - if (ACPI_SUCCESS(status) && > - memcmp(&tz->devices, &devices, sizeof(devices))) { > - tz->devices = devices; > - ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > - } > - } > -} > - > static void update_acpi_thermal_trip_temp(struct acpi_thermal_trip *acpi_trip, > int temp) > { > @@ -338,6 +250,78 @@ static void acpi_thermal_update_passive_ > ACPI_THERMAL_TRIPS_EXCEPTION(ACPI_TRIPS_PASSIVE, tz, "state"); > } > > +static long get_active_temp(struct acpi_thermal *tz, int index) > +{ > + char method[] = { '_', 'A', 'C', '0' + index, '\0' }; > + unsigned long long tmp; > + acpi_status status; > + > + status = acpi_evaluate_integer(tz->device->handle, method, NULL, &tmp); > + if (ACPI_FAILURE(status)) > + return THERMAL_TEMP_INVALID; > + > + /* > + * If an override has been provided, apply it so there are no active > + * trips with thresholds greater than the override. > + */ > + if (act > 0) { > + unsigned long long override = celsius_to_deci_kelvin(act); > + > + if (tmp > override) > + tmp = override; > + } > + return tmp; > +} > + > +static void acpi_thermal_update_active_trip(struct acpi_thermal *tz, int index) > +{ > + struct acpi_thermal_trip *acpi_trip = &tz->trips.active[index].trip; > + > + if (!acpi_trip->valid) > + return; > + > + update_acpi_thermal_trip_temp(acpi_trip, get_active_temp(tz, index)); > + if (!acpi_trip->valid) > + ACPI_THERMAL_TRIPS_EXCEPTION(ACPI_TRIPS_ACTIVE, tz, "state"); > +} > + > +static bool update_active_devices(struct acpi_thermal *tz, int index, bool compare) > +{ > + char method[] = { '_', 'A', 'L', '0' + index, '\0' }; > + 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(&tz->trips.active[index].devices, &devices, sizeof(devices))) > + ACPI_THERMAL_TRIPS_EXCEPTION(ACPI_TRIPS_ACTIVE, tz, "device"); > + > + memcpy(&tz->trips.active[index].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; > + > + if (!acpi_trip->valid) > + return; > + > + if (update_active_devices(tz, index, true)) > + return; > + > + update_acpi_thermal_trip_temp(acpi_trip, THERMAL_TEMP_INVALID); > + ACPI_THERMAL_TRIPS_EXCEPTION(ACPI_TRIPS_ACTIVE, tz, "state"); > +} > + > static int acpi_thermal_adjust_trip(struct thermal_trip *trip, void *data) > { > struct acpi_thermal_trip *acpi_trip = trip->priv; > @@ -358,18 +342,18 @@ static void acpi_thermal_adjust_thermal_ > unsigned long data) > { > struct acpi_thermal *tz = thermal_zone_device_priv(thermal); > - int flag; > + int i; > > if (data == ACPI_THERMAL_NOTIFY_THRESHOLDS) { > acpi_thermal_update_passive_trip(tz); > - flag = ACPI_TRIPS_THRESHOLDS; > + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) > + acpi_thermal_update_active_trip(tz, i); > } else { > acpi_thermal_update_passive_devices(tz); > - flag = ACPI_TRIPS_DEVICES; > + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) > + acpi_thermal_update_active_devices(tz, i); > } > > - __acpi_thermal_trips_update(tz, flag); > - > for_each_thermal_trip(tz->thermal_zone, acpi_thermal_adjust_trip, tz); > } > > @@ -498,6 +482,28 @@ fail: > return false; > } > > +static bool acpi_thermal_init_active_trip(struct acpi_thermal *tz, int index) > +{ > + long temp; > + > + if (act == -1) > + goto fail; > + > + temp = get_active_temp(tz, index); > + if (temp == THERMAL_TEMP_INVALID) > + goto fail; > + > + if (!update_active_devices(tz, false, index)) This should be if (!update_active_devices(tz, index, false)) and I've already fixed it in the tree. > + goto fail; > + > + update_acpi_thermal_trip_temp(&tz->trips.active[index].trip, temp); > + return true; > + > +fail: > + update_acpi_thermal_trip_temp(&tz->trips.active[index].trip, THERMAL_TEMP_INVALID); > + return false; > +} > + > static int acpi_thermal_get_trip_points(struct acpi_thermal *tz) > { > unsigned int count = 0; > @@ -506,11 +512,8 @@ static int acpi_thermal_get_trip_points( > if (acpi_thermal_init_passive_trip(tz)) > count++; > > - /* Active trip points (optional). */ > - __acpi_thermal_trips_update(tz, ACPI_TRIPS_INIT); > - > for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) { > - if (tz->trips.active[i].trip.valid) > + if (acpi_thermal_init_active_trip(tz, i)) > count++; > else > break; > > >
On Tue, Sep 12, 2023 at 08:47:23PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Notice that the valid flag in struct acpi_thermal_trip is in fact > redundant, because the temperature field of invalid trips is always > equal to THERMAL_TEMP_INVALID, so drop it from there and adjust the > code accordingly. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
On 12/09/2023 20:35, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Use the observation that the critical and hot trip points are never > updated by the ACPI thermal driver, because the flags passed from > acpi_thermal_notify() to acpi_thermal_trips_update() do not include > ACPI_TRIPS_CRITICAL or ACPI_TRIPS_HOT, to move the initialization > of those trip points directly into acpi_thermal_get_trip_points() and > reduce the size of __acpi_thermal_trips_update(). > > Also make the critical and hot trip points initialization code more > straightforward and drop the flags that are not needed any more. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- [ ... ] > +static void acpi_thermal_get_critical_trip(struct acpi_thermal *tz) > +{ > + unsigned long long tmp; > + acpi_status status; > + > + if (crt > 0) { > + tmp = celsius_to_deci_kelvin(crt); > + goto set; > + } > + if (crt == -1) { > + acpi_handle_debug(tz->device->handle, "Critical threshold disabled\n"); > + goto fail; > + } > + > + status = acpi_evaluate_integer(tz->device->handle, "_CRT", NULL, &tmp); > + if (ACPI_FAILURE(status)) { > + acpi_handle_debug(tz->device->handle, "No critical threshold\n"); > + goto fail; > + } > + if (tmp <= 2732) { > + /* > + * Below zero (Celsius) values clearly aren't right for sure, > + * so discard them as invalid. > + */ > + pr_info(FW_BUG "Invalid critical threshold (%llu)\n", tmp); > + goto fail; > + } > + > +set: > + tz->trips.critical.valid = true; > + tz->trips.critical.temperature = tmp; > + acpi_handle_debug(tz->device->handle, "Critical threshold [%lu]\n", > + tz->trips.critical.temperature); > + return; > + > +fail: nit: 'notset' may be more adequate > + tz->trips.critical.valid = false; > + tz->trips.critical.temperature = THERMAL_TEMP_INVALID; > +} Other than that, Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
On 12/09/2023 20:37, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Compute the number of trip points in acpi_thermal_add() so as to allow the > driver's data structures to be simplified going forward. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
On 12/09/2023 20:39, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Create and populate the driver's trip points table in acpi_thermal_add() > so as to allow the its data structures to be simplified going forward. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/acpi/thermal.c | 105 ++++++++++++++++++++++++------------------------- > 1 file changed, 52 insertions(+), 53 deletions(-) > > Index: linux-pm/drivers/acpi/thermal.c > =================================================================== > --- linux-pm.orig/drivers/acpi/thermal.c > +++ linux-pm/drivers/acpi/thermal.c > @@ -688,53 +688,10 @@ static void acpi_thermal_zone_sysfs_remo > } > > static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz, > - unsigned int trip_count) > + unsigned int trip_count, > + int passive_delay) > { > - struct acpi_thermal_trip *acpi_trip; > - struct thermal_trip *trip; > - int passive_delay = 0; > int result; > - int i; > - > - trip = kcalloc(trip_count, sizeof(*trip), GFP_KERNEL); > - if (!trip) > - return -ENOMEM; > - > - tz->trip_table = trip; > - > - if (tz->trips.critical.valid) { > - trip->type = THERMAL_TRIP_CRITICAL; > - trip->temperature = acpi_thermal_temp(tz, tz->trips.critical.temperature); > - trip++; > - } > - > - if (tz->trips.hot.valid) { > - trip->type = THERMAL_TRIP_HOT; > - trip->temperature = acpi_thermal_temp(tz, tz->trips.hot.temperature); > - trip++; > - } > - > - acpi_trip = &tz->trips.passive.trip; > - if (acpi_trip->valid) { > - passive_delay = tz->trips.passive.tsp * 100; > - > - trip->type = THERMAL_TRIP_PASSIVE; > - trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature); > - trip->priv = acpi_trip; > - trip++; > - } > - > - for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) { > - acpi_trip = &tz->trips.active[i].trip; > - > - if (!acpi_trip->valid) > - break; > - > - trip->type = THERMAL_TRIP_ACTIVE; > - trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature); > - trip->priv = acpi_trip; > - trip++; > - } > > tz->thermal_zone = thermal_zone_device_register_with_trips("acpitz", > tz->trip_table, > @@ -744,10 +701,8 @@ static int acpi_thermal_register_thermal > NULL, > passive_delay, > tz->polling_frequency * 100); > - if (IS_ERR(tz->thermal_zone)) { > - result = PTR_ERR(tz->thermal_zone); > - goto free_trip_table; > - } > + if (IS_ERR(tz->thermal_zone)) > + return PTR_ERR(tz->thermal_zone); > > result = acpi_thermal_zone_sysfs_add(tz); > if (result) > @@ -766,8 +721,6 @@ remove_links: > acpi_thermal_zone_sysfs_remove(tz); > unregister_tzd: > thermal_zone_device_unregister(tz->thermal_zone); > -free_trip_table: > - kfree(tz->trip_table); > > return result; > } > @@ -886,9 +839,13 @@ static void acpi_thermal_check_fn(struct > > static int acpi_thermal_add(struct acpi_device *device) > { > + struct acpi_thermal_trip *acpi_trip; > + struct thermal_trip *trip; > struct acpi_thermal *tz; > unsigned int trip_count; > + int passive_delay = 0; > int result; > + int i; > > if (!device) > return -EINVAL; > @@ -930,9 +887,49 @@ static int acpi_thermal_add(struct acpi_ > > acpi_thermal_guess_offset(tz); > > - result = acpi_thermal_register_thermal_zone(tz, trip_count); > + trip = kcalloc(trip_count, sizeof(*trip), GFP_KERNEL); > + if (!trip) > + return -ENOMEM; > + > + tz->trip_table = trip; > + > + if (tz->trips.critical.valid) { > + trip->type = THERMAL_TRIP_CRITICAL; > + trip->temperature = acpi_thermal_temp(tz, tz->trips.critical.temperature); > + trip++; > + } > + > + if (tz->trips.hot.valid) { > + trip->type = THERMAL_TRIP_HOT; > + trip->temperature = acpi_thermal_temp(tz, tz->trips.hot.temperature); > + trip++; > + } > + > + acpi_trip = &tz->trips.passive.trip; > + if (acpi_trip->valid) { > + passive_delay = tz->trips.passive.tsp * 100; > + > + trip->type = THERMAL_TRIP_PASSIVE; > + trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature); > + trip->priv = acpi_trip; > + trip++; > + } > + > + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) { > + acpi_trip = &tz->trips.active[i].trip; > + > + if (!acpi_trip->valid) > + break; > + > + trip->type = THERMAL_TRIP_ACTIVE; > + trip->temperature = acpi_thermal_temp(tz, acpi_trip->temperature); > + trip->priv = acpi_trip; > + trip++; > + } > + > + result = acpi_thermal_register_thermal_zone(tz, trip_count, passive_delay); > if (result) > - goto free_memory; > + goto free_trips; > > refcount_set(&tz->thermal_check_count, 3); > mutex_init(&tz->thermal_check_lock); > @@ -951,6 +948,8 @@ 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: > kfree(tz); > > > >
On Mon, Sep 25, 2023 at 5:20 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 12/09/2023 20:35, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Use the observation that the critical and hot trip points are never > > updated by the ACPI thermal driver, because the flags passed from > > acpi_thermal_notify() to acpi_thermal_trips_update() do not include > > ACPI_TRIPS_CRITICAL or ACPI_TRIPS_HOT, to move the initialization > > of those trip points directly into acpi_thermal_get_trip_points() and > > reduce the size of __acpi_thermal_trips_update(). > > > > Also make the critical and hot trip points initialization code more > > straightforward and drop the flags that are not needed any more. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > [ ... ] > > > +static void acpi_thermal_get_critical_trip(struct acpi_thermal *tz) > > +{ > > + unsigned long long tmp; > > + acpi_status status; > > + > > + if (crt > 0) { > > + tmp = celsius_to_deci_kelvin(crt); > > + goto set; > > + } > > + if (crt == -1) { > > + acpi_handle_debug(tz->device->handle, "Critical threshold disabled\n"); > > + goto fail; > > + } > > + > > + status = acpi_evaluate_integer(tz->device->handle, "_CRT", NULL, &tmp); > > + if (ACPI_FAILURE(status)) { > > + acpi_handle_debug(tz->device->handle, "No critical threshold\n"); > > + goto fail; > > + } > > + if (tmp <= 2732) { > > + /* > > + * Below zero (Celsius) values clearly aren't right for sure, > > + * so discard them as invalid. > > + */ > > + pr_info(FW_BUG "Invalid critical threshold (%llu)\n", tmp); > > + goto fail; > > + } > > + > > +set: > > + tz->trips.critical.valid = true; > > + tz->trips.critical.temperature = tmp; > > + acpi_handle_debug(tz->device->handle, "Critical threshold [%lu]\n", > > + tz->trips.critical.temperature); > > + return; > > + > > +fail: > > nit: 'notset' may be more adequate Well, this is a bit moot, because the label goes away in one of the later patches anyway. > > + tz->trips.critical.valid = false; > > + tz->trips.critical.temperature = THERMAL_TEMP_INVALID; > > +} > > Other than that, > > Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org> Thanks!
On 12/09/2023 20:43, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Separate the code needed to update active trips (in a response to a > notification from the platform firmware) as well as to initialize them > from the code that is only necessary for their initialization and > cleanly divide it into functions that each carry out a specific action. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
On 12/09/2023 20:47, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Notice that the valid flag in struct acpi_thermal_trip is in fact > redundant, because the temperature field of invalid trips is always > equal to THERMAL_TEMP_INVALID, so drop it from there and adjust the > code accordingly. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>