* [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated
@ 2023-09-27 20:17 Vicki Pfau
2023-09-28 15:40 ` Rafael J. Wysocki
2023-11-06 4:26 ` kernel test robot
0 siblings, 2 replies; 4+ messages in thread
From: Vicki Pfau @ 2023-09-27 20:17 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-acpi, Len Brown; +Cc: Vicki Pfau
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(-)
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 */
--
2.42.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated
2023-09-27 20:17 [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated Vicki Pfau
@ 2023-09-28 15:40 ` Rafael J. Wysocki
2023-09-28 21:06 ` Vicki Pfau
2023-11-06 4:26 ` kernel test robot
1 sibling, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2023-09-28 15:40 UTC (permalink / raw)
To: Vicki Pfau; +Cc: Rafael J . Wysocki, linux-acpi, Len Brown
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);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated
2023-09-28 15:40 ` Rafael J. Wysocki
@ 2023-09-28 21:06 ` Vicki Pfau
0 siblings, 0 replies; 4+ messages in thread
From: Vicki Pfau @ 2023-09-28 21:06 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J . Wysocki, linux-acpi, Len Brown
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated
2023-09-27 20:17 [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated Vicki Pfau
2023-09-28 15:40 ` Rafael J. Wysocki
@ 2023-11-06 4:26 ` kernel test robot
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2023-11-06 4:26 UTC (permalink / raw)
To: Vicki Pfau, Rafael J . Wysocki, linux-acpi, Len Brown
Cc: oe-kbuild-all, Vicki Pfau
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-06 4:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-27 20:17 [PATCH v2] ACPI: utils: Make acpi_handle_list dynamically allocated Vicki Pfau
2023-09-28 15:40 ` Rafael J. Wysocki
2023-09-28 21:06 ` Vicki Pfau
2023-11-06 4:26 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox