[PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[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" does not need to
be in kernel messages all the time but only when debug is enabled.

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

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 85ad679..9d9cf24 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -173,7 +173,7 @@ 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"));
+		acpi_handle_debug(link->device->handle, "failed to evaluate _PRS");
 		return -ENODEV;
 	}
 
-- 
2.7.4

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Bjorn Helgaas]

On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
> be in kernel messages all the time but only when debug is enabled.

I agree and would even go further: _PRS is optional and I don't think
there's a reason to log anything at all if it's absent.  A log message
like "failed to evaluate _PRS" makes people think something is wrong
when in fact nothing is wrong.

That leads to the mindset of treating a missing _PRS as an error when
it's not.  In fact, it looks like acpi_pci_link_add() *does* treat
this as an error.  If _PRS doesn't exist, it skips the _CRS
evaluation.  That seems wrong.

> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>  drivers/acpi/pci_link.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 85ad679..9d9cf24 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -173,7 +173,7 @@ 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"));
> +		acpi_handle_debug(link->device->handle, "failed to evaluate _PRS");
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.7.4
> 

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Alex Hung]

On Fri, Feb 9, 2018 at 5:05 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>> be in kernel messages all the time but only when debug is enabled.
>
> I agree and would even go further: _PRS is optional and I don't think
> there's a reason to log anything at all if it's absent.  A log message
> like "failed to evaluate _PRS" makes people think something is wrong
> when in fact nothing is wrong.

Bjorn,

Thanks for the feedback. Rafael and I had discussion on the previous
patch that removed the error message
(https://patchwork.codeaurora.org/patch/440263/), and had a conclusion
that using a level log of "debug" is more appropriate and less noisy
for most of default setting. After all, there can be other failure
types than _PRS is absent.

>
> That leads to the mindset of treating a missing _PRS as an error when
> it's not.  In fact, it looks like acpi_pci_link_add() *does* treat
> this as an error.  If _PRS doesn't exist, it skips the _CRS
> evaluation.  That seems wrong.

Do you mean we can do something like

        status = acpi_walk_resources(link->device->handle, METHOD_NAME__PRS,
                                     acpi_pci_link_check_possible, link);
        if (status == AE_NOT_FOUND) { // acceptable scenario but let's
still output a message
                acpi_handle_debug(link->device->handle, "_PRS is absent");
        } else if (ACPI_FAILURE(status)) { // something indeed wrong with _PRS
                acpi_handle_debug(link->device->handle, "failed to
evaluate _PRS");
                return -ENODEV;
        }
or
        status = acpi_walk_resources(link->device->handle, METHOD_NAME__PRS,
                                     acpi_pci_link_check_possible, link);
        if (ACPI_FAILURE(status)) { // output messages but do not return -ENODEV
                acpi_handle_debug(link->device->handle, "something
wrong with _PRS, so let's not use it");  // or a more meaningful
message
        }

and plus other probable changes and many tests as this affects other
parts of pci_link.c

Perhaps we can do this in two patches:
1. fix the error messages first - low risk and nobody freaks out with
the new hardware
2. refine _PRS & _CRT because of higher risk and extensive testing required

Rafael and Len

Any comments and suggestions on error messages or _CRS?


>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>  drivers/acpi/pci_link.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index 85ad679..9d9cf24 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -173,7 +173,7 @@ 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"));
>> +             acpi_handle_debug(link->device->handle, "failed to evaluate _PRS");
>>               return -ENODEV;
>>       }
>>
>> --
>> 2.7.4
>>



-- 
Cheers,
Alex Hung

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Rafael J. Wysocki]

On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>> be in kernel messages all the time but only when debug is enabled.
>
> I agree and would even go further: _PRS is optional and I don't think
> there's a reason to log anything at all if it's absent.  A log message
> like "failed to evaluate _PRS" makes people think something is wrong
> when in fact nothing is wrong.

_PRS is required if the link object is pointed to by a _PRT entry.

> That leads to the mindset of treating a missing _PRS as an error when
> it's not.  In fact, it looks like acpi_pci_link_add() *does* treat
> this as an error.  If _PRS doesn't exist, it skips the _CRS
> evaluation.  That seems wrong.

I agree here.  _CRS still should be evaluated if _PRS is not there in
general, but in some cases the lack of _PRS *is* an error.

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Bjorn Helgaas]

On Sat, Feb 10, 2018 at 09:08:42AM +0100, Rafael J. Wysocki wrote:
> On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
> >> be in kernel messages all the time but only when debug is enabled.
> >
> > I agree and would even go further: _PRS is optional and I don't think
> > there's a reason to log anything at all if it's absent.  A log message
> > like "failed to evaluate _PRS" makes people think something is wrong
> > when in fact nothing is wrong.
> 
> _PRS is required if the link object is pointed to by a _PRT entry.

The closest thing I see with a quick look is ACPI 6.0, sec 6.2.13:

  These objects [PCI Interrupt Link Devices] have _PRS, _CRS, _SRS,
  and _DIS control methods to allocate the interrupt.

I don't read that as a requirement for _PRS in particular; I read it
as saying that the interrupt link devices use the normal device
configuration method, i.e., _CRS is required and _PRS and _SRS are
optional and needed for configurable devices but not for static ones.

But this is just a drive-by comment and I'm really fine with whatever
you decide to do.

> > That leads to the mindset of treating a missing _PRS as an error when
> > it's not.  In fact, it looks like acpi_pci_link_add() *does* treat
> > this as an error.  If _PRS doesn't exist, it skips the _CRS
> > evaluation.  That seems wrong.
> 
> I agree here.  _CRS still should be evaluated if _PRS is not there in
> general, but in some cases the lack of _PRS *is* an error.

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Rafael J. Wysocki]

On Sat, Feb 10, 2018 at 4:34 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sat, Feb 10, 2018 at 09:08:42AM +0100, Rafael J. Wysocki wrote:
>> On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>> >> be in kernel messages all the time but only when debug is enabled.
>> >
>> > I agree and would even go further: _PRS is optional and I don't think
>> > there's a reason to log anything at all if it's absent.  A log message
>> > like "failed to evaluate _PRS" makes people think something is wrong
>> > when in fact nothing is wrong.
>>
>> _PRS is required if the link object is pointed to by a _PRT entry.
>
> The closest thing I see with a quick look is ACPI 6.0, sec 6.2.13:
>
>   These objects [PCI Interrupt Link Devices] have _PRS, _CRS, _SRS,
>   and _DIS control methods to allocate the interrupt.
>
> I don't read that as a requirement for _PRS in particular; I read it
> as saying that the interrupt link devices use the normal device
> configuration method, i.e., _CRS is required and _PRS and _SRS are
> optional and needed for configurable devices but not for static ones.

Well, that's slightly out of context. :-)

First of all, Section 6.2.13 says this: "Typically, the interrupt
input that a given PCI interrupt is on is configurable. [...]  In this
model, each interrupt is represented in the ACPI namespace as a PCI
Interrupt Link Device."  Then, it says the above.

Now, if _PRS is not present, the IRQ represented by the link object
cannot be configured, so in fact the underlying assumption doesn't
apply to it, strictly speaking.  It follows from the last paragraph in
Section 6.2.13 that _PRT entries representing such IRQs should not
point to interrupt link device objects (there should be 0 in their
Source fields).

Hence, if a _PRT entry points to an interrupt link device object in
the namespace, _PRS should be present under this object or at least it
is reasonable to expect that it will be present in there.

> But this is just a drive-by comment and I'm really fine with whatever
> you decide to do.

OK

So I think that (a) the message should be printed using
acpi_handle_debug(), except that I would make it slightly more
neutral, something like "_PRS not present or invalid", and (b) 0
should be returned instead of -ENODEV when _PRS evaluation doesn't
succeed.

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Rafael J. Wysocki]

On Mon, Feb 12, 2018 at 9:51 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Sat, Feb 10, 2018 at 4:34 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> On Sat, Feb 10, 2018 at 09:08:42AM +0100, Rafael J. Wysocki wrote:
>>> On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> > On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>>> >> be in kernel messages all the time but only when debug is enabled.
>>> >
>>> > I agree and would even go further: _PRS is optional and I don't think
>>> > there's a reason to log anything at all if it's absent.  A log message
>>> > like "failed to evaluate _PRS" makes people think something is wrong
>>> > when in fact nothing is wrong.
>>>
>>> _PRS is required if the link object is pointed to by a _PRT entry.
>>
>> The closest thing I see with a quick look is ACPI 6.0, sec 6.2.13:
>>
>>   These objects [PCI Interrupt Link Devices] have _PRS, _CRS, _SRS,
>>   and _DIS control methods to allocate the interrupt.
>>
>> I don't read that as a requirement for _PRS in particular; I read it
>> as saying that the interrupt link devices use the normal device
>> configuration method, i.e., _CRS is required and _PRS and _SRS are
>> optional and needed for configurable devices but not for static ones.
>
> Well, that's slightly out of context. :-)
>
> First of all, Section 6.2.13 says this: "Typically, the interrupt
> input that a given PCI interrupt is on is configurable. [...]  In this
> model, each interrupt is represented in the ACPI namespace as a PCI
> Interrupt Link Device."  Then, it says the above.
>
> Now, if _PRS is not present, the IRQ represented by the link object
> cannot be configured, so in fact the underlying assumption doesn't
> apply to it, strictly speaking.  It follows from the last paragraph in
> Section 6.2.13 that _PRT entries representing such IRQs should not
> point to interrupt link device objects (there should be 0 in their
> Source fields).
>
> Hence, if a _PRT entry points to an interrupt link device object in
> the namespace, _PRS should be present under this object or at least it
> is reasonable to expect that it will be present in there.
>
>> But this is just a drive-by comment and I'm really fine with whatever
>> you decide to do.
>
> OK
>
> So I think that (a) the message should be printed using
> acpi_handle_debug(), except that I would make it slightly more
> neutral, something like "_PRS not present or invalid", and (b) 0
> should be returned instead of -ENODEV when _PRS evaluation doesn't
> succeed.

But then, of course, we may not want to evaluate _DIS on this link and
I'm not quite sure if adding it to acpi_link_list makes sense then.

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Alex Hung]

On Mon, Feb 12, 2018 at 12:51 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Sat, Feb 10, 2018 at 4:34 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> On Sat, Feb 10, 2018 at 09:08:42AM +0100, Rafael J. Wysocki wrote:
>>> On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> > On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>>> >> be in kernel messages all the time but only when debug is enabled.
>>> >
>>> > I agree and would even go further: _PRS is optional and I don't think
>>> > there's a reason to log anything at all if it's absent.  A log message
>>> > like "failed to evaluate _PRS" makes people think something is wrong
>>> > when in fact nothing is wrong.
>>>
>>> _PRS is required if the link object is pointed to by a _PRT entry.
>>
>> The closest thing I see with a quick look is ACPI 6.0, sec 6.2.13:
>>
>>   These objects [PCI Interrupt Link Devices] have _PRS, _CRS, _SRS,
>>   and _DIS control methods to allocate the interrupt.
>>
>> I don't read that as a requirement for _PRS in particular; I read it
>> as saying that the interrupt link devices use the normal device
>> configuration method, i.e., _CRS is required and _PRS and _SRS are
>> optional and needed for configurable devices but not for static ones.
>
> Well, that's slightly out of context. :-)
>
> First of all, Section 6.2.13 says this: "Typically, the interrupt
> input that a given PCI interrupt is on is configurable. [...]  In this
> model, each interrupt is represented in the ACPI namespace as a PCI
> Interrupt Link Device."  Then, it says the above.
>
> Now, if _PRS is not present, the IRQ represented by the link object
> cannot be configured, so in fact the underlying assumption doesn't
> apply to it, strictly speaking.  It follows from the last paragraph in
> Section 6.2.13 that _PRT entries representing such IRQs should not
> point to interrupt link device objects (there should be 0 in their
> Source fields).
>
> Hence, if a _PRT entry points to an interrupt link device object in
> the namespace, _PRS should be present under this object or at least it
> is reasonable to expect that it will be present in there.
>
>> But this is just a drive-by comment and I'm really fine with whatever
>> you decide to do.
>
> OK
>
> So I think that (a) the message should be printed using
> acpi_handle_debug(), except that I would make it slightly more
> neutral, something like "_PRS not present or invalid", and (b) 0
> should be returned instead of -ENODEV when _PRS evaluation doesn't
> succeed.

Looks like we will do the following.

        if (ACPI_FAILURE(status)) {
-               ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
-                return -ENODEV;
+               acpi_handle_debug(link->device->handle, "_PRS not
present or invalid");
+                return 0;

I will revise the patch and send it if this is all agreed.

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Rafael J. Wysocki]

On Tue, Feb 13, 2018 at 5:03 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On Mon, Feb 12, 2018 at 12:51 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Sat, Feb 10, 2018 at 4:34 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Sat, Feb 10, 2018 at 09:08:42AM +0100, Rafael J. Wysocki wrote:
>>>> On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>> > On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>>>> >> be in kernel messages all the time but only when debug is enabled.
>>>> >
>>>> > I agree and would even go further: _PRS is optional and I don't think
>>>> > there's a reason to log anything at all if it's absent.  A log message
>>>> > like "failed to evaluate _PRS" makes people think something is wrong
>>>> > when in fact nothing is wrong.
>>>>
>>>> _PRS is required if the link object is pointed to by a _PRT entry.
>>>
>>> The closest thing I see with a quick look is ACPI 6.0, sec 6.2.13:
>>>
>>>   These objects [PCI Interrupt Link Devices] have _PRS, _CRS, _SRS,
>>>   and _DIS control methods to allocate the interrupt.
>>>
>>> I don't read that as a requirement for _PRS in particular; I read it
>>> as saying that the interrupt link devices use the normal device
>>> configuration method, i.e., _CRS is required and _PRS and _SRS are
>>> optional and needed for configurable devices but not for static ones.
>>
>> Well, that's slightly out of context. :-)
>>
>> First of all, Section 6.2.13 says this: "Typically, the interrupt
>> input that a given PCI interrupt is on is configurable. [...]  In this
>> model, each interrupt is represented in the ACPI namespace as a PCI
>> Interrupt Link Device."  Then, it says the above.
>>
>> Now, if _PRS is not present, the IRQ represented by the link object
>> cannot be configured, so in fact the underlying assumption doesn't
>> apply to it, strictly speaking.  It follows from the last paragraph in
>> Section 6.2.13 that _PRT entries representing such IRQs should not
>> point to interrupt link device objects (there should be 0 in their
>> Source fields).
>>
>> Hence, if a _PRT entry points to an interrupt link device object in
>> the namespace, _PRS should be present under this object or at least it
>> is reasonable to expect that it will be present in there.
>>
>>> But this is just a drive-by comment and I'm really fine with whatever
>>> you decide to do.
>>
>> OK
>>
>> So I think that (a) the message should be printed using
>> acpi_handle_debug(), except that I would make it slightly more
>> neutral, something like "_PRS not present or invalid", and (b) 0
>> should be returned instead of -ENODEV when _PRS evaluation doesn't
>> succeed.
>
> Looks like we will do the following.
>
>         if (ACPI_FAILURE(status)) {
> -               ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
> -                return -ENODEV;
> +               acpi_handle_debug(link->device->handle, "_PRS not
> present or invalid");
> +                return 0;

Yes, but this affects the caller.

Please make sure that everything will work correctly in there after this change.

Re: [PATCH] ACPI/PCI: pci_link: change log level of no _PRS messages

[Alex Hung]

On Tue, Feb 13, 2018 at 12:49 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Feb 13, 2018 at 5:03 AM, Alex Hung <alex.hung@canonical.com> wrote:
>> On Mon, Feb 12, 2018 at 12:51 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Sat, Feb 10, 2018 at 4:34 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>> On Sat, Feb 10, 2018 at 09:08:42AM +0100, Rafael J. Wysocki wrote:
>>>>> On Sat, Feb 10, 2018 at 2:05 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> > On Fri, Feb 09, 2018 at 02:56:43PM -0800, 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" does not need to
>>>>> >> be in kernel messages all the time but only when debug is enabled.
>>>>> >
>>>>> > I agree and would even go further: _PRS is optional and I don't think
>>>>> > there's a reason to log anything at all if it's absent.  A log message
>>>>> > like "failed to evaluate _PRS" makes people think something is wrong
>>>>> > when in fact nothing is wrong.
>>>>>
>>>>> _PRS is required if the link object is pointed to by a _PRT entry.
>>>>
>>>> The closest thing I see with a quick look is ACPI 6.0, sec 6.2.13:
>>>>
>>>>   These objects [PCI Interrupt Link Devices] have _PRS, _CRS, _SRS,
>>>>   and _DIS control methods to allocate the interrupt.
>>>>
>>>> I don't read that as a requirement for _PRS in particular; I read it
>>>> as saying that the interrupt link devices use the normal device
>>>> configuration method, i.e., _CRS is required and _PRS and _SRS are
>>>> optional and needed for configurable devices but not for static ones.
>>>
>>> Well, that's slightly out of context. :-)
>>>
>>> First of all, Section 6.2.13 says this: "Typically, the interrupt
>>> input that a given PCI interrupt is on is configurable. [...]  In this
>>> model, each interrupt is represented in the ACPI namespace as a PCI
>>> Interrupt Link Device."  Then, it says the above.
>>>
>>> Now, if _PRS is not present, the IRQ represented by the link object
>>> cannot be configured, so in fact the underlying assumption doesn't
>>> apply to it, strictly speaking.  It follows from the last paragraph in
>>> Section 6.2.13 that _PRT entries representing such IRQs should not
>>> point to interrupt link device objects (there should be 0 in their
>>> Source fields).
>>>
>>> Hence, if a _PRT entry points to an interrupt link device object in
>>> the namespace, _PRS should be present under this object or at least it
>>> is reasonable to expect that it will be present in there.
>>>
>>>> But this is just a drive-by comment and I'm really fine with whatever
>>>> you decide to do.
>>>
>>> OK
>>>
>>> So I think that (a) the message should be printed using
>>> acpi_handle_debug(), except that I would make it slightly more
>>> neutral, something like "_PRS not present or invalid", and (b) 0
>>> should be returned instead of -ENODEV when _PRS evaluation doesn't
>>> succeed.
>>
>> Looks like we will do the following.
>>
>>         if (ACPI_FAILURE(status)) {
>> -               ACPI_EXCEPTION((AE_INFO, status, "Evaluating _PRS"));
>> -                return -ENODEV;
>> +               acpi_handle_debug(link->device->handle, "_PRS not
>> present or invalid");
>> +                return 0;
>
> Yes, but this affects the caller.
>
> Please make sure that everything will work correctly in there after this change.

I tried the above changes and everything seems to work fine. I will
send updated patch shortly.