| Message ID | 20240119042407.185798-2-ivan.hu@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Dynamically determine acpi_handle_list size | expand |
The patch "ACPI: utils: Dynamically determine acpi_handle_list size" introduces a bug that was later fixed by the patch "ACPI: utils: Fix error path in acpi_evaluate_reference()" [1]. The two patches should probably be submitted together. Jacob [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.7&id=8f0b960a42badda7a2781e8a33564624200debc9 On Fri, Jan 19, 2024 at 12:24:07PM +0800, Ivan Hu wrote: > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > > BugLink: https://bugs.launchpad.net/bugs/2049733 > > 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> > (cherry picked from commit 2e57d10a6591560724b80a628235559571f4cb8d) > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > drivers/acpi/acpi_lpss.c | 10 ++- > drivers/acpi/scan.c | 1 + > drivers/acpi/thermal.c | 29 ++++++--- > drivers/acpi/utils.c | 61 ++++++++++++++++++- > .../platform/surface/surface_acpi_notify.c | 10 ++- > include/acpi/acpi_bus.h | 9 ++- > 6 files changed, 100 insertions(+), 20 deletions(-) > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c > index 539e700de4d2..3e756b9b5aa6 100644 > --- a/drivers/acpi/acpi_lpss.c > +++ b/drivers/acpi/acpi_lpss.c > @@ -578,6 +578,7 @@ static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > { > 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_device *adev, acpi_handle handle) > } > > 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, > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 87e385542576..e3ea3868611b 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -2029,6 +2029,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep) > mutex_unlock(&acpi_dep_list_lock); > } > > + acpi_handle_list_free(&dep_devices); > return count; > } > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 3163a40f02e3..91aeea3ad766 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -190,7 +190,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > { > acpi_status status; > unsigned long long tmp; > - struct acpi_handle_list devices; > + struct acpi_handle_list devices = { 0 }; > bool valid = false; > int i; > > @@ -294,7 +294,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > } > } > if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.passive.valid) { > - memset(&devices, 0, sizeof(struct acpi_handle_list)); > + > status = acpi_evaluate_reference(tz->device->handle, "_PSL", > NULL, &devices); > if (ACPI_FAILURE(status)) { > @@ -305,12 +305,12 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) > tz->trips.passive.valid = true; > } > > - if (memcmp(&tz->trips.passive.devices, &devices, > - sizeof(struct acpi_handle_list))) { > - memcpy(&tz->trips.passive.devices, &devices, > - sizeof(struct acpi_handle_list)); > - ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > + if (acpi_handle_list_equal(&tz->trips.passive.devices, &devices)) { > + acpi_handle_list_free(&devices); > + return 0; > } > + ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); > + acpi_handle_list_replace(&tz->trips.passive.devices, &devices); > } > if ((flag & ACPI_TRIPS_PASSIVE) || (flag & ACPI_TRIPS_DEVICES)) { > if (valid != tz->trips.passive.valid) > @@ -959,6 +959,17 @@ static void acpi_thermal_check_fn(struct work_struct *work) > 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.devices); > + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) > + acpi_handle_list_free(&tz->trips.active[i].devices); > + > + kfree(tz); > +} > + > static int acpi_thermal_add(struct acpi_device *device) > { > struct acpi_thermal *tz; > @@ -996,7 +1007,7 @@ static int acpi_thermal_add(struct acpi_device *device) > goto end; > > free_memory: > - kfree(tz); > + acpi_thermal_free_thermal_zone(tz); > end: > return result; > } > @@ -1012,7 +1023,7 @@ static void acpi_thermal_remove(struct acpi_device *device) > tz = acpi_driver_data(device); > > acpi_thermal_unregister_thermal_zone(tz); > - kfree(tz); > + acpi_thermal_free_thermal_zone(tz); > } > > #ifdef CONFIG_PM_SLEEP > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 2ea14648a661..c6f83c21bb2a 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,8 @@ acpi_evaluate_reference(acpi_handle handle, > 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 handle, > > 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) > { > diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c > index 897cdd9c3aae..0412a644fece 100644 > --- a/drivers/platform/surface/surface_acpi_notify.c > +++ b/drivers/platform/surface/surface_acpi_notify.c > @@ -741,6 +741,7 @@ static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle) > 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 platform_device *pdev, acpi_handle handle) > } > > 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, > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index fb95fff23e5b..9a4782d3e738 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 */ > @@ -32,6 +30,11 @@ acpi_evaluate_reference(acpi_handle handle, > 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); > -- > 2.34.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 539e700de4d2..3e756b9b5aa6 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -578,6 +578,7 @@ static bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) { 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_device *adev, acpi_handle handle) } 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, diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 87e385542576..e3ea3868611b 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -2029,6 +2029,7 @@ static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep) mutex_unlock(&acpi_dep_list_lock); } + acpi_handle_list_free(&dep_devices); return count; } diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index 3163a40f02e3..91aeea3ad766 100644 --- a/drivers/acpi/thermal.c +++ b/drivers/acpi/thermal.c @@ -190,7 +190,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) { acpi_status status; unsigned long long tmp; - struct acpi_handle_list devices; + struct acpi_handle_list devices = { 0 }; bool valid = false; int i; @@ -294,7 +294,7 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) } } if ((flag & ACPI_TRIPS_DEVICES) && tz->trips.passive.valid) { - memset(&devices, 0, sizeof(struct acpi_handle_list)); + status = acpi_evaluate_reference(tz->device->handle, "_PSL", NULL, &devices); if (ACPI_FAILURE(status)) { @@ -305,12 +305,12 @@ static int acpi_thermal_trips_update(struct acpi_thermal *tz, int flag) tz->trips.passive.valid = true; } - if (memcmp(&tz->trips.passive.devices, &devices, - sizeof(struct acpi_handle_list))) { - memcpy(&tz->trips.passive.devices, &devices, - sizeof(struct acpi_handle_list)); - ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); + if (acpi_handle_list_equal(&tz->trips.passive.devices, &devices)) { + acpi_handle_list_free(&devices); + return 0; } + ACPI_THERMAL_TRIPS_EXCEPTION(flag, tz, "device"); + acpi_handle_list_replace(&tz->trips.passive.devices, &devices); } if ((flag & ACPI_TRIPS_PASSIVE) || (flag & ACPI_TRIPS_DEVICES)) { if (valid != tz->trips.passive.valid) @@ -959,6 +959,17 @@ static void acpi_thermal_check_fn(struct work_struct *work) 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.devices); + for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) + acpi_handle_list_free(&tz->trips.active[i].devices); + + kfree(tz); +} + static int acpi_thermal_add(struct acpi_device *device) { struct acpi_thermal *tz; @@ -996,7 +1007,7 @@ static int acpi_thermal_add(struct acpi_device *device) goto end; free_memory: - kfree(tz); + acpi_thermal_free_thermal_zone(tz); end: return result; } @@ -1012,7 +1023,7 @@ static void acpi_thermal_remove(struct acpi_device *device) tz = acpi_driver_data(device); acpi_thermal_unregister_thermal_zone(tz); - kfree(tz); + acpi_thermal_free_thermal_zone(tz); } #ifdef CONFIG_PM_SLEEP diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 2ea14648a661..c6f83c21bb2a 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,8 @@ acpi_evaluate_reference(acpi_handle handle, 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 handle, 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) { diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c index 897cdd9c3aae..0412a644fece 100644 --- a/drivers/platform/surface/surface_acpi_notify.c +++ b/drivers/platform/surface/surface_acpi_notify.c @@ -741,6 +741,7 @@ static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle) 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 platform_device *pdev, acpi_handle handle) } 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, diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index fb95fff23e5b..9a4782d3e738 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 */ @@ -32,6 +30,11 @@ acpi_evaluate_reference(acpi_handle handle, 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);