ACPI / CPPC: do not require the _PSD method when using CPPC

PATCH Superseded Al Stone 2 participants 3 messages Aug 26, 2019 – Aug 27, 2019

Subsystems (2)

ACPI ACPI CPPC

lore

Share & tools

lore

Al Stone

author

2019-08-26 22:30 UTC #0 lore raw

PATCH v2 1/1 ACPI / CPPC: do not require the _PSD method when using CPPC

According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption is that each CPU can change frequency independently from all other CPUs; _PSD is provided to tell the OS that some processors can NOT do that.

However, the acpi_get_psd() function returns ENODEV if there is no _PSD method present, or an ACPI error status if an error occurs when evaluating _PSD, if present. This makes _PSD mandatory when using CPPC, in violation of the specification, and only on Linux.

This has forced some firmware writers to provide a dummy _PSD, even though it is irrelevant, but only because Linux requires it; other OSPMs follow the spec. We really do not want to have OS specific ACPI tables, though.

So, correct acpi_get_psd() so that it does not return an error if there is no _PSD method present, but does return a failure when the method can not be executed properly. This allows _PSD to be optional as it should be.

v2:
-- verified simple check for AE_NOT_FOUND was sufficient -- simplified return status check per Rafael's suggestion

Signed-off-by Al Stone <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Len Brown <[email protected]>

---
 drivers/acpi/cppc_acpi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/acpi/cppc_acpi.c +6 -4

index 15f103d7532b..7a946f1944ab 100644
@@ -365,10 +365,12 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
 	union acpi_object  *psd = NULL;
 	struct acpi_psd_package *pdomain;
 
-	status = acpi_evaluate_object_typed(handle, "_PSD", NULL, &buffer,
-			ACPI_TYPE_PACKAGE);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
+	if (acpi_has_method(handle, "_PSD")) {
+		status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+						    &buffer, ACPI_TYPE_PACKAGE);
+		if (status == AE_NOT_FOUND)	/* _PSD is optional */
+			return 0;
+	}
 
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {

Signature · 1 line

2.21.0

Rafael J. Wysocki

maintainer

2019-08-26 23:02 UTC #1 lore raw

Quoted 36 lines · Al Stone

According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption is that each CPU can change frequency independently from all other CPUs; _PSD is provided to tell the OS that some processors can NOT do that.
However, the acpi_get_psd() function returns ENODEV if there is no _PSD method present, or an ACPI error status if an error occurs when evaluating _PSD, if present. This makes _PSD mandatory when using CPPC, in violation of the specification, and only on Linux.
This has forced some firmware writers to provide a dummy _PSD, even though it is irrelevant, but only because Linux requires it; other OSPMs follow the spec. We really do not want to have OS specific ACPI tables, though.
So, correct acpi_get_psd() so that it does not return an error if there is no _PSD method present, but does return a failure when the method can not be executed properly. This allows _PSD to be optional as it should be.
v2:
-- verified simple check for AE_NOT_FOUND was sufficient -- simplified return status check per Rafael's suggestion
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +0 -0
index 15f103d7532b..7a946f1944ab 100644

drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 368

Quoted 8 lines

drivers/acpi/cppc_acpi.c +1 -4

@@ -365,10 +365,12 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
        union acpi_object  *psd = NULL;
        struct acpi_psd_package *pdomain;
 
-       status = acpi_evaluate_object_typed(handle, "_PSD", NULL, &buffer,
-                       ACPI_TYPE_PACKAGE);
-       if (ACPI_FAILURE(status))
-               return -ENODEV;
+       if (acpi_has_method(handle, "_PSD")) {

This doesn't look necessary any more.

drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 372

Quoted 4 lines

drivers/acpi/cppc_acpi.c +4 -0

+               status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+                                                   &buffer, ACPI_TYPE_PACKAGE);
+               if (status == AE_NOT_FOUND)     /* _PSD is optional */
+                       return 0;

And what about the other possible errors?

Quoted 7 lines

drivers/acpi/cppc_acpi.c +1 -1

+       }
 
        psd = buffer.pointer;
        if (!psd || psd->package.count != 1) {
--

2.21.0

Al Stone

author

2019-08-27 02:31 UTC #2 lore raw

Quoted 40 lines · Rafael J. Wysocki

Quoted 39 lines · Al Stone
According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption is that each CPU can change frequency independently from all other CPUs; _PSD is provided to tell the OS that some processors can NOT do that.
However, the acpi_get_psd() function returns ENODEV if there is no _PSD method present, or an ACPI error status if an error occurs when evaluating _PSD, if present. This makes _PSD mandatory when using CPPC, in violation of the specification, and only on Linux.
This has forced some firmware writers to provide a dummy _PSD, even though it is irrelevant, but only because Linux requires it; other OSPMs follow the spec. We really do not want to have OS specific ACPI tables, though.
So, correct acpi_get_psd() so that it does not return an error if there is no _PSD method present, but does return a failure when the method can not be executed properly. This allows _PSD to be optional as it should be.
v2:
-- verified simple check for AE_NOT_FOUND was sufficient -- simplified return status check per Rafael's suggestion
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +0 -0
index 15f103d7532b..7a946f1944ab 100644
@@ -365,10 +365,12 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle)
        union acpi_object  *psd = NULL;
        struct acpi_psd_package *pdomain;
 

Quoted 7 lines

drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 368
Quoted 5 lines
drivers/acpi/cppc_acpi.c +1 -4
-       status = acpi_evaluate_object_typed(handle, "_PSD", NULL, &buffer,
-                       ACPI_TYPE_PACKAGE);
-       if (ACPI_FAILURE(status))
-               return -ENODEV;
+       if (acpi_has_method(handle, "_PSD")) {
This doesn't look necessary any more.

Probably true. I'll look back through acpi_evaluate_object_typed().

Quoted 6 lines

drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 372
Quoted 4 lines
drivers/acpi/cppc_acpi.c +4 -0
+               status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+                                                   &buffer, ACPI_TYPE_PACKAGE);
+               if (status == AE_NOT_FOUND)     /* _PSD is optional */
+                       return 0;
And what about the other possible errors?

Argh. My apologies. I was not paying attention. I'll correct this and send proper code tomorrow. Really sorry for the noise :(...

Quoted 7 lines

drivers/acpi/cppc_acpi.c +1 -1

+       }
 
        psd = buffer.pointer;
        if (!psd || psd->package.count != 1) {
--

2.21.0

Signature · 8 lines

ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------