| Message ID | 20230927201725.2339488-1-vi@endrift.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] ACPI: utils: Make acpi_handle_list dynamically allocated | expand |
On Wednesday, September 27, 2023 10:17:25 PM CEST Vicki Pfau wrote: > This fixes a long-standing "TBD" comment in the ACPI headers regarding making > the acpi_handle_list struct's size dynamic. The number 10, which along with the > comment dates back to 2.4.23, seems like it may have been arbitrarily chosen, > and isn't sufficient in all cases. This patch finally makes the size dynamic > and updates its users to handle the modified API. First off, the effort is definitely appreciated, because this is about addressing a real issue seen in the field. However, the users of acpi_handle_list should not need to care about its internal structure, so functions should be provided for freeing it and manipulating it as needed. > > Signed-off-by: Vicki Pfau <vi@endrift.com> > --- > drivers/acpi/acpi_lpss.c | 5 +++- > drivers/acpi/scan.c | 1 + > drivers/acpi/thermal.c | 23 ++++++++++++++----- > drivers/acpi/utils.c | 5 ++-- > .../platform/surface/surface_acpi_notify.c | 5 +++- > include/acpi/acpi_bus.h | 4 +--- > 6 files changed, 30 insertions(+), 13 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index 539e700de4d2..4b3aa84faf70 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -591,10 +591,13 @@ static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > } > > for (i = 0; i < dep_devices.count; i++) { > - if (dep_devices.handles[i] == handle) > + if (dep_devices.handles[i] == handle) { > + kfree(dep_devices.handles); > return true; > + } > } > > + kfree(dep_devices.handles); Moreover, the code duplication here can be avoided by using a return variable. > return false; > } > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 691d4b7686ee..2fbe354db0c0 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2032,6 +2032,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep) > mutex_unlock(&acpi_dep_list_lock); > } > > + kfree(dep_devices.handles); > return count; > } > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 312730f8272e..48ddc56f96f6 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -307,8 +307,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > tz->trips.passive.trip.valid = true; > } > > - if (memcmp(&tz->trips.passive.devices, &devices, > - sizeof(struct acpi_handle_list))) { > + if (&tz->trips.passive.devices.count != devices.count || > + memcmp(tz->trips.passive.devices.handles, > + devices.handles, sizeof(acpi_handle) * devices.count)) { > + kfree(tz->trips.passive.devices.handles); > memcpy(&tz->trips.passive.devices, &devices, > sizeof(struct acpi_handle_list)); > ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > @@ -372,8 +374,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > tz->trips.active[i].trip.valid = true; > } > > - if (memcmp(&tz->trips.active[i].devices, &devices, > - sizeof(struct acpi_handle_list))) { > + if (&tz->trips.active[i].devices.count != devices.count || > + memcmp(tz->trips.active[i].devices.handles, > + devices.handles, sizeof(acpi_handle) * devices.count)) { > + kfree(tz->trips.active[i].devices.handles); > memcpy(&tz->trips.active[i].devices, &devices, > sizeof(struct acpi_handle_list)); > ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > @@ -391,8 +395,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > 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))) { > + if (ACPI_SUCCESS(status) && (tz->devices.count != devices.count || > + memcmp(tz->devices.handles, devices.handles, > + sizeof(acpi_handle) * devices.count))) { > + kfree(tz->devices.handles); > tz->devices = devices; > ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > } All of the changes in thermal.c above need to be rebased on the ACPI thermal changes currently in-flight, but that's something for me to take care of. > @@ -974,6 +980,7 @@ static int acpi_thermal_add(struct acpi_device *device) > static void acpi_thermal_remove(struct acpi_device *device) > { > struct acpi_thermal *tz; > + int i; > > if (!device || !acpi_driver_data(device)) > return; > @@ -986,6 +993,10 @@ static void acpi_thermal_remove(struct acpi_device *device) > flush_workqueue(acpi_thermal_pm_queue); > acpi_thermal_unregister_thermal_zone(tz); > > + kfree(tz->trips.passive.devices.handles); > + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) > + kfree(tz->trips.active[i].devices.handles); > + kfree(tz->devices.handles); And this also needs to be done in the acpi_thermal_add() error cleanup path so as to avoid leaking memory from there. > kfree(tz); > } > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 2ea14648a661..96f821c41756 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -370,7 +370,8 @@ acpi_evaluate_reference(acpi_handle handle, > goto end; > } > > - if (package->package.count > ACPI_MAX_HANDLES) { > + list->handles = kcalloc(package->package.count, sizeof(*list->handles), GFP_KERNEL); > + if (!list->handles) { > kfree(package); > return AE_NO_MEMORY; > } > @@ -402,7 +403,7 @@ acpi_evaluate_reference(acpi_handle handle, > end: > if (ACPI_FAILURE(status)) { > list->count = 0; > - //kfree(list->handles); > + kfree(list->handles); Clearing list->handles (to avoid leaking a stale pointer) here would be nice. > } > > kfree(buffer.pointer); > diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c > index 897cdd9c3aae..6932d4b35c6c 100644 > --- a/drivers/platform/surface/surface_acpi_notify.c > +++ b/drivers/platform/surface/surface_acpi_notify.c > @@ -753,10 +753,13 @@ static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle) > } > > for (i = 0; i < dep_devices.count; i++) { > - if (dep_devices.handles[i] == supplier) > + if (dep_devices.handles[i] == supplier) { > + kfree(dep_devices.handles); > return true; > + } > } > > + kfree(dep_devices.handles); > return false; And again, the code duplication here is avoidable. > } > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 254685085c82..b4bf12343a22 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -12,11 +12,9 @@ > #include <linux/device.h> > #include <linux/property.h> > > -/* TBD: Make dynamic */ > -#define ACPI_MAX_HANDLES 10 > struct acpi_handle_list { > u32 count; > - acpi_handle handles[ACPI_MAX_HANDLES]; > + acpi_handle* handles; > }; > > /* acpi_utils.h */ > All of the above said, I don't really want to defer making this change too much, so below is my version of it (with the Co-developed-by tag pointing to you and your S-o-b as the co-author of the changes) and this is what I'm going to apply unless somebody is able to poke a hole in it. --- Subject: [PATCH] ACPI: utils: Dynamically determine acpi_handle_list size From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Address a long-standing "TBD" comment in the ACPI headers regarding the number of handles in struct acpi_handle_list. The number 10, which along with the comment dates back to 2.4.23, seems like it may have been arbitrarily chosen and isn't sufficient in all cases [1]. Finally change the code to dynamically determine the size of the handles table in struct acpi_handle_list and allocate it accordingly. Update the users of to struct acpi_handle_list to take the additional dynamic allocation into account. Link: https://lore.kernel.org/linux-acpi/20230809094451.15473-1-ivan.hu@canonical.com # [1] Co-developed-by: Vicki Pfau <vi@endrift.com> Signed-off-by: Vicki Pfau <vi@endrift.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- This applies on top of https://patchwork.kernel.org/project/linux-pm/patch/1785516.VLH7GnMWUR@kreacher/ --- drivers/acpi/acpi_lpss.c | 10 ++-- drivers/acpi/scan.c | 1 drivers/acpi/thermal.c | 28 ++++++++--- drivers/acpi/utils.c | 61 ++++++++++++++++++++++++- drivers/platform/surface/surface_acpi_notify.c | 10 ++-- include/acpi/acpi_bus.h | 9 ++- 6 files changed, 101 insertions(+), 18 deletions(-) Index: linux-pm/drivers/acpi/acpi_lpss.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_lpss.c +++ linux-pm/drivers/acpi/acpi_lpss.c @@ -578,6 +578,7 @@ static bool acpi_lpss_dep(struct acpi_de { struct acpi_handle_list dep_devices; acpi_status status; + bool ret = false; int i; if (!acpi_has_method(adev->handle, "_DEP")) @@ -591,11 +592,14 @@ static bool acpi_lpss_dep(struct acpi_de } for (i = 0; i < dep_devices.count; i++) { - if (dep_devices.handles[i] == handle) - return true; + if (dep_devices.handles[i] == handle) { + ret = true; + break; + } } - return false; + acpi_handle_list_free(&dep_devices); + return ret; } static void acpi_lpss_link_consumer(struct device *dev1, Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -2031,6 +2031,7 @@ static u32 acpi_scan_check_dep(acpi_hand mutex_unlock(&acpi_dep_list_lock); } + acpi_handle_list_free(&dep_devices); return count; } Index: linux-pm/drivers/acpi/thermal.c =================================================================== --- linux-pm.orig/drivers/acpi/thermal.c +++ linux-pm/drivers/acpi/thermal.c @@ -208,7 +208,7 @@ static bool update_trip_devices(struct a struct acpi_thermal_trip *acpi_trip, int index, bool compare) { - struct acpi_handle_list devices; + struct acpi_handle_list devices = { 0 }; char method[] = "_PSL"; acpi_status status; @@ -218,18 +218,21 @@ static bool update_trip_devices(struct a method[3] = '0' + index; } - 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, "%s evaluation failure\n", method); return false; } - if (compare && memcmp(&acpi_trip->devices, &devices, sizeof(devices))) + if (acpi_handle_list_equal(&acpi_trip->devices, &devices)) { + acpi_handle_list_free(&devices); + return true; + } + + if (compare) ACPI_THERMAL_TRIPS_EXCEPTION(tz, "device"); - memcpy(&acpi_trip->devices, &devices, sizeof(devices)); + acpi_handle_list_replace(&acpi_trip->devices, &devices); return true; } @@ -845,6 +848,17 @@ static void acpi_thermal_check_fn(struct mutex_unlock(&tz->thermal_check_lock); } +static void acpi_thermal_free_thermal_zone(struct acpi_thermal *tz) +{ + int i; + + acpi_handle_list_free(&tz->trips.passive.trip.devices); + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) + acpi_handle_list_free(&tz->trips.active[i].trip.devices); + + kfree(tz); +} + static int acpi_thermal_add(struct acpi_device *device) { struct acpi_thermal_trip *acpi_trip; @@ -971,7 +985,7 @@ flush_wq: free_trips: kfree(tz->trip_table); free_memory: - kfree(tz); + acpi_thermal_free_thermal_zone(tz); return result; } @@ -991,7 +1005,7 @@ static void acpi_thermal_remove(struct a flush_workqueue(acpi_thermal_pm_queue); acpi_thermal_unregister_thermal_zone(tz); - kfree(tz); + acpi_thermal_free_thermal_zone(tz); } #ifdef CONFIG_PM_SLEEP Index: linux-pm/drivers/acpi/utils.c =================================================================== --- linux-pm.orig/drivers/acpi/utils.c +++ linux-pm/drivers/acpi/utils.c @@ -370,7 +370,8 @@ acpi_evaluate_reference(acpi_handle hand goto end; } - if (package->package.count > ACPI_MAX_HANDLES) { + list->handles = kcalloc(package->package.count, sizeof(*list->handles), GFP_KERNEL); + if (!list->handles) { kfree(package); return AE_NO_MEMORY; } @@ -402,7 +403,8 @@ acpi_evaluate_reference(acpi_handle hand end: if (ACPI_FAILURE(status)) { list->count = 0; - //kfree(list->handles); + kfree(list->handles); + list->handles = NULL; } kfree(buffer.pointer); @@ -412,6 +414,61 @@ acpi_evaluate_reference(acpi_handle hand EXPORT_SYMBOL(acpi_evaluate_reference); +/** + * acpi_handle_list_equal - Check if two ACPI handle lists are the same + * @list1: First list to compare. + * @List2: Second list to compare. + * + * Return true if the given ACPI handle lists are of the same size and + * contain the same ACPI handles in the same order. Otherwise, return false. + */ +bool acpi_handle_list_equal(struct acpi_handle_list *list1, + struct acpi_handle_list *list2) +{ + return list1->count == list2->count && + !memcmp(list1->handles, list2->handles, + list1->count * sizeof(acpi_handle)); +} +EXPORT_SYMBOL_GPL(acpi_handle_list_equal); + +/** + * acpi_handle_list_replace - Replace one ACPI handle list with another + * @dst: ACPI handle list to replace. + * @src: Source ACPI handle list. + * + * Free the handles table in @dst, move the handles table from @src to @dst, + * copy count from @src to @dst and clear @src. + */ +void acpi_handle_list_replace(struct acpi_handle_list *dst, + struct acpi_handle_list *src) +{ + if (dst->count) + kfree(dst->handles); + + dst->count = src->count; + dst->handles = src->handles; + + src->handles = NULL; + src->count = 0; +} +EXPORT_SYMBOL_GPL(acpi_handle_list_replace); + +/** + * acpi_handle_list_free - Free the handles table in an ACPI handle list + * @list: ACPI handle list to free. + * + * Free the handles table in @list and clear its count field. + */ +void acpi_handle_list_free(struct acpi_handle_list *list) +{ + if (!list->count) + return; + + kfree(list->handles); + list->count = 0; +} +EXPORT_SYMBOL_GPL(acpi_handle_list_free); + acpi_status acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld) { Index: linux-pm/drivers/platform/surface/surface_acpi_notify.c =================================================================== --- linux-pm.orig/drivers/platform/surface/surface_acpi_notify.c +++ linux-pm/drivers/platform/surface/surface_acpi_notify.c @@ -741,6 +741,7 @@ static bool is_san_consumer(struct platf struct acpi_handle_list dep_devices; acpi_handle supplier = ACPI_HANDLE(&pdev->dev); acpi_status status; + bool ret = false; int i; if (!acpi_has_method(handle, "_DEP")) @@ -753,11 +754,14 @@ static bool is_san_consumer(struct platf } for (i = 0; i < dep_devices.count; i++) { - if (dep_devices.handles[i] == supplier) - return true; + if (dep_devices.handles[i] == supplier) { + ret = true; + break; + } } - return false; + acpi_handle_list_free(&dep_devices); + return ret; } static acpi_status san_consumer_setup(acpi_handle handle, u32 lvl, Index: linux-pm/include/acpi/acpi_bus.h =================================================================== --- linux-pm.orig/include/acpi/acpi_bus.h +++ linux-pm/include/acpi/acpi_bus.h @@ -12,11 +12,9 @@ #include <linux/device.h> #include <linux/property.h> -/* TBD: Make dynamic */ -#define ACPI_MAX_HANDLES 10 struct acpi_handle_list { u32 count; - acpi_handle handles[ACPI_MAX_HANDLES]; + acpi_handle* handles; }; /* acpi_utils.h */ @@ -32,6 +30,11 @@ acpi_evaluate_reference(acpi_handle hand acpi_string pathname, struct acpi_object_list *arguments, struct acpi_handle_list *list); +bool acpi_handle_list_equal(struct acpi_handle_list *list1, + struct acpi_handle_list *list2); +void acpi_handle_list_replace(struct acpi_handle_list *dst, + struct acpi_handle_list *src); +void acpi_handle_list_free(struct acpi_handle_list *list); acpi_status acpi_evaluate_ost(acpi_handle handle, u32 source_event, u32 status_code, struct acpi_buffer *status_buf);
On 9/28/23 08:40, Rafael J. Wysocki wrote: > On Wednesday, September 27, 2023 10:17:25 PM CEST Vicki Pfau wrote: >> This fixes a long-standing "TBD" comment in the ACPI headers regarding making >> the acpi_handle_list struct's size dynamic. The number 10, which along with the >> comment dates back to 2.4.23, seems like it may have been arbitrarily chosen, >> and isn't sufficient in all cases. This patch finally makes the size dynamic >> and updates its users to handle the modified API. > > First off, the effort is definitely appreciated, because this is about > addressing a real issue seen in the field. > > However, the users of acpi_handle_list should not need to care about its > internal structure, so functions should be provided for freeing it and > manipulating it as needed. I didn't want them to have to care about the internal structure either, but it was so simple that utility functions seemed pointless, which is what led in some capacity to my v1 version of this patch (this is basically actually v0, but fixed up because I lost the original version and had to reimplement part of it). But I guess you're right, utility functions are the right approach here. > >> >> Signed-off-by: Vicki Pfau <vi@endrift.com> >> --- >> drivers/acpi/acpi_lpss.c | 5 +++- >> drivers/acpi/scan.c | 1 + >> drivers/acpi/thermal.c | 23 ++++++++++++++----- >> drivers/acpi/utils.c | 5 ++-- >> .../platform/surface/surface_acpi_notify.c | 5 +++- >> include/acpi/acpi_bus.h | 4 +--- >> 6 files changed, 30 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c >> index 539e700de4d2..4b3aa84faf70 100644 >> --- a/drivers/acpi/acpi_lpss.c >> +++ b/drivers/acpi/acpi_lpss.c >> @@ -591,10 +591,13 @@ static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> } >> >> for (i = 0; i < dep_devices.count; i++) { >> - if (dep_devices.handles[i] == handle) >> + if (dep_devices.handles[i] == handle) { >> + kfree(dep_devices.handles); >> return true; >> + } >> } >> >> + kfree(dep_devices.handles); > > Moreover, the code duplication here can be avoided by using a return variable. > >> return false; >> } >> >> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c >> index 691d4b7686ee..2fbe354db0c0 100644 >> --- a/drivers/acpi/scan.c >> +++ b/drivers/acpi/scan.c >> @@ -2032,6 +2032,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep) >> mutex_unlock(&acpi_dep_list_lock); >> } >> >> + kfree(dep_devices.handles); >> return count; >> } >> >> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c >> index 312730f8272e..48ddc56f96f6 100644 >> --- a/drivers/acpi/thermal.c >> +++ b/drivers/acpi/thermal.c >> @@ -307,8 +307,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) >> tz->trips.passive.trip.valid = true; >> } >> >> - if (memcmp(&tz->trips.passive.devices, &devices, >> - sizeof(struct acpi_handle_list))) { >> + if (&tz->trips.passive.devices.count != devices.count || >> + memcmp(tz->trips.passive.devices.handles, >> + devices.handles, sizeof(acpi_handle) * devices.count)) { >> + kfree(tz->trips.passive.devices.handles); >> memcpy(&tz->trips.passive.devices, &devices, >> sizeof(struct acpi_handle_list)); >> ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); >> @@ -372,8 +374,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) >> tz->trips.active[i].trip.valid = true; >> } >> >> - if (memcmp(&tz->trips.active[i].devices, &devices, >> - sizeof(struct acpi_handle_list))) { >> + if (&tz->trips.active[i].devices.count != devices.count || >> + memcmp(tz->trips.active[i].devices.handles, >> + devices.handles, sizeof(acpi_handle) * devices.count)) { >> + kfree(tz->trips.active[i].devices.handles); >> memcpy(&tz->trips.active[i].devices, &devices, >> sizeof(struct acpi_handle_list)); >> ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); >> @@ -391,8 +395,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) >> 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))) { >> + if (ACPI_SUCCESS(status) && (tz->devices.count != devices.count || >> + memcmp(tz->devices.handles, devices.handles, >> + sizeof(acpi_handle) * devices.count))) { >> + kfree(tz->devices.handles); >> tz->devices = devices; >> ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); >> } > > All of the changes in thermal.c above need to be rebased on the ACPI thermal > changes currently in-flight, but that's something for me to take care of. > >> @@ -974,6 +980,7 @@ static int acpi_thermal_add(struct acpi_device *device) >> static void acpi_thermal_remove(struct acpi_device *device) >> { >> struct acpi_thermal *tz; >> + int i; >> >> if (!device || !acpi_driver_data(device)) >> return; >> @@ -986,6 +993,10 @@ static void acpi_thermal_remove(struct acpi_device *device) >> flush_workqueue(acpi_thermal_pm_queue); >> acpi_thermal_unregister_thermal_zone(tz); >> >> + kfree(tz->trips.passive.devices.handles); >> + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) >> + kfree(tz->trips.active[i].devices.handles); >> + kfree(tz->devices.handles); > > And this also needs to be done in the acpi_thermal_add() error cleanup path > so as to avoid leaking memory from there. > >> kfree(tz); >> } >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 2ea14648a661..96f821c41756 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -370,7 +370,8 @@ acpi_evaluate_reference(acpi_handle handle, >> goto end; >> } >> >> - if (package->package.count > ACPI_MAX_HANDLES) { >> + list->handles = kcalloc(package->package.count, sizeof(*list->handles), GFP_KERNEL); >> + if (!list->handles) { >> kfree(package); >> return AE_NO_MEMORY; >> } >> @@ -402,7 +403,7 @@ acpi_evaluate_reference(acpi_handle handle, >> end: >> if (ACPI_FAILURE(status)) { >> list->count = 0; >> - //kfree(list->handles); >> + kfree(list->handles); > > Clearing list->handles (to avoid leaking a stale pointer) here would be nice. > >> } >> >> kfree(buffer.pointer); >> diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c >> index 897cdd9c3aae..6932d4b35c6c 100644 >> --- a/drivers/platform/surface/surface_acpi_notify.c >> +++ b/drivers/platform/surface/surface_acpi_notify.c >> @@ -753,10 +753,13 @@ static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle) >> } >> >> for (i = 0; i < dep_devices.count; i++) { >> - if (dep_devices.handles[i] == supplier) >> + if (dep_devices.handles[i] == supplier) { >> + kfree(dep_devices.handles); >> return true; >> + } >> } >> >> + kfree(dep_devices.handles); >> return false; > > And again, the code duplication here is avoidable. > >> } >> >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 254685085c82..b4bf12343a22 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -12,11 +12,9 @@ >> #include <linux/device.h> >> #include <linux/property.h> >> >> -/* TBD: Make dynamic */ >> -#define ACPI_MAX_HANDLES 10 >> struct acpi_handle_list { >> u32 count; >> - acpi_handle handles[ACPI_MAX_HANDLES]; >> + acpi_handle* handles; >> }; >> >> /* acpi_utils.h */ >> > > All of the above said, I don't really want to defer making this change > too much, so below is my version of it (with the Co-developed-by tag > pointing to you and your S-o-b as the co-author of the changes) and > this is what I'm going to apply unless somebody is able to poke a hole > in it. Understandable, and thanks for saving me the effort I suppose. > --- > Subject: [PATCH] ACPI: utils: Dynamically determine acpi_handle_list size > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Address a long-standing "TBD" comment in the ACPI headers regarding the > number of handles in struct acpi_handle_list. > > The number 10, which along with the comment dates back to 2.4.23, seems > like it may have been arbitrarily chosen and isn't sufficient in all > cases [1]. > > Finally change the code to dynamically determine the size of the handles > table in struct acpi_handle_list and allocate it accordingly. > > Update the users of to struct acpi_handle_list to take the additional > dynamic allocation into account. > > Link: https://lore.kernel.org/linux-acpi/20230809094451.15473-1-ivan.hu@canonical.com # [1] > Co-developed-by: Vicki Pfau <vi@endrift.com> > Signed-off-by: Vicki Pfau <vi@endrift.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > This applies on top of > > https://patchwork.kernel.org/project/linux-pm/patch/1785516.VLH7GnMWUR@kreacher/ > > --- > drivers/acpi/acpi_lpss.c | 10 ++-- > drivers/acpi/scan.c | 1 > drivers/acpi/thermal.c | 28 ++++++++--- > drivers/acpi/utils.c | 61 ++++++++++++++++++++++++- > drivers/platform/surface/surface_acpi_notify.c | 10 ++-- > include/acpi/acpi_bus.h | 9 ++- > 6 files changed, 101 insertions(+), 18 deletions(-) > > Index: linux-pm/drivers/acpi/acpi_lpss.c > =================================================================== > --- linux-pm.orig/drivers/acpi/acpi_lpss.c > +++ linux-pm/drivers/acpi/acpi_lpss.c > @@ -578,6 +578,7 @@ static bool acpi_lpss_dep(struct acpi_de > { > struct acpi_handle_list dep_devices; > acpi_status status; > + bool ret = false; > int i; > > if (!acpi_has_method(adev->handle, "_DEP")) > @@ -591,11 +592,14 @@ static bool acpi_lpss_dep(struct acpi_de > } > > for (i = 0; i < dep_devices.count; i++) { > - if (dep_devices.handles[i] == handle) > - return true; > + if (dep_devices.handles[i] == handle) { > + ret = true; > + break; > + } > } > > - return false; > + acpi_handle_list_free(&dep_devices); > + return ret; > } > > static void acpi_lpss_link_consumer(struct device *dev1, > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -2031,6 +2031,7 @@ static u32 acpi_scan_check_dep(acpi_hand > mutex_unlock(&acpi_dep_list_lock); > } > > + acpi_handle_list_free(&dep_devices); > return count; > } > > Index: linux-pm/drivers/acpi/thermal.c > =================================================================== > --- linux-pm.orig/drivers/acpi/thermal.c > +++ linux-pm/drivers/acpi/thermal.c > @@ -208,7 +208,7 @@ static bool update_trip_devices(struct a > struct acpi_thermal_trip *acpi_trip, > int index, bool compare) > { > - struct acpi_handle_list devices; > + struct acpi_handle_list devices = { 0 }; > char method[] = "_PSL"; > acpi_status status; > > @@ -218,18 +218,21 @@ static bool update_trip_devices(struct a > method[3] = '0' + index; > } > > - 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, "%s evaluation failure\n", method); > return false; > } > > - if (compare && memcmp(&acpi_trip->devices, &devices, sizeof(devices))) > + if (acpi_handle_list_equal(&acpi_trip->devices, &devices)) { > + acpi_handle_list_free(&devices); > + return true; > + } > + > + if (compare) > ACPI_THERMAL_TRIPS_EXCEPTION(tz, "device"); > > - memcpy(&acpi_trip->devices, &devices, sizeof(devices)); > + acpi_handle_list_replace(&acpi_trip->devices, &devices); > return true; > } > > @@ -845,6 +848,17 @@ static void acpi_thermal_check_fn(struct > mutex_unlock(&tz->thermal_check_lock); > } > > +static void acpi_thermal_free_thermal_zone(struct acpi_thermal *tz) > +{ > + int i; > + > + acpi_handle_list_free(&tz->trips.passive.trip.devices); > + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) > + acpi_handle_list_free(&tz->trips.active[i].trip.devices); > + > + kfree(tz); > +} > + > static int acpi_thermal_add(struct acpi_device *device) > { > struct acpi_thermal_trip *acpi_trip; > @@ -971,7 +985,7 @@ flush_wq: > free_trips: > kfree(tz->trip_table); > free_memory: > - kfree(tz); > + acpi_thermal_free_thermal_zone(tz); > > return result; > } > @@ -991,7 +1005,7 @@ static void acpi_thermal_remove(struct a > flush_workqueue(acpi_thermal_pm_queue); > acpi_thermal_unregister_thermal_zone(tz); > > - kfree(tz); > + acpi_thermal_free_thermal_zone(tz); > } > > #ifdef CONFIG_PM_SLEEP > Index: linux-pm/drivers/acpi/utils.c > =================================================================== > --- linux-pm.orig/drivers/acpi/utils.c > +++ linux-pm/drivers/acpi/utils.c > @@ -370,7 +370,8 @@ acpi_evaluate_reference(acpi_handle hand > goto end; > } > > - if (package->package.count > ACPI_MAX_HANDLES) { > + list->handles = kcalloc(package->package.count, sizeof(*list->handles), GFP_KERNEL); > + if (!list->handles) { > kfree(package); > return AE_NO_MEMORY; > } > @@ -402,7 +403,8 @@ acpi_evaluate_reference(acpi_handle hand > end: > if (ACPI_FAILURE(status)) { > list->count = 0; > - //kfree(list->handles); > + kfree(list->handles); > + list->handles = NULL; > } > > kfree(buffer.pointer); > @@ -412,6 +414,61 @@ acpi_evaluate_reference(acpi_handle hand > > EXPORT_SYMBOL(acpi_evaluate_reference); > > +/** > + * acpi_handle_list_equal - Check if two ACPI handle lists are the same > + * @list1: First list to compare. > + * @List2: Second list to compare. > + * > + * Return true if the given ACPI handle lists are of the same size and > + * contain the same ACPI handles in the same order. Otherwise, return false. > + */ > +bool acpi_handle_list_equal(struct acpi_handle_list *list1, > + struct acpi_handle_list *list2) > +{ > + return list1->count == list2->count && > + !memcmp(list1->handles, list2->handles, > + list1->count * sizeof(acpi_handle)); > +} > +EXPORT_SYMBOL_GPL(acpi_handle_list_equal); Why is this exported as GPL? I was under the impression this was intended to be used for things that touched the deep guts of Linux, and thus aren't really separable, not a default> I don't think utility functions in the ACPI code qualify as this qualification. The convention in the rest of the file, save for three functions, appears to be non-GPL as well. > + > +/** > + * acpi_handle_list_replace - Replace one ACPI handle list with another > + * @dst: ACPI handle list to replace. > + * @src: Source ACPI handle list. > + * > + * Free the handles table in @dst, move the handles table from @src to @dst, > + * copy count from @src to @dst and clear @src. > + */ > +void acpi_handle_list_replace(struct acpi_handle_list *dst, > + struct acpi_handle_list *src) > +{ > + if (dst->count) > + kfree(dst->handles); > + > + dst->count = src->count; > + dst->handles = src->handles; > + > + src->handles = NULL; > + src->count = 0; > +} > +EXPORT_SYMBOL_GPL(acpi_handle_list_replace); Same here > + > +/** > + * acpi_handle_list_free - Free the handles table in an ACPI handle list > + * @list: ACPI handle list to free. > + * > + * Free the handles table in @list and clear its count field. > + */ > +void acpi_handle_list_free(struct acpi_handle_list *list) > +{ > + if (!list->count) > + return; > + > + kfree(list->handles); > + list->count = 0; > +} > +EXPORT_SYMBOL_GPL(acpi_handle_list_free); Same here > + > acpi_status > acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld) > { > Index: linux-pm/drivers/platform/surface/surface_acpi_notify.c > =================================================================== > --- linux-pm.orig/drivers/platform/surface/surface_acpi_notify.c > +++ linux-pm/drivers/platform/surface/surface_acpi_notify.c > @@ -741,6 +741,7 @@ static bool is_san_consumer(struct platf > struct acpi_handle_list dep_devices; > acpi_handle supplier = ACPI_HANDLE(&pdev->dev); > acpi_status status; > + bool ret = false; > int i; > > if (!acpi_has_method(handle, "_DEP")) > @@ -753,11 +754,14 @@ static bool is_san_consumer(struct platf > } > > for (i = 0; i < dep_devices.count; i++) { > - if (dep_devices.handles[i] == supplier) > - return true; > + if (dep_devices.handles[i] == supplier) { > + ret = true; > + break; > + } > } > > - return false; > + acpi_handle_list_free(&dep_devices); > + return ret; > } > > static acpi_status san_consumer_setup(acpi_handle handle, u32 lvl, > Index: linux-pm/include/acpi/acpi_bus.h > =================================================================== > --- linux-pm.orig/include/acpi/acpi_bus.h > +++ linux-pm/include/acpi/acpi_bus.h > @@ -12,11 +12,9 @@ > #include <linux/device.h> > #include <linux/property.h> > > -/* TBD: Make dynamic */ > -#define ACPI_MAX_HANDLES 10 > struct acpi_handle_list { > u32 count; > - acpi_handle handles[ACPI_MAX_HANDLES]; > + acpi_handle* handles; > }; > > /* acpi_utils.h */ > @@ -32,6 +30,11 @@ acpi_evaluate_reference(acpi_handle hand > acpi_string pathname, > struct acpi_object_list *arguments, > struct acpi_handle_list *list); > +bool acpi_handle_list_equal(struct acpi_handle_list *list1, > + struct acpi_handle_list *list2); > +void acpi_handle_list_replace(struct acpi_handle_list *dst, > + struct acpi_handle_list *src); > +void acpi_handle_list_free(struct acpi_handle_list *list); > acpi_status > acpi_evaluate_ost(acpi_handle handle, u32 source_event, u32 status_code, > struct acpi_buffer *status_buf); > > > Thanks for fixing it up for me. Looks fine otherwise, at least as far as my not-quite-familiar-with-this-subsystem eyes go. Vicki
Hi Vicki, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.6] [cannot apply to rafael-pm/linux-next next-20231106] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Vicki-Pfau/ACPI-utils-Make-acpi_handle_list-dynamically-allocated/20230928-043202 base: linus/master patch link: https://lore.kernel.org/r/20230927201725.2339488-1-vi%40endrift.com patch subject: [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated config: x86_64-randconfig-123-20230930 (https://download.01.org/0day-ci/archive/20231106/202311061141.kEqwdu4M-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231106/202311061141.kEqwdu4M-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311061141.kEqwdu4M-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/acpi/thermal.c:310:54: sparse: sparse: incompatible types for operation (!=): >> drivers/acpi/thermal.c:310:54: sparse: unsigned int * >> drivers/acpi/thermal.c:310:54: sparse: unsigned int [addressable] [usertype] count drivers/acpi/thermal.c:377:64: sparse: sparse: incompatible types for operation (!=): drivers/acpi/thermal.c:377:64: sparse: unsigned int * drivers/acpi/thermal.c:377:64: sparse: unsigned int [addressable] [usertype] count vim +310 drivers/acpi/thermal.c 190 191 static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) 192 { 193 acpi_status status; 194 unsigned long long tmp; 195 struct acpi_handle_list devices; 196 bool valid = false; 197 int i; 198 199 /* Critical Shutdown */ 200 if (flag & ACPI_TRIPS_CRITICAL) { 201 status = acpi_evaluate_integer(tz->device->handle, "_CRT", NULL, &tmp); 202 tz->trips.critical.temperature = tmp; 203 /* 204 * Treat freezing temperatures as invalid as well; some 205 * BIOSes return really low values and cause reboots at startup. 206 * Below zero (Celsius) values clearly aren't right for sure.. 207 * ... so lets discard those as invalid. 208 */ 209 if (ACPI_FAILURE(status)) { 210 tz->trips.critical.valid = false; 211 acpi_handle_debug(tz->device->handle, 212 "No critical threshold\n"); 213 } else if (tmp <= 2732) { 214 pr_info(FW_BUG "Invalid critical threshold (%llu)\n", tmp); 215 tz->trips.critical.valid = false; 216 } else { 217 tz->trips.critical.valid = true; 218 acpi_handle_debug(tz->device->handle, 219 "Found critical threshold [%lu]\n", 220 tz->trips.critical.temperature); 221 } 222 if (tz->trips.critical.valid) { 223 if (crt == -1) { 224 tz->trips.critical.valid = false; 225 } else if (crt > 0) { 226 unsigned long crt_k = celsius_to_deci_kelvin(crt); 227 228 /* 229 * Allow override critical threshold 230 */ 231 if (crt_k > tz->trips.critical.temperature) 232 pr_info("Critical threshold %d C\n", crt); 233 234 tz->trips.critical.temperature = crt_k; 235 } 236 } 237 } 238 239 /* Critical Sleep (optional) */ 240 if (flag & ACPI_TRIPS_HOT) { 241 status = acpi_evaluate_integer(tz->device->handle, "_HOT", NULL, &tmp); 242 if (ACPI_FAILURE(status)) { 243 tz->trips.hot.valid = false; 244 acpi_handle_debug(tz->device->handle, 245 "No hot threshold\n"); 246 } else { 247 tz->trips.hot.temperature = tmp; 248 tz->trips.hot.valid = true; 249 acpi_handle_debug(tz->device->handle, 250 "Found hot threshold [%lu]\n", 251 tz->trips.hot.temperature); 252 } 253 } 254 255 /* Passive (optional) */ 256 if (((flag & ACPI_TRIPS_PASSIVE) && tz->trips.passive.trip.valid) || 257 flag == ACPI_TRIPS_INIT) { 258 valid = tz->trips.passive.trip.valid; 259 if (psv == -1) { 260 status = AE_SUPPORT; 261 } else if (psv > 0) { 262 tmp = celsius_to_deci_kelvin(psv); 263 status = AE_OK; 264 } else { 265 status = acpi_evaluate_integer(tz->device->handle, 266 "_PSV", NULL, &tmp); 267 } 268 269 if (ACPI_FAILURE(status)) { 270 tz->trips.passive.trip.valid = false; 271 } else { 272 tz->trips.passive.trip.temperature = tmp; 273 tz->trips.passive.trip.valid = true; 274 if (flag == ACPI_TRIPS_INIT) { 275 status = acpi_evaluate_integer(tz->device->handle, 276 "_TC1", NULL, &tmp); 277 if (ACPI_FAILURE(status)) 278 tz->trips.passive.trip.valid = false; 279 else 280 tz->trips.passive.tc1 = tmp; 281 282 status = acpi_evaluate_integer(tz->device->handle, 283 "_TC2", NULL, &tmp); 284 if (ACPI_FAILURE(status)) 285 tz->trips.passive.trip.valid = false; 286 else 287 tz->trips.passive.tc2 = tmp; 288 289 status = acpi_evaluate_integer(tz->device->handle, 290 "_TSP", NULL, &tmp); 291 if (ACPI_FAILURE(status)) 292 tz->trips.passive.trip.valid = false; 293 else 294 tz->trips.passive.tsp = tmp; 295 } 296 } 297 } 298 if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.passive.trip.valid) { 299 memset(&devices, 0, sizeof(struct acpi_handle_list)); 300 status = acpi_evaluate_reference(tz->device->handle, "_PSL", 301 NULL, &devices); 302 if (ACPI_FAILURE(status)) { 303 acpi_handle_info(tz->device->handle, 304 "Invalid passive threshold\n"); 305 tz->trips.passive.trip.valid = false; 306 } else { 307 tz->trips.passive.trip.valid = true; 308 } 309 > 310 if (&tz->trips.passive.devices.count != devices.count || 311 memcmp(tz->trips.passive.devices.handles, 312 devices.handles, sizeof(acpi_handle) * devices.count)) { 313 kfree(tz->trips.passive.devices.handles); 314 memcpy(&tz->trips.passive.devices, &devices, 315 sizeof(struct acpi_handle_list)); 316 ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); 317 } 318 } 319 if ((flag & ACPI_TRIPS_PASSIVE) || (flag & ACPI_TRIPS_DEVICES)) { 320 if (valid != tz->trips.passive.trip.valid) 321 ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state"); 322 } 323 324 /* Active (optional) */ 325 for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) { 326 char name[5] = { '_', 'A', 'C', ('0' + i), '\0' }; 327 valid = tz->trips.active[i].trip.valid; 328 329 if (act == -1) 330 break; /* disable all active trip points */ 331 332 if (flag == ACPI_TRIPS_INIT || ((flag & ACPI_TRIPS_ACTIVE) && 333 tz->trips.active[i].trip.valid)) { 334 status = acpi_evaluate_integer(tz->device->handle, 335 name, NULL, &tmp); 336 if (ACPI_FAILURE(status)) { 337 tz->trips.active[i].trip.valid = false; 338 if (i == 0) 339 break; 340 341 if (act <= 0) 342 break; 343 344 if (i == 1) 345 tz->trips.active[0].trip.temperature = 346 celsius_to_deci_kelvin(act); 347 else 348 /* 349 * Don't allow override higher than 350 * the next higher trip point 351 */ 352 tz->trips.active[i-1].trip.temperature = 353 min_t(unsigned long, 354 tz->trips.active[i-2].trip.temperature, 355 celsius_to_deci_kelvin(act)); 356 357 break; 358 } else { 359 tz->trips.active[i].trip.temperature = tmp; 360 tz->trips.active[i].trip.valid = true; 361 } 362 } 363 364 name[2] = 'L'; 365 if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.active[i].trip.valid) { 366 memset(&devices, 0, sizeof(struct acpi_handle_list)); 367 status = acpi_evaluate_reference(tz->device->handle, 368 name, NULL, &devices); 369 if (ACPI_FAILURE(status)) { 370 acpi_handle_info(tz->device->handle, 371 "Invalid active%d threshold\n", i); 372 tz->trips.active[i].trip.valid = false; 373 } else { 374 tz->trips.active[i].trip.valid = true; 375 } 376 377 if (&tz->trips.active[i].devices.count != devices.count || 378 memcmp(tz->trips.active[i].devices.handles, 379 devices.handles, sizeof(acpi_handle) * devices.count)) { 380 kfree(tz->trips.active[i].devices.handles); 381 memcpy(&tz->trips.active[i].devices, &devices, 382 sizeof(struct acpi_handle_list)); 383 ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); 384 } 385 } 386 if ((flag & ACPI_TRIPS_ACTIVE) || (flag & ACPI_TRIPS_DEVICES)) 387 if (valid != tz->trips.active[i].trip.valid) 388 ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "state"); 389 390 if (!tz->trips.active[i].trip.valid) 391 break; 392 } 393 394 if (flag & ACPI_TRIPS_DEVICES) { 395 memset(&devices, 0, sizeof(devices)); 396 status = acpi_evaluate_reference(tz->device->handle, "_TZD", 397 NULL, &devices); 398 if (ACPI_SUCCESS(status) && (tz->devices.count != devices.count || 399 memcmp(tz->devices.handles, devices.handles, 400 sizeof(acpi_handle) * devices.count))) { 401 kfree(tz->devices.handles); 402 tz->devices = devices; 403 ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); 404 } 405 } 406 } 407
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 539e700de4d2..4b3aa84faf70 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -591,10 +591,13 @@ static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) } for (i = 0; i < dep_devices.count; i++) { - if (dep_devices.handles[i] == handle) + if (dep_devices.handles[i] == handle) { + kfree(dep_devices.handles); return true; + } } + kfree(dep_devices.handles); return false; } diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 691d4b7686ee..2fbe354db0c0 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2032,6 +2032,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep) mutex_unlock(&acpi_dep_list_lock); } + kfree(dep_devices.handles); return count; } diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index 312730f8272e..48ddc56f96f6 100644 --- a/drivers/acpi/thermal.c +++ b/drivers/acpi/thermal.c @@ -307,8 +307,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) tz->trips.passive.trip.valid = true; } - if (memcmp(&tz->trips.passive.devices, &devices, - sizeof(struct acpi_handle_list))) { + if (&tz->trips.passive.devices.count != devices.count || + memcmp(tz->trips.passive.devices.handles, + devices.handles, sizeof(acpi_handle) * devices.count)) { + kfree(tz->trips.passive.devices.handles); memcpy(&tz->trips.passive.devices, &devices, sizeof(struct acpi_handle_list)); ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); @@ -372,8 +374,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) tz->trips.active[i].trip.valid = true; } - if (memcmp(&tz->trips.active[i].devices, &devices, - sizeof(struct acpi_handle_list))) { + if (&tz->trips.active[i].devices.count != devices.count || + memcmp(tz->trips.active[i].devices.handles, + devices.handles, sizeof(acpi_handle) * devices.count)) { + kfree(tz->trips.active[i].devices.handles); memcpy(&tz->trips.active[i].devices, &devices, sizeof(struct acpi_handle_list)); ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); @@ -391,8 +395,10 @@ static void __acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) 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))) { + if (ACPI_SUCCESS(status) && (tz->devices.count != devices.count || + memcmp(tz->devices.handles, devices.handles, + sizeof(acpi_handle) * devices.count))) { + kfree(tz->devices.handles); tz->devices = devices; ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); } @@ -974,6 +980,7 @@ static int acpi_thermal_add(struct acpi_device *device) static void acpi_thermal_remove(struct acpi_device *device) { struct acpi_thermal *tz; + int i; if (!device || !acpi_driver_data(device)) return; @@ -986,6 +993,10 @@ static void acpi_thermal_remove(struct acpi_device *device) flush_workqueue(acpi_thermal_pm_queue); acpi_thermal_unregister_thermal_zone(tz); + kfree(tz->trips.passive.devices.handles); + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) + kfree(tz->trips.active[i].devices.handles); + kfree(tz->devices.handles); kfree(tz); } diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 2ea14648a661..96f821c41756 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -370,7 +370,8 @@ acpi_evaluate_reference(acpi_handle handle, goto end; } - if (package->package.count > ACPI_MAX_HANDLES) { + list->handles = kcalloc(package->package.count, sizeof(*list->handles), GFP_KERNEL); + if (!list->handles) { kfree(package); return AE_NO_MEMORY; } @@ -402,7 +403,7 @@ acpi_evaluate_reference(acpi_handle handle, end: if (ACPI_FAILURE(status)) { list->count = 0; - //kfree(list->handles); + kfree(list->handles); } kfree(buffer.pointer); diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c index 897cdd9c3aae..6932d4b35c6c 100644 --- a/drivers/platform/surface/surface_acpi_notify.c +++ b/drivers/platform/surface/surface_acpi_notify.c @@ -753,10 +753,13 @@ static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle) } for (i = 0; i < dep_devices.count; i++) { - if (dep_devices.handles[i] == supplier) + if (dep_devices.handles[i] == supplier) { + kfree(dep_devices.handles); return true; + } } + kfree(dep_devices.handles); return false; } diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 254685085c82..b4bf12343a22 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -12,11 +12,9 @@ #include <linux/device.h> #include <linux/property.h> -/* TBD: Make dynamic */ -#define ACPI_MAX_HANDLES 10 struct acpi_handle_list { u32 count; - acpi_handle handles[ACPI_MAX_HANDLES]; + acpi_handle* handles; }; /* acpi_utils.h */
This fixes a long-standing "TBD" comment in the ACPI headers regarding making the acpi_handle_list struct's size dynamic. The number 10, which along with the comment dates back to 2.4.23, seems like it may have been arbitrarily chosen, and isn't sufficient in all cases. This patch finally makes the size dynamic and updates its users to handle the modified API. Signed-off-by: Vicki Pfau <vi@endrift.com> --- drivers/acpi/acpi_lpss.c | 5 +++- drivers/acpi/scan.c | 1 + drivers/acpi/thermal.c | 23 ++++++++++++++----- drivers/acpi/utils.c | 5 ++-- .../platform/surface/surface_acpi_notify.c | 5 +++- include/acpi/acpi_bus.h | 4 +--- 6 files changed, 30 insertions(+), 13 deletions(-)