[PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Alex Hung]

In recent Intel hardware the IRQs become non-configurable after BIOS
initializes them in PEI phase and _PRS objects are no longer included in
ASL.

This is the same as "static (non-configurable) devices do not
specify a _PRS object" in ACPI spec. As a result, error messages
saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
needed.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 drivers/acpi/pci_link.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 85ad679390e3..f98215971f30 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -172,10 +172,8 @@ static int acpi_pci_link_get_possible(struct acpi_pci_link *link)
 
 	status = acpi_walk_resources(link->device->handle, METHOD_NAME__PRS,
 				     acpi_pci_link_check_possible, link);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
+	if (ACPI_FAILURE(status))
 		return -ENODEV;
-	}
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			  "Found %d possible IRQs\n",
-- 
2.15.1

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Rafael J. Wysocki]

On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
> In recent Intel hardware the IRQs become non-configurable after BIOS
> initializes them in PEI phase and _PRS objects are no longer included in
> ASL.
> 
> This is the same as "static (non-configurable) devices do not
> specify a _PRS object" in ACPI spec. As a result, error messages
> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
> needed.

That's questionable at best.

The errors basically indicate that _PRT entries corresponding to these
IRQs are messed up (because they should contain the value of 0 instead of
a NamePath in the Source column), so we are now going to paper over bugs
in ACPI tables as someone in the firmware land cannot be bothered with
putting correct values into them. :-/

> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  drivers/acpi/pci_link.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 85ad679390e3..f98215971f30 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -172,10 +172,8 @@ static int acpi_pci_link_get_possible(struct acpi_pci_link *link)
>  
>  	status = acpi_walk_resources(link->device->handle, METHOD_NAME__PRS,
>  				     acpi_pci_link_check_possible, link);
> -	if (ACPI_FAILURE(status)) {
> -		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
> +	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> -	}
>  
>  	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			  "Found %d possible IRQs\n",
> 

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Alex Hung]

On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>> In recent Intel hardware the IRQs become non-configurable after BIOS
>> initializes them in PEI phase and _PRS objects are no longer included in
>> ASL.
>>
>> This is the same as "static (non-configurable) devices do not
>> specify a _PRS object" in ACPI spec. As a result, error messages
>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>> needed.
>
> That's questionable at best.
>
> The errors basically indicate that _PRT entries corresponding to these
> IRQs are messed up (because they should contain the value of 0 instead of
> a NamePath in the Source column), so we are now going to paper over bugs
> in ACPI tables as someone in the firmware land cannot be bothered with
> putting correct values into them. :-/

Rafael,

Thanks for quick reply and sharing the information

It seems static (non-configurable) devices on ACPI are discussed in
both _PRS and _PRT as below:

6.2.12 _PRS (Possible Resource Settings)
"... Static (non-configurable) devices do not specify a _PRS object... "

6.2.13 _PRT (PCI Routing Table)
"In the second model, the PCI interrupts are hardwired to specific
interrupt inputs on the interrupt controller and are not configurable.
In this case, the Source field in _PRT does not reference a device,
but instead contains the value zero, and the Source Index field
contains the global system interrupt to which the PCI interrupt is
hardwired."

My interpretation is the both are true from ACPI's perspective, and
both should be implemented by system firmware. On this particular
system I am debugging remotely, it does the _PRS part but not _PRT,
and I will follow up with firmware engineers.

On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
as defined in 6.2.12 in ACPI spec. I also did a code trace and it
seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
zero in Source field in _PRT.

Finally, this seems to be a confusing topic in ACPI spec (to the
firmware engineers and I at least). If we can figure this out, we may
want to clarify the wording with ASWG.

>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>  drivers/acpi/pci_link.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index 85ad679390e3..f98215971f30 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -172,10 +172,8 @@ static int acpi_pci_link_get_possible(struct acpi_pci_link *link)
>>
>>       status = acpi_walk_resources(link->device->handle, METHOD_NAME__PRS,
>>                                    acpi_pci_link_check_possible, link);
>> -     if (ACPI_FAILURE(status)) {
>> -             ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
>> +     if (ACPI_FAILURE(status))
>>               return -ENODEV;
>> -     }
>>
>>       ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>                         "Found %d possible IRQs\n",
>>
>
>



-- 
Cheers,
Alex Hung

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Alex Hung]

On Wed, Jan 31, 2018 at 8:39 PM, Alex Hung <alex.hung@canonical.com> wrote:
> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>>> In recent Intel hardware the IRQs become non-configurable after BIOS
>>> initializes them in PEI phase and _PRS objects are no longer included in
>>> ASL.
>>>
>>> This is the same as "static (non-configurable) devices do not
>>> specify a _PRS object" in ACPI spec. As a result, error messages
>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>>> needed.
>>
>> That's questionable at best.
>>
>> The errors basically indicate that _PRT entries corresponding to these
>> IRQs are messed up (because they should contain the value of 0 instead of
>> a NamePath in the Source column), so we are now going to paper over bugs
>> in ACPI tables as someone in the firmware land cannot be bothered with
>> putting correct values into them. :-/
>
> Rafael,
>
> Thanks for quick reply and sharing the information
>
> It seems static (non-configurable) devices on ACPI are discussed in
> both _PRS and _PRT as below:
>
> 6.2.12 _PRS (Possible Resource Settings)
> "... Static (non-configurable) devices do not specify a _PRS object... "
>
> 6.2.13 _PRT (PCI Routing Table)
> "In the second model, the PCI interrupts are hardwired to specific
> interrupt inputs on the interrupt controller and are not configurable.
> In this case, the Source field in _PRT does not reference a device,
> but instead contains the value zero, and the Source Index field
> contains the global system interrupt to which the PCI interrupt is
> hardwired."
>
> My interpretation is the both are true from ACPI's perspective, and
> both should be implemented by system firmware. On this particular
> system I am debugging remotely, it does the _PRS part but not _PRT,
> and I will follow up with firmware engineers.

I made a mistake saying firmware's _PRT part is not done. In the DSDT,
the _PRT is as below:


            Method (_PRT, 0, NotSerialized)  // _PRT: PCI Routing Table
            {
                If (PICM)
                {
                    Return (AR00) /* \_SB_.AR00 */
                }

                Return (PK00) /* \_SB_.PK00 */
            }

When I used acpiexec to load *.dat from acpidump, it returns _SB.PK00;
however, actual system should return _SB.AR00 which is defined as
below:

        Name (AR00, Package (0x2E)
        {
            Package (0x04)
            {
                0x0004FFFF,
                Zero,
                Zero,
                0x10
            },

            Package (0x04)
            {
                0x0005FFFF,
                Zero,
                Zero,
                0x10
            },

            Package (0x04)
            {
                0x001EFFFF,
                Zero,
                Zero,
                0x14
            },

            Package (0x04)
            {
                0x001EFFFF,
                One,
                Zero,
                0x15
            },
            ... // skips more

The third elements, Source, are zero and it meets ACPI's requirement.

>
> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
> zero in Source field in _PRT.
>
> Finally, this seems to be a confusing topic in ACPI spec (to the
> firmware engineers and I at least). If we can figure this out, we may
> want to clarify the wording with ASWG.
>
>>
>>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>>> ---
>>>  drivers/acpi/pci_link.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>>> index 85ad679390e3..f98215971f30 100644
>>> --- a/drivers/acpi/pci_link.c
>>> +++ b/drivers/acpi/pci_link.c
>>> @@ -172,10 +172,8 @@ static int acpi_pci_link_get_possible(struct acpi_pci_link *link)
>>>
>>>       status = acpi_walk_resources(link->device->handle, METHOD_NAME__PRS,
>>>                                    acpi_pci_link_check_possible, link);
>>> -     if (ACPI_FAILURE(status)) {
>>> -             ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
>>> +     if (ACPI_FAILURE(status))
>>>               return -ENODEV;
>>> -     }
>>>
>>>       ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>>                         "Found %d possible IRQs\n",
>>>
>>
>>
>
>
>
> --
> Cheers,
> Alex Hung



-- 
Cheers,
Alex Hung

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Rafael J. Wysocki]

On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>>> In recent Intel hardware the IRQs become non-configurable after BIOS
>>> initializes them in PEI phase and _PRS objects are no longer included in
>>> ASL.
>>>
>>> This is the same as "static (non-configurable) devices do not
>>> specify a _PRS object" in ACPI spec. As a result, error messages
>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>>> needed.
>>
>> That's questionable at best.
>>
>> The errors basically indicate that _PRT entries corresponding to these
>> IRQs are messed up (because they should contain the value of 0 instead of
>> a NamePath in the Source column), so we are now going to paper over bugs
>> in ACPI tables as someone in the firmware land cannot be bothered with
>> putting correct values into them. :-/
>
> Rafael,
>
> Thanks for quick reply and sharing the information
>
> It seems static (non-configurable) devices on ACPI are discussed in
> both _PRS and _PRT as below:
>
> 6.2.12 _PRS (Possible Resource Settings)
> "... Static (non-configurable) devices do not specify a _PRS object... "
>
> 6.2.13 _PRT (PCI Routing Table)
> "In the second model, the PCI interrupts are hardwired to specific
> interrupt inputs on the interrupt controller and are not configurable.
> In this case, the Source field in _PRT does not reference a device,
> but instead contains the value zero, and the Source Index field
> contains the global system interrupt to which the PCI interrupt is
> hardwired."
>
> My interpretation is the both are true from ACPI's perspective, and
> both should be implemented by system firmware. On this particular
> system I am debugging remotely, it does the _PRS part but not _PRT,
> and I will follow up with firmware engineers.

OK

> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
> zero in Source field in _PRT.

I'm not sure what you mean here.

Do you mean that the code would mishandle 0 in the Source field of _PRT?

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Alex Hung]

On Wed, Jan 31, 2018 at 11:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
>> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>>>> In recent Intel hardware the IRQs become non-configurable after BIOS
>>>> initializes them in PEI phase and _PRS objects are no longer included in
>>>> ASL.
>>>>
>>>> This is the same as "static (non-configurable) devices do not
>>>> specify a _PRS object" in ACPI spec. As a result, error messages
>>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>>>> needed.
>>>
>>> That's questionable at best.
>>>
>>> The errors basically indicate that _PRT entries corresponding to these
>>> IRQs are messed up (because they should contain the value of 0 instead of
>>> a NamePath in the Source column), so we are now going to paper over bugs
>>> in ACPI tables as someone in the firmware land cannot be bothered with
>>> putting correct values into them. :-/
>>
>> Rafael,
>>
>> Thanks for quick reply and sharing the information
>>
>> It seems static (non-configurable) devices on ACPI are discussed in
>> both _PRS and _PRT as below:
>>
>> 6.2.12 _PRS (Possible Resource Settings)
>> "... Static (non-configurable) devices do not specify a _PRS object... "
>>
>> 6.2.13 _PRT (PCI Routing Table)
>> "In the second model, the PCI interrupts are hardwired to specific
>> interrupt inputs on the interrupt controller and are not configurable.
>> In this case, the Source field in _PRT does not reference a device,
>> but instead contains the value zero, and the Source Index field
>> contains the global system interrupt to which the PCI interrupt is
>> hardwired."
>>
>> My interpretation is the both are true from ACPI's perspective, and
>> both should be implemented by system firmware. On this particular
>> system I am debugging remotely, it does the _PRS part but not _PRT,
>> and I will follow up with firmware engineers.
>
> OK
>
>> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
>> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
>> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
>> zero in Source field in _PRT.
>
> I'm not sure what you mean here.
>
> Do you mean that the code would mishandle 0 in the Source field of _PRT?

I meant the AE_NOT_FOUND messages still pop up when SOURCE = 0.

-- 
Cheers,
Alex Hung

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Rafael J. Wysocki]

On Thursday, February 1, 2018 9:07:59 AM CET Alex Hung wrote:
> On Wed, Jan 31, 2018 at 11:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
> >> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
> >>>> In recent Intel hardware the IRQs become non-configurable after BIOS
> >>>> initializes them in PEI phase and _PRS objects are no longer included in
> >>>> ASL.
> >>>>
> >>>> This is the same as "static (non-configurable) devices do not
> >>>> specify a _PRS object" in ACPI spec. As a result, error messages
> >>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
> >>>> needed.
> >>>
> >>> That's questionable at best.
> >>>
> >>> The errors basically indicate that _PRT entries corresponding to these
> >>> IRQs are messed up (because they should contain the value of 0 instead of
> >>> a NamePath in the Source column), so we are now going to paper over bugs
> >>> in ACPI tables as someone in the firmware land cannot be bothered with
> >>> putting correct values into them. :-/
> >>
> >> Rafael,
> >>
> >> Thanks for quick reply and sharing the information
> >>
> >> It seems static (non-configurable) devices on ACPI are discussed in
> >> both _PRS and _PRT as below:
> >>
> >> 6.2.12 _PRS (Possible Resource Settings)
> >> "... Static (non-configurable) devices do not specify a _PRS object... "
> >>
> >> 6.2.13 _PRT (PCI Routing Table)
> >> "In the second model, the PCI interrupts are hardwired to specific
> >> interrupt inputs on the interrupt controller and are not configurable.
> >> In this case, the Source field in _PRT does not reference a device,
> >> but instead contains the value zero, and the Source Index field
> >> contains the global system interrupt to which the PCI interrupt is
> >> hardwired."
> >>
> >> My interpretation is the both are true from ACPI's perspective, and
> >> both should be implemented by system firmware. On this particular
> >> system I am debugging remotely, it does the _PRS part but not _PRT,
> >> and I will follow up with firmware engineers.
> >
> > OK
> >
> >> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
> >> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
> >> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
> >> zero in Source field in _PRT.
> >
> > I'm not sure what you mean here.
> >
> > Do you mean that the code would mishandle 0 in the Source field of _PRT?
> 
> I meant the AE_NOT_FOUND messages still pop up when SOURCE = 0.

OK, so why does the firmware define the link objects in the namespace then?

> Do you have other comments about this patch or concerns that I can work
> with firmware engineers?

It looks to me that there are some PCI interrupt link objects in the
namespace without _PRS and which aren't pointed to by any _PRT entries.

If so, what are they useful for then?

We sure may ignore such things, but the patch makes us ignore cases that
are outright invalid too AFAICS.

Thanks,
Rafael

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Alex Hung]

On Fri, Feb 2, 2018 at 3:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, February 1, 2018 9:07:59 AM CET Alex Hung wrote:
>> On Wed, Jan 31, 2018 at 11:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
>> >> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>> >>>> In recent Intel hardware the IRQs become non-configurable after BIOS
>> >>>> initializes them in PEI phase and _PRS objects are no longer included in
>> >>>> ASL.
>> >>>>
>> >>>> This is the same as "static (non-configurable) devices do not
>> >>>> specify a _PRS object" in ACPI spec. As a result, error messages
>> >>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>> >>>> needed.
>> >>>
>> >>> That's questionable at best.
>> >>>
>> >>> The errors basically indicate that _PRT entries corresponding to these
>> >>> IRQs are messed up (because they should contain the value of 0 instead of
>> >>> a NamePath in the Source column), so we are now going to paper over bugs
>> >>> in ACPI tables as someone in the firmware land cannot be bothered with
>> >>> putting correct values into them. :-/
>> >>
>> >> Rafael,
>> >>
>> >> Thanks for quick reply and sharing the information
>> >>
>> >> It seems static (non-configurable) devices on ACPI are discussed in
>> >> both _PRS and _PRT as below:
>> >>
>> >> 6.2.12 _PRS (Possible Resource Settings)
>> >> "... Static (non-configurable) devices do not specify a _PRS object... "
>> >>
>> >> 6.2.13 _PRT (PCI Routing Table)
>> >> "In the second model, the PCI interrupts are hardwired to specific
>> >> interrupt inputs on the interrupt controller and are not configurable.
>> >> In this case, the Source field in _PRT does not reference a device,
>> >> but instead contains the value zero, and the Source Index field
>> >> contains the global system interrupt to which the PCI interrupt is
>> >> hardwired."
>> >>
>> >> My interpretation is the both are true from ACPI's perspective, and
>> >> both should be implemented by system firmware. On this particular
>> >> system I am debugging remotely, it does the _PRS part but not _PRT,
>> >> and I will follow up with firmware engineers.
>> >
>> > OK
>> >
>> >> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
>> >> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
>> >> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
>> >> zero in Source field in _PRT.
>> >
>> > I'm not sure what you mean here.
>> >
>> > Do you mean that the code would mishandle 0 in the Source field of _PRT?
>>
>> I meant the AE_NOT_FOUND messages still pop up when SOURCE = 0.
>
> OK, so why does the firmware define the link objects in the namespace then?
>
>> Do you have other comments about this patch or concerns that I can work
>> with firmware engineers?
>
> It looks to me that there are some PCI interrupt link objects in the
> namespace without _PRS and which aren't pointed to by any _PRT entries.
>
> If so, what are they useful for then?

The LNKA~LNKD used in Name(PK00) are used when PIC mode is used, ex.
_PIC(0). Disassembled ASL code is as below:

Method (_PIC, 1, NotSerialized)  // _PIC: Interrupt Model
{
    GPIC = Arg0
    PICM = Arg0
}

Method (_PRT, 0, NotSerialized)  // _PRT: PCI Routing Table
{
    If (PICM)
    {
        Return (AR00) /* \_SB_.AR00 */
    }

   Return (PK00) /* \_SB_.PK00 */
}

When the default APIC mode is used, Name(AR00) is reported, as below:

        Name (AR00, Package (0x2E)
        {
            Package (0x04)
            {
                0x0004FFFF,
                Zero,
                Zero,
                0x10
            },

            Package (0x04)
            {
                0x0005FFFF,
                Zero,
                Zero,
                0x10
            },

        // more are skipped..
        }

>
> We sure may ignore such things, but the patch makes us ignore cases that
> are outright invalid too AFAICS.

How about skipping when status == AE_NOT_FOUND, like

-       if (ACPI_FAILURE(status)) {
+       if (status != AE_NOT_FOUND && ACPI_FAILURE(status))

>
> Thanks,
> Rafael
>

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Rafael J. Wysocki]

On Tuesday, February 6, 2018 6:59:34 AM CET Alex Hung wrote:
> On Fri, Feb 2, 2018 at 3:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, February 1, 2018 9:07:59 AM CET Alex Hung wrote:
> >> On Wed, Jan 31, 2018 at 11:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> > On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
> >> >> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> >>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
> >> >>>> In recent Intel hardware the IRQs become non-configurable after BIOS
> >> >>>> initializes them in PEI phase and _PRS objects are no longer included in
> >> >>>> ASL.
> >> >>>>
> >> >>>> This is the same as "static (non-configurable) devices do not
> >> >>>> specify a _PRS object" in ACPI spec. As a result, error messages
> >> >>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
> >> >>>> needed.
> >> >>>
> >> >>> That's questionable at best.
> >> >>>
> >> >>> The errors basically indicate that _PRT entries corresponding to these
> >> >>> IRQs are messed up (because they should contain the value of 0 instead of
> >> >>> a NamePath in the Source column), so we are now going to paper over bugs
> >> >>> in ACPI tables as someone in the firmware land cannot be bothered with
> >> >>> putting correct values into them. :-/
> >> >>
> >> >> Rafael,
> >> >>
> >> >> Thanks for quick reply and sharing the information
> >> >>
> >> >> It seems static (non-configurable) devices on ACPI are discussed in
> >> >> both _PRS and _PRT as below:
> >> >>
> >> >> 6.2.12 _PRS (Possible Resource Settings)
> >> >> "... Static (non-configurable) devices do not specify a _PRS object... "
> >> >>
> >> >> 6.2.13 _PRT (PCI Routing Table)
> >> >> "In the second model, the PCI interrupts are hardwired to specific
> >> >> interrupt inputs on the interrupt controller and are not configurable.
> >> >> In this case, the Source field in _PRT does not reference a device,
> >> >> but instead contains the value zero, and the Source Index field
> >> >> contains the global system interrupt to which the PCI interrupt is
> >> >> hardwired."
> >> >>
> >> >> My interpretation is the both are true from ACPI's perspective, and
> >> >> both should be implemented by system firmware. On this particular
> >> >> system I am debugging remotely, it does the _PRS part but not _PRT,
> >> >> and I will follow up with firmware engineers.
> >> >
> >> > OK
> >> >
> >> >> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
> >> >> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
> >> >> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
> >> >> zero in Source field in _PRT.
> >> >
> >> > I'm not sure what you mean here.
> >> >
> >> > Do you mean that the code would mishandle 0 in the Source field of _PRT?
> >>
> >> I meant the AE_NOT_FOUND messages still pop up when SOURCE = 0.
> >
> > OK, so why does the firmware define the link objects in the namespace then?
> >
> >> Do you have other comments about this patch or concerns that I can work
> >> with firmware engineers?
> >
> > It looks to me that there are some PCI interrupt link objects in the
> > namespace without _PRS and which aren't pointed to by any _PRT entries.
> >
> > If so, what are they useful for then?
> 
> The LNKA~LNKD used in Name(PK00) are used when PIC mode is used, ex.
> _PIC(0).

I see.

OK, in that case I'd just change the log level of the message to "debug",
and use acpi_handle_debug() for printing it for that matter.

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Alex Hung]

On Thu, Feb 8, 2018 at 1:55 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, February 6, 2018 6:59:34 AM CET Alex Hung wrote:
>> On Fri, Feb 2, 2018 at 3:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Thursday, February 1, 2018 9:07:59 AM CET Alex Hung wrote:
>> >> On Wed, Jan 31, 2018 at 11:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >> > On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
>> >> >> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >> >>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>> >> >>>> In recent Intel hardware the IRQs become non-configurable after BIOS
>> >> >>>> initializes them in PEI phase and _PRS objects are no longer included in
>> >> >>>> ASL.
>> >> >>>>
>> >> >>>> This is the same as "static (non-configurable) devices do not
>> >> >>>> specify a _PRS object" in ACPI spec. As a result, error messages
>> >> >>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>> >> >>>> needed.
>> >> >>>
>> >> >>> That's questionable at best.
>> >> >>>
>> >> >>> The errors basically indicate that _PRT entries corresponding to these
>> >> >>> IRQs are messed up (because they should contain the value of 0 instead of
>> >> >>> a NamePath in the Source column), so we are now going to paper over bugs
>> >> >>> in ACPI tables as someone in the firmware land cannot be bothered with
>> >> >>> putting correct values into them. :-/
>> >> >>
>> >> >> Rafael,
>> >> >>
>> >> >> Thanks for quick reply and sharing the information
>> >> >>
>> >> >> It seems static (non-configurable) devices on ACPI are discussed in
>> >> >> both _PRS and _PRT as below:
>> >> >>
>> >> >> 6.2.12 _PRS (Possible Resource Settings)
>> >> >> "... Static (non-configurable) devices do not specify a _PRS object... "
>> >> >>
>> >> >> 6.2.13 _PRT (PCI Routing Table)
>> >> >> "In the second model, the PCI interrupts are hardwired to specific
>> >> >> interrupt inputs on the interrupt controller and are not configurable.
>> >> >> In this case, the Source field in _PRT does not reference a device,
>> >> >> but instead contains the value zero, and the Source Index field
>> >> >> contains the global system interrupt to which the PCI interrupt is
>> >> >> hardwired."
>> >> >>
>> >> >> My interpretation is the both are true from ACPI's perspective, and
>> >> >> both should be implemented by system firmware. On this particular
>> >> >> system I am debugging remotely, it does the _PRS part but not _PRT,
>> >> >> and I will follow up with firmware engineers.
>> >> >
>> >> > OK
>> >> >
>> >> >> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
>> >> >> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
>> >> >> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
>> >> >> zero in Source field in _PRT.
>> >> >
>> >> > I'm not sure what you mean here.
>> >> >
>> >> > Do you mean that the code would mishandle 0 in the Source field of _PRT?
>> >>
>> >> I meant the AE_NOT_FOUND messages still pop up when SOURCE = 0.
>> >
>> > OK, so why does the firmware define the link objects in the namespace then?
>> >
>> >> Do you have other comments about this patch or concerns that I can work
>> >> with firmware engineers?
>> >
>> > It looks to me that there are some PCI interrupt link objects in the
>> > namespace without _PRS and which aren't pointed to by any _PRT entries.
>> >
>> > If so, what are they useful for then?
>>
>> The LNKA~LNKD used in Name(PK00) are used when PIC mode is used, ex.
>> _PIC(0).
>
> I see.
>
> OK, in that case I'd just change the log level of the message to "debug",
> and use acpi_handle_debug() for printing it for that matter.
>

Hi Rafael,

Do you mean something like the below change?

        if (ACPI_FAILURE(status)) {
-               ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
+               acpi_handle_debug(link->device->handle, "failed to
evaluate _PRS");
                return -ENODEV;
        }

If so, I will submit a V2 accordingly.

Re: [PATCH] ACPI/PCI: pci_link: remove error messages when no _PRS methods

[Rafael J. Wysocki]

On Fri, Feb 9, 2018 at 10:02 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On Thu, Feb 8, 2018 at 1:55 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Tuesday, February 6, 2018 6:59:34 AM CET Alex Hung wrote:
>>> On Fri, Feb 2, 2018 at 3:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> > On Thursday, February 1, 2018 9:07:59 AM CET Alex Hung wrote:
>>> >> On Wed, Jan 31, 2018 at 11:30 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> >> > On Thu, Feb 1, 2018 at 5:39 AM, Alex Hung <alex.hung@canonical.com> wrote:
>>> >> >> On Wed, Jan 31, 2018 at 1:20 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> >> >>> On Wednesday, January 31, 2018 6:52:19 AM CET Alex Hung wrote:
>>> >> >>>> In recent Intel hardware the IRQs become non-configurable after BIOS
>>> >> >>>> initializes them in PEI phase and _PRS objects are no longer included in
>>> >> >>>> ASL.
>>> >> >>>>
>>> >> >>>> This is the same as "static (non-configurable) devices do not
>>> >> >>>> specify a _PRS object" in ACPI spec. As a result, error messages
>>> >> >>>> saying "ACPI Exception: AE_NOT_FOUND, Evaluating _PRS" are not
>>> >> >>>> needed.
>>> >> >>>
>>> >> >>> That's questionable at best.
>>> >> >>>
>>> >> >>> The errors basically indicate that _PRT entries corresponding to these
>>> >> >>> IRQs are messed up (because they should contain the value of 0 instead of
>>> >> >>> a NamePath in the Source column), so we are now going to paper over bugs
>>> >> >>> in ACPI tables as someone in the firmware land cannot be bothered with
>>> >> >>> putting correct values into them. :-/
>>> >> >>
>>> >> >> Rafael,
>>> >> >>
>>> >> >> Thanks for quick reply and sharing the information
>>> >> >>
>>> >> >> It seems static (non-configurable) devices on ACPI are discussed in
>>> >> >> both _PRS and _PRT as below:
>>> >> >>
>>> >> >> 6.2.12 _PRS (Possible Resource Settings)
>>> >> >> "... Static (non-configurable) devices do not specify a _PRS object... "
>>> >> >>
>>> >> >> 6.2.13 _PRT (PCI Routing Table)
>>> >> >> "In the second model, the PCI interrupts are hardwired to specific
>>> >> >> interrupt inputs on the interrupt controller and are not configurable.
>>> >> >> In this case, the Source field in _PRT does not reference a device,
>>> >> >> but instead contains the value zero, and the Source Index field
>>> >> >> contains the global system interrupt to which the PCI interrupt is
>>> >> >> hardwired."
>>> >> >>
>>> >> >> My interpretation is the both are true from ACPI's perspective, and
>>> >> >> both should be implemented by system firmware. On this particular
>>> >> >> system I am debugging remotely, it does the _PRS part but not _PRT,
>>> >> >> and I will follow up with firmware engineers.
>>> >> >
>>> >> > OK
>>> >> >
>>> >> >> On the other hand, it may not be unreasonable to remove AE_NOT_FOUND
>>> >> >> as defined in 6.2.12 in ACPI spec. I also did a code trace and it
>>> >> >> seems that the AE_NOT_FOUND in _PRS cannot be removed by a value of
>>> >> >> zero in Source field in _PRT.
>>> >> >
>>> >> > I'm not sure what you mean here.
>>> >> >
>>> >> > Do you mean that the code would mishandle 0 in the Source field of _PRT?
>>> >>
>>> >> I meant the AE_NOT_FOUND messages still pop up when SOURCE = 0.
>>> >
>>> > OK, so why does the firmware define the link objects in the namespace then?
>>> >
>>> >> Do you have other comments about this patch or concerns that I can work
>>> >> with firmware engineers?
>>> >
>>> > It looks to me that there are some PCI interrupt link objects in the
>>> > namespace without _PRS and which aren't pointed to by any _PRT entries.
>>> >
>>> > If so, what are they useful for then?
>>>
>>> The LNKA~LNKD used in Name(PK00) are used when PIC mode is used, ex.
>>> _PIC(0).
>>
>> I see.
>>
>> OK, in that case I'd just change the log level of the message to "debug",
>> and use acpi_handle_debug() for printing it for that matter.
>>
>
> Hi Rafael,
>
> Do you mean something like the below change?
>
>         if (ACPI_FAILURE(status)) {
> -               ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
> +               acpi_handle_debug(link->device->handle, "failed to
> evaluate _PRS");
>                 return -ENODEV;
>         }
>
> If so, I will submit a V2 accordingly.

Yes, please.