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

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

[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 <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/cppc_acpi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 15f103d7532b..e9ecfa13e997 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -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) {
-- 
2.21.0

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Sudeep Holla]

On Mon, Aug 05, 2019 at 11:03:38AM -0600, Al Stone wrote:
> 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.
>

Makes sense to me. FWIW,

Reviewed-by: Sudeep Holla < sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Al Stone]

On 8/7/19 5:41 AM, Sudeep Holla wrote:
> On Mon, Aug 05, 2019 at 11:03:38AM -0600, Al Stone wrote:
>> 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.
>>
> 
> Makes sense to me. FWIW,
> 
> Reviewed-by: Sudeep Holla < sudeep.holla@arm.com>
> 
> --
> 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 :).

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Al Stone]

On 8/5/19 11:03 AM, Al Stone wrote:
> 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 <ahs3@redhat.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> ---
>  drivers/acpi/cppc_acpi.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 15f103d7532b..e9ecfa13e997 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -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) {
> 

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.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Rafael J. Wysocki]

On Tuesday, August 13, 2019 4:00:56 PM CEST Al Stone wrote:
> On 8/5/19 11:03 AM, Al Stone wrote:
> > 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 <ahs3@redhat.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Len Brown <lenb@kernel.org>
> > ---
> >  drivers/acpi/cppc_acpi.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > index 15f103d7532b..e9ecfa13e997 100644
> > --- a/drivers/acpi/cppc_acpi.c
> > +++ b/drivers/acpi/cppc_acpi.c
> > @@ -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) {
> > 
> 
> Rafael,
> 
> Any other comments?

Yes (they will be sent separately).

> 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

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Al Stone]

On 8/13/19 3:57 PM, Rafael J. Wysocki wrote:
> On Tuesday, August 13, 2019 4:00:56 PM CEST Al Stone wrote:
>> On 8/5/19 11:03 AM, Al Stone wrote:
>>> 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 <ahs3@redhat.com>
>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>>> Cc: Len Brown <lenb@kernel.org>
>>> ---
>>>  drivers/acpi/cppc_acpi.c | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index 15f103d7532b..e9ecfa13e997 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -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) {
>>>
>>
>> Rafael,
>>
>> Any other comments?
> 
> Yes (they will be sent separately).

Thanks, I appreciate it.

>> 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....

> Thanks,
> Rafael
> 
> 
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Rafael J. Wysocki]

On Monday, August 5, 2019 7:03:38 PM CEST Al Stone wrote:
> 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 <ahs3@redhat.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> ---
>  drivers/acpi/cppc_acpi.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 15f103d7532b..e9ecfa13e997 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -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.

> +		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) {
> 

Re: [PATCH] ACPI / CPPC: do not require the _PSD method when using CPPC

[Al Stone]

On 8/13/19 3:59 PM, Rafael J. Wysocki wrote:
> On Monday, August 5, 2019 7:03:38 PM CEST Al Stone wrote:
>> 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 <ahs3@redhat.com>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Len Brown <lenb@kernel.org>
>> ---
>>  drivers/acpi/cppc_acpi.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 15f103d7532b..e9ecfa13e997 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -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.

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.

>> +		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.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------