NACK: [SRU][M][PATCH 1/1] ACPI: utils: Dynamically determine acpi_handle_list size
Jacob Martin
jacob.martin at canonical.com
Fri Jan 19 20:56:44 UTC 2024
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 at 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 at endrift.com>
> Signed-off-by: Vicki Pfau <vi at endrift.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> (cherry picked from commit 2e57d10a6591560724b80a628235559571f4cb8d)
> Signed-off-by: Ivan Hu <ivan.hu at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list