| Message ID | 20190826223047.13146-1-ahs3@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] ACPI / CPPC: do not require the _PSD method when using CPPC | expand |
On Tue, Aug 27, 2019 at 12:30 AM Al Stone <ahs3@redhat.com> wrote: > > 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 <ahs3@redhat.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > --- > drivers/acpi/cppc_acpi.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 15f103d7532b..7a946f1944ab 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -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. > + 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? > + } > > psd = buffer.pointer; > if (!psd || psd->package.count != 1) { > -- > 2.21.0 >
On 8/26/19 5:02 PM, Rafael J. Wysocki wrote: > On Tue, Aug 27, 2019 at 12:30 AM Al Stone <ahs3@redhat.com> wrote: >> >> 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 <ahs3@redhat.com> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Len Brown <lenb@kernel.org> >> --- >> drivers/acpi/cppc_acpi.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 15f103d7532b..7a946f1944ab 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -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. Probably true. I'll look back through acpi_evaluate_object_typed(). >> + 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 :(... >> + } >> >> psd = buffer.pointer; >> if (!psd || psd->package.count != 1) { >> -- >> 2.21.0 >>
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 15f103d7532b..7a946f1944ab 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -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) {
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 <ahs3@redhat.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> --- drivers/acpi/cppc_acpi.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)