2018-05-28 09:10:30

by Dou Liyang

[permalink] [raw]
Subject: [RESEND RFC PATCH] acpi/processor: Remove the check of duplicate processors

We found there are some processors which have the same processor ID
but in different PXM in the ACPI namespace. such as this:

proc_id | pxm
--------------------
0 <-> 0
1 <-> 0
2 <-> 1
3 <-> 1
......
89 <-> 0
89 <-> 1
89 <-> 2
89 <-> 3
......

So we create a mechanism to validate them. make the processor(ID=89)
as invalid. And once a processor be hotplugged physically, we check its
processor id.

Commit 8e089eaa1999 ("acpi: Provide mechanism to validate processors in the ACPI tables")
Commit a77d6cd96849 ("acpi/processor: Check for duplicate processor ids at hotplug time")

Recently, I found the check mechanism has a bug, it didn't use the
acpi_processor_ids_walk() right and always gave us a wrong result.
First, I fixed it by modifying the value with AE_OK which is the
standard acpi_status value.

https://lkml.org/lkml/2018/3/20/273

But, now, I even think this check is useless. my reasons are following:

1). Based on the practical tests, It works well, and no bug be reported
2). Based on the code, the duplicate cases can be dealed with by

if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id))

That seems more reasonable, let's see the following case:

Before the patch, After the patch

the first processor(ID=89)
hot-add failed success

the others processor(ID=89)
hot-add failed failed

So, Remove the check code.

Signed-off-by: Dou Liyang <[email protected]>

Signed-off-by: Dou Liyang <[email protected]>
---
drivers/acpi/acpi_processor.c | 126 ------------------------------------------
include/linux/acpi.h | 3 -
2 files changed, 129 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 449d86d39965..8358708e0bbb 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -281,13 +281,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
pr->acpi_id = value;
}

- if (acpi_duplicate_processor_id(pr->acpi_id)) {
- dev_err(&device->dev,
- "Failed to get unique processor _UID (0x%x)\n",
- pr->acpi_id);
- return -ENODEV;
- }
-
pr->phys_id = acpi_get_phys_id(pr->handle, device_declaration,
pr->acpi_id);
if (invalid_phys_cpuid(pr->phys_id))
@@ -579,127 +572,8 @@ static struct acpi_scan_handler processor_container_handler = {
.attach = acpi_processor_container_attach,
};

-/* The number of the unique processor IDs */
-static int nr_unique_ids __initdata;
-
-/* The number of the duplicate processor IDs */
-static int nr_duplicate_ids;
-
-/* Used to store the unique processor IDs */
-static int unique_processor_ids[] __initdata = {
- [0 ... NR_CPUS - 1] = -1,
-};
-
-/* Used to store the duplicate processor IDs */
-static int duplicate_processor_ids[] = {
- [0 ... NR_CPUS - 1] = -1,
-};
-
-static void __init processor_validated_ids_update(int proc_id)
-{
- int i;
-
- if (nr_unique_ids == NR_CPUS||nr_duplicate_ids == NR_CPUS)
- return;
-
- /*
- * Firstly, compare the proc_id with duplicate IDs, if the proc_id is
- * already in the IDs, do nothing.
- */
- for (i = 0; i < nr_duplicate_ids; i++) {
- if (duplicate_processor_ids[i] == proc_id)
- return;
- }
-
- /*
- * Secondly, compare the proc_id with unique IDs, if the proc_id is in
- * the IDs, put it in the duplicate IDs.
- */
- for (i = 0; i < nr_unique_ids; i++) {
- if (unique_processor_ids[i] == proc_id) {
- duplicate_processor_ids[nr_duplicate_ids] = proc_id;
- nr_duplicate_ids++;
- return;
- }
- }
-
- /*
- * Lastly, the proc_id is a unique ID, put it in the unique IDs.
- */
- unique_processor_ids[nr_unique_ids] = proc_id;
- nr_unique_ids++;
-}
-
-static acpi_status __init acpi_processor_ids_walk(acpi_handle handle,
- u32 lvl,
- void *context,
- void **rv)
-{
- acpi_status status;
- acpi_object_type acpi_type;
- unsigned long long uid;
- union acpi_object object = { 0 };
- struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
-
- status = acpi_get_type(handle, &acpi_type);
- if (ACPI_FAILURE(status))
- return false;
-
- switch (acpi_type) {
- case ACPI_TYPE_PROCESSOR:
- status = acpi_evaluate_object(handle, NULL, NULL, &buffer);
- if (ACPI_FAILURE(status))
- goto err;
- uid = object.processor.proc_id;
- break;
-
- case ACPI_TYPE_DEVICE:
- status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
- if (ACPI_FAILURE(status))
- goto err;
- break;
- default:
- goto err;
- }
-
- processor_validated_ids_update(uid);
- return true;
-
-err:
- acpi_handle_info(handle, "Invalid processor object\n");
- return false;
-
-}
-
-static void __init acpi_processor_check_duplicates(void)
-{
- /* check the correctness for all processors in ACPI namespace */
- acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
- ACPI_UINT32_MAX,
- acpi_processor_ids_walk,
- NULL, NULL, NULL);
- acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, acpi_processor_ids_walk,
- NULL, NULL);
-}
-
-bool acpi_duplicate_processor_id(int proc_id)
-{
- int i;
-
- /*
- * compare the proc_id with duplicate IDs, if the proc_id is already
- * in the duplicate IDs, return true, otherwise, return false.
- */
- for (i = 0; i < nr_duplicate_ids; i++) {
- if (duplicate_processor_ids[i] == proc_id)
- return true;
- }
- return false;
-}
-
void __init acpi_processor_init(void)
{
- acpi_processor_check_duplicates();
acpi_scan_add_handler_with_hotplug(&processor_handler, "processor");
acpi_scan_add_handler(&processor_container_handler);
}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 15bfb15c2fa5..068dcfe6768b 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -285,9 +285,6 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
return phys_id == PHYS_CPUID_INVALID;
}

-/* Validate the processor object's proc_id */
-bool acpi_duplicate_processor_id(int proc_id);
-
#ifdef CONFIG_ACPI_HOTPLUG_CPU
/* Arch dependent functions for cpu hotplug support */
int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
--
2.14.3