[RFC PATCH] ACPI / processors: allow a processor device _UID to be a string

* [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string
@ 2019-06-10 20:07 Al Stone
  2019-06-11 12:53 ` Sudeep Holla
  0 siblings, 1 reply; 6+ messages in thread
From: Al Stone @ 2019-06-10 20:07 UTC (permalink / raw)
  To: linux-acpi, linux-kernel; +Cc: ahs3

In the ACPI specification, section 6.1.12, a _UID may be either an
integer or a string object.  Up until now, when defining processor
Device()s in ACPI (_HID ACPI0007), only integers were allowed even
though this ignored the specification.  As a practical matter, it
was not an issue.

Recently, some DSDTs have shown up that look like this:

  Device (XX00)
  {
	Name (_HID, "ACPI0007" /* Processor Device */)
        Name (_UID, "XYZZY-XX00")
        .....
  }

which is perfectly legal.  However, the kernel will report instead:

...
[    0.706970] ACPI: \_SB_.XXXX.XX00: Invalid processor object
...
[    0.776998] acpi ACPI0007:xx: Failed to evaluate processor _UID (0xc)
...

The second message comes from acpi_processor_get_info() not being
able to use the string value for _UID that was provided in the DSDT;
the 0xc in the message is actually the buffer size of the returned
value from trying to evaluate _UID.

We correct this by first adding a field called acpi_name to struct
acpi_processor.  Then we add the function acpi_processor_evaluate_uid()
to get the returned _UID object and, if it is an integer, behave exactly
as acpi_processor_get_info() did before, but if it is a string, assign
it an arbitrary integer value (required by all the users of this
struct) and capture the actual string used.  This results in the
functions acpi_processor_add() and acpi_processor_remove() changing
also so that we do not inadvertently forget to free the space used
for the new acpi_name field.

Why RFC and not a straight patch?  I have some concerns that need broader
knowledge than I have at my disposal:

   1) Is there a need to expose the CPU name captured from the ACPI
      namespace in sysfs (or elsewhere)?  I could not think of any
      good reason but I probably just missed something.

   2) Is making last_used_string_cpuid atomic required?  I think it
      is, but only because I think acpi_processor_add() could get called
      on multiple CPUs at roughly the same time.  If that never happens,
      an int would be just fine.

   3) Is it necessary to check that a _UID string is a duplicate of the
      _UID string of some other CPU Device() (_HID ACPI0007)?  The code
      will ensure unique integer IDs, and the firmware writer should not
      have provided duplicate string _UIDs, but things happen.  I would
      need to add this code to this patch.

   4) Testing: I only have one very fragile machine to test this on (the
      firmware is really, really unstable right now); more testing would
      be greatly appreciated.

Signed-off-by: Al Stone <ahs3@redhat.com>
---
 drivers/acpi/acpi_processor.c | 57 ++++++++++++++++++++++++++++++-----
 include/acpi/processor.h      |  1 +
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 449d86d39965..f40163e0edf9 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -14,6 +14,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -32,6 +33,8 @@ ACPI_MODULE_NAME("processor");
 DEFINE_PER_CPU(struct acpi_processor *, processors);
 EXPORT_PER_CPU_SYMBOL(processors);
 
+static atomic_t last_used_string_cpuid;
+
 /* --------------------------------------------------------------------------
                                 Errata Handling
    -------------------------------------------------------------------------- */
@@ -229,6 +232,49 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
 }
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
 
+static acpi_status acpi_processor_evaluate_uid(struct acpi_device *device)
+{
+	acpi_status status = AE_OK;
+	struct acpi_processor *pr;
+	union acpi_object object = { 0 };
+	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
+
+	if (!device)
+		return -ENODEV;
+
+	/*
+	 * Declared with "Device" statement; match _UID.
+	 *
+	 * By definition in the ACPI spec, _UID may return either
+	 * an integer or a string.
+	 */
+	pr = acpi_driver_data(device);
+	if (!pr)
+		return -ENODEV;
+
+	status = acpi_evaluate_object(pr->handle, METHOD_NAME__UID,
+				      NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		ACPI_FREE(buffer.pointer);
+		return status;
+	}
+
+	if (object.type == ACPI_TYPE_INTEGER) {
+		pr->acpi_id = object.integer.value;
+		pr->acpi_name = NULL;
+		ACPI_FREE(buffer.pointer);		/* no longer needed */
+	} else if (object.type == ACPI_TYPE_STRING) {
+		/* no need to be clever, just pick an unused number */
+		pr->acpi_id = atomic_inc_return(&last_used_string_cpuid);
+		pr->acpi_name = object.string.pointer;	/* still needed */
+	} else {
+		ACPI_FREE(buffer.pointer);
+		return AE_BAD_DATA;
+	}
+
+	return status;
+}
+
 static int acpi_processor_get_info(struct acpi_device *device)
 {
 	union acpi_object object = { 0 };
@@ -265,12 +311,8 @@ static int acpi_processor_get_info(struct acpi_device *device)
 
 		pr->acpi_id = object.processor.proc_id;
 	} else {
-		/*
-		 * Declared with "Device" statement; match _UID.
-		 * Note that we don't handle string _UIDs yet.
-		 */
-		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
-						NULL, &value);
+		/* Declared with "Device" statement; match _UID.  */
+		status = acpi_processor_evaluate_uid(device);
 		if (ACPI_FAILURE(status)) {
 			dev_err(&device->dev,
 				"Failed to evaluate processor _UID (0x%x)\n",
@@ -278,7 +320,6 @@ static int acpi_processor_get_info(struct acpi_device *device)
 			return -ENODEV;
 		}
 		device_declaration = 1;
-		pr->acpi_id = value;
 	}
 
 	if (acpi_duplicate_processor_id(pr->acpi_id)) {
@@ -435,6 +476,7 @@ static int acpi_processor_add(struct acpi_device *device,
 	device->driver_data = NULL;
 	per_cpu(processors, pr->id) = NULL;
  err_free_pr:
+	kfree(pr->acpi_name);
 	kfree(pr);
 	return result;
 }
@@ -484,6 +526,7 @@ static void acpi_processor_remove(struct acpi_device *device)
 
  out:
 	free_cpumask_var(pr->throttling.shared_cpu_map);
+	kfree(pr->acpi_name);
 	kfree(pr);
 }
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 1194a4c78d55..115e9eee830b 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -216,6 +216,7 @@ struct acpi_processor_flags {
 struct acpi_processor {
 	acpi_handle handle;
 	u32 acpi_id;
+	char *acpi_name; /* only used if _UID is a string, otherwise null */
 	phys_cpuid_t phys_id;	/* CPU hardware ID such as APIC ID for x86 */
 	u32 id;		/* CPU logical ID allocated by OS */
 	u32 pblk;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread