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

PATCH Superseded Al Stone 3 participants 8 messages Aug 5, 2019 – Aug 13, 2019

Subsystems (2)

ACPI ACPI CPPC

lore

Share & tools

lore

Al Stone

author

2019-08-05 17:03 UTC #0 lore raw

PATCH 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 appears to be 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 essentially 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.

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

Cc Rafael J. Wysocki <[email protected]>

Cc Len Brown <[email protected]>

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

drivers/acpi/cppc_acpi.c +7 -4

index 15f103d7532b..e9ecfa13e997 100644
@@ -365,10 +365,13 @@ 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 (ACPI_FAILURE(status))
+			return -ENODEV;
+	} else
+		return 0;		/* _PSD is optional */
 
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {

Signature · 1 line

2.21.0

Sudeep Holla

Reviewed-by on v1 1/1

2019-08-07 11:41 UTC #1 lore raw

Quoted 10 lines · Al Stone

According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially makes _PSD mandatory when using CPPC, in violation of the specification, and only on Linux.

Quoted 9 lines

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.

Makes sense to me. FWIW,

Reviewed-by Sudeep Holla < [email protected]>

--
Regards,
Sudeep

Al Stone

author

2019-08-10 17:25 UTC #2 lore raw

Quoted 21 lines · Sudeep Holla

Quoted 10 lines · Al Stone
According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially makes _PSD mandatory when using CPPC, in violation of the specification, and only on Linux.
Quoted 9 lines
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.

Quoted 8 lines

Makes sense to me. FWIW,
Reviewed-by Sudeep Holla < [email protected]>
--
Regards,
Sudeep

Thanks for the review, Sudeep. All the ARM systems I've seen seem to have a _PSD so this hasn't been an issue there. Some newer platforms coming out are starting to use CPPC, though, and took the spec at face value :).

Signature · 8 lines

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

Al Stone

author

2019-08-13 14:00 UTC #3 lore raw

In reply to Al Stone

Quoted 39 lines · Al Stone

According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially 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.
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +0 -4
index 15f103d7532b..e9ecfa13e997 100644
@@ -365,10 +365,13 @@ 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;

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

Quoted 10 lines

drivers/acpi/cppc_acpi.c +7 -0

+	if (acpi_has_method(handle, "_PSD")) {
+		status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+						    &buffer, ACPI_TYPE_PACKAGE);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
+	} else
+		return 0;		/* _PSD is optional */
 
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {

Rafael,

Any other comments? Would it be possible to pull this into an -rc? I'd really like to avoid anyone else having to ship Linux-specific DSDTs and SSDTs.

Thanks.

Signature · 8 lines

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

Rafael J. Wysocki

2019-08-13 21:57 UTC #4 lore raw

Quoted 47 lines · CEST Al Stone

Quoted 46 lines · Al Stone
According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially 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.
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +7 -4
index 15f103d7532b..e9ecfa13e997 100644
@@ -365,10 +365,13 @@ 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 (ACPI_FAILURE(status))
+			return -ENODEV;
+	} else
+		return 0;		/* _PSD is optional */
 

Quoted 7 lines

drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 377
Quoted 3 lines
drivers/acpi/cppc_acpi.c +0 -0
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {
Rafael,
Any other comments?

Yes (they will be sent separately).

Quoted 3 lines

Would it be possible to pull this into an -rc?
I'd really like to avoid anyone else having to ship Linux-specific DSDTs and SSDTs.

You won't achieve that through this patch alone, because they will also want older kernels that don't include it to run on their platforms.

Thanks,
Rafael

Al Stone

author

2019-08-13 22:15 UTC #5 lore raw

Quoted 54 lines · Rafael J. Wysocki

Quoted 53 lines · CEST Al Stone
Quoted 39 lines · Al Stone
According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially 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.
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +0 -4
index 15f103d7532b..e9ecfa13e997 100644
@@ -365,10 +365,13 @@ 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;
drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 377
Quoted 10 lines
drivers/acpi/cppc_acpi.c +7 -0
+	if (acpi_has_method(handle, "_PSD")) {
+		status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+						    &buffer, ACPI_TYPE_PACKAGE);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
+	} else
+		return 0;		/* _PSD is optional */
 
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {
Rafael,

Quoted 3 lines

Quoted 1 line
Any other comments?
Yes (they will be sent separately).

Thanks, I appreciate it.

Quoted 6 lines

Quoted 3 lines
Would it be possible to pull this into an -rc?
I'd really like to avoid anyone else having to ship Linux-specific DSDTs and SSDTs.
You won't achieve that through this patch alone, because they will also want older kernels that don't include it to run on their platforms.

My apologies for not mentioning this before, but these are platforms that are not widely available yet. As far as I know they will not be able to use older kernels at all, even with this fix. They are very heavily reliant on the most recent changes to quite a few other things such as HMAT, PPTT, and CPPC in general. This was just one of the items their firmware developers ran into, so a 5.3 fix is plenty.

Unless, of course, I missed your point entirely....

Quoted 5 lines

Thanks,
Rafael

Signature · 8 lines

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

Rafael J. Wysocki

2019-08-13 21:59 UTC #6 lore raw

In reply to Al Stone

Quoted 31 lines · CEST Al Stone

According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially 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.
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +0 -0
index 15f103d7532b..e9ecfa13e997 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,13 @@ 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")) {

It would be better to compare the status below to AE_NOT_FOUND and return 0 if that's the case.

A couple of code lines could be saved this way at least.

Quoted 10 lines

drivers/acpi/cppc_acpi.c +6 -0

+		status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+						    &buffer, ACPI_TYPE_PACKAGE);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
+	} else
+		return 0;		/* _PSD is optional */
 
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {

Al Stone

author

2019-08-13 22:26 UTC #7 lore raw

Quoted 44 lines · Rafael J. Wysocki

Quoted 31 lines · CEST Al Stone
According to the ACPI 6.3 specification, the _PSD method is optional when using CPPC. The underlying assumption appears to be 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 essentially 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.
Signed-off-by Al Stone <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Len Brown <[email protected]>
---
 drivers/acpi/cppc_acpi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/acpi/cppc_acpi.c +0 -0
index 15f103d7532b..e9ecfa13e997 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,13 @@ 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")) {
It would be better to compare the status below to AE_NOT_FOUND and return 0 if that's the case.

Quoted 1 line

A couple of code lines could be saved this way at least.

D'oh. Good point.

Let me dig back through the ACPICA code again; I had some reason for not relying on AE_NOT_FOUND alone that I apparently did not write down in my notes. I'll send out a v2 when I figure out what it was, and if it was of any consequence.

Quoted 10 lines

drivers/acpi/cppc_acpi.c static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) · line 377
Quoted 10 lines
drivers/acpi/cppc_acpi.c +6 -0
+		status = acpi_evaluate_object_typed(handle, "_PSD", NULL,
+						    &buffer, ACPI_TYPE_PACKAGE);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
+	} else
+		return 0;		/* _PSD is optional */
 
 	psd = buffer.pointer;
 	if (!psd || psd->package.count != 1) {

Thanks.

Signature · 8 lines

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