[PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Yicong Yang]

Previously we'll print wrong ASPM status if _OSC method return
failed. For example, if ASPM is enabled by setting pcie_aspm=force,
we get message below:

    acpi PNP0A08:02: _OSC failed (AE_NOT_FOUND); disabling ASPM

Fix it and print correct ASPM status when _OSC failed.

Fixes: 1ad61b612b95 ("PCI/ACPI: Correct error message for ASPM disabling")
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/acpi/pci_root.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ac8ad6c..5140b26 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 
 		dev_info(&device->dev, "_OSC failed (%s)%s\n",
 			 acpi_format_exception(status),
-			 pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
+			 pcie_aspm_support_enabled() ? "" : "; disabling ASPM");
 		return;
 	}
 
-- 
2.8.1

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Rafael J. Wysocki]

On Thu, May 28, 2020 at 10:39 AM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Previously we'll print wrong ASPM status if _OSC method return
> failed. For example, if ASPM is enabled by setting pcie_aspm=force,
> we get message below:
>
>     acpi PNP0A08:02: _OSC failed (AE_NOT_FOUND); disabling ASPM
>
> Fix it and print correct ASPM status when _OSC failed.
>
> Fixes: 1ad61b612b95 ("PCI/ACPI: Correct error message for ASPM disabling")
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/acpi/pci_root.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ac8ad6c..5140b26 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>
>                 dev_info(&device->dev, "_OSC failed (%s)%s\n",
>                          acpi_format_exception(status),
> -                        pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
> +                        pcie_aspm_support_enabled() ? "" : "; disabling ASPM");
>                 return;
>         }
>
> --

Applied as 5.8 material under the "ACPI: PCI: Fix the ASPM part of the
_OSC failure message" subject and with a different changelog.

Thanks!

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Sinan Kaya]

On 5/28/2020 7:42 AM, Rafael J. Wysocki wrote:
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index ac8ad6c..5140b26 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>
>>                 dev_info(&device->dev, "_OSC failed (%s)%s\n",
>>                          acpi_format_exception(status),
>> -                        pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
>> +                        pcie_aspm_support_enabled() ? "" : "; disabling ASPM");
>>                 return;
>>         }
>>
>> --
> Applied as 5.8 material under the "ACPI: PCI: Fix the ASPM part of the
> _OSC failure message" subject and with a different changelog.


I'm confused. The original change would print ASPM is getting disabled
only when ASPM is supported. Now, we are printing disabling ASPM when
ASPM is not supported.

Now, we reverted the change and went back to incorrect behavior again.

Am I missing something?

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Rafael J. Wysocki]

On Monday, June 1, 2020 5:14:45 PM CEST Sinan Kaya wrote:
> On 5/28/2020 7:42 AM, Rafael J. Wysocki wrote:
> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> >> index ac8ad6c..5140b26 100644
> >> --- a/drivers/acpi/pci_root.c
> >> +++ b/drivers/acpi/pci_root.c
> >> @@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> >>
> >>                 dev_info(&device->dev, "_OSC failed (%s)%s\n",
> >>                          acpi_format_exception(status),
> >> -                        pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
> >> +                        pcie_aspm_support_enabled() ? "" : "; disabling ASPM");
> >>                 return;
> >>         }
> >>
> >> --
> > Applied as 5.8 material under the "ACPI: PCI: Fix the ASPM part of the
> > _OSC failure message" subject and with a different changelog.
> 
> 
> I'm confused. The original change would print ASPM is getting disabled
> only when ASPM is supported. Now, we are printing disabling ASPM when
> ASPM is not supported.
> 
> Now, we reverted the change and went back to incorrect behavior again.
> 
> Am I missing something?

Well, it turns out that I was confused, as well as the author of the patch.

Dropped now, thanks for the heads-up!

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Yicong Yang]

On 2020/6/1 23:22, Rafael J. Wysocki wrote:
> On Monday, June 1, 2020 5:14:45 PM CEST Sinan Kaya wrote:
>> On 5/28/2020 7:42 AM, Rafael J. Wysocki wrote:
>>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>>> index ac8ad6c..5140b26 100644
>>>> --- a/drivers/acpi/pci_root.c
>>>> +++ b/drivers/acpi/pci_root.c
>>>> @@ -456,7 +456,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>>>
>>>>                 dev_info(&device->dev, "_OSC failed (%s)%s\n",
>>>>                          acpi_format_exception(status),
>>>> -                        pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
>>>> +                        pcie_aspm_support_enabled() ? "" : "; disabling ASPM");
>>>>                 return;
>>>>         }
>>>>
>>>> --
>>> Applied as 5.8 material under the "ACPI: PCI: Fix the ASPM part of the
>>> _OSC failure message" subject and with a different changelog.
>>
>> I'm confused. The original change would print ASPM is getting disabled
>> only when ASPM is supported. Now, we are printing disabling ASPM when
>> ASPM is not supported.
>>
>> Now, we reverted the change and went back to incorrect behavior again.
>>
>> Am I missing something?
> Well, it turns out that I was confused, as well as the author of the patch.
>
> Dropped now, thanks for the heads-up!

well, Sinan's words make sense to me. But I'm still confused that, the message
says we're "disabling ASPM" but ASPM maybe enabled if we designate
pcie_aspm=force as I mentioned in the commit message. Will it be possible if
we replace "disabling" to "disabled" or we can do something else to make
the message reflect the real status of ASPM?

Thanks,
Yicong


>
>
>
> .
>

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Sinan Kaya]

Bjorn,

On 6/1/2020 9:57 PM, Yicong Yang wrote:
> well, Sinan's words make sense to me. But I'm still confused that, the message
> says we're "disabling ASPM" but ASPM maybe enabled if we designate
> pcie_aspm=force as I mentioned in the commit message. Will it be possible if
> we replace "disabling" to "disabled" or we can do something else to make
> the message reflect the real status of ASPM?

What do you think?

Sinan

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Bjorn Helgaas]

On Tue, Jun 02, 2020 at 01:50:37PM -0400, Sinan Kaya wrote:
> Bjorn,
> 
> On 6/1/2020 9:57 PM, Yicong Yang wrote:
> > well, Sinan's words make sense to me. But I'm still confused that, the message
> > says we're "disabling ASPM" but ASPM maybe enabled if we designate
> > pcie_aspm=force as I mentioned in the commit message. Will it be possible if
> > we replace "disabling" to "disabled" or we can do something else to make
> > the message reflect the real status of ASPM?
> 
> What do you think?

ASPM is a mess in general, and the whole "no_aspm" dance for delaying
setting of aspm_disabled is ... well, it's confusing at best.

These "_OSC failed" messages are confusing to users as well.  They
lead to bug reports against Linux (when it's usually a BIOS problem)
and users booting with "pcie_aspm=force" (which is a poor user
experience and potentially dangerous since the platform hasn't granted
us control of the PCIe Capability).

And it's not even specific to ASPM; when _OSC fails, we don't take
over *any* PCIe features.  At least, we're not *supposed* to -- I
don't think we're very careful about random things in the PCIe
capability.

What if we just removed the ASPM text from the message completely,
e.g., something like this:

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 800a3d26d24b..49fdb07061b1 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -453,9 +453,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		if ((status == AE_NOT_FOUND) && !is_pcie)
 			return;
 
-		dev_info(&device->dev, "_OSC failed (%s)%s\n",
-			 acpi_format_exception(status),
-			 pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
+		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
+			 acpi_format_exception(status));
 		return;
 	}
 
@@ -516,7 +515,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 	} else {
 		decode_osc_control(root, "OS requested", requested);
 		decode_osc_control(root, "platform willing to grant", control);
-		dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
+		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
 			acpi_format_exception(status));
 
 		/*

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Sean V Kelley]

On 2 Jun 2020, at 15:36, Bjorn Helgaas wrote:

> On Tue, Jun 02, 2020 at 01:50:37PM -0400, Sinan Kaya wrote:
>> Bjorn,
>>
>> On 6/1/2020 9:57 PM, Yicong Yang wrote:
>>> well, Sinan's words make sense to me. But I'm still confused that, 
>>> the message
>>> says we're "disabling ASPM" but ASPM maybe enabled if we designate
>>> pcie_aspm=force as I mentioned in the commit message. Will it be 
>>> possible if
>>> we replace "disabling" to "disabled" or we can do something else to 
>>> make
>>> the message reflect the real status of ASPM?
>>
>> What do you think?
>
> ASPM is a mess in general, and the whole "no_aspm" dance for delaying
> setting of aspm_disabled is ... well, it's confusing at best.
>
> These "_OSC failed" messages are confusing to users as well.  They
> lead to bug reports against Linux (when it's usually a BIOS problem)
> and users booting with "pcie_aspm=force" (which is a poor user
> experience and potentially dangerous since the platform hasn't granted
> us control of the PCIe Capability).
>
> And it's not even specific to ASPM; when _OSC fails, we don't take
> over *any* PCIe features.  At least, we're not *supposed* to -- I
> don't think we're very careful about random things in the PCIe
> capability.

I agree.  It also will become even more potentially confusing where _OSC 
fails for CXL.

>
> What if we just removed the ASPM text from the message completely,
> e.g., something like this:
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 800a3d26d24b..49fdb07061b1 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -453,9 +453,8 @@ static void negotiate_os_control(struct 
> acpi_pci_root *root, int *no_aspm,
>  		if ((status == AE_NOT_FOUND) && !is_pcie)
>  			return;
>
> -		dev_info(&device->dev, "_OSC failed (%s)%s\n",
> -			 acpi_format_exception(status),
> -			 pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
> +		dev_info(&device->dev, "_OSC: platform retains control of PCIe 
> features (%s)\n",
> +			 acpi_format_exception(status));
>  		return;
>  	}
>
> @@ -516,7 +515,7 @@ static void negotiate_os_control(struct 
> acpi_pci_root *root, int *no_aspm,
>  	} else {
>  		decode_osc_control(root, "OS requested", requested);
>  		decode_osc_control(root, "platform willing to grant", control);
> -		dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
> +		dev_info(&device->dev, "_OSC: platform retains control of PCIe 
> features (%s)\n",
>  			acpi_format_exception(status));
>

That looks good to me and I can add similar wording for CXL features in 
my patches.

Thanks,

Sean

>  		/*

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Sinan Kaya]

On 6/2/2020 7:21 PM, Sean V Kelley wrote:


Thanks,

>> -        dev_info(&device->dev, "_OSC failed (%s)%s\n",
>> -             acpi_format_exception(status),
>> -             pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
>> +        dev_info(&device->dev, "_OSC: platform retains control of
>> PCIe features (%s)\n",
>> +             acpi_format_exception(status));
>>          return;
>>      }
>>
>> @@ -516,7 +515,7 @@ static void negotiate_os_control(struct
>> acpi_pci_root *root, int *no_aspm,
>>      } else {
>>          decode_osc_control(root, "OS requested", requested);
>>          decode_osc_control(root, "platform willing to grant", control);
>> -        dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
>> +        dev_info(&device->dev, "_OSC: platform retains control of
>> PCIe features (%s)\n",
>>              acpi_format_exception(status));
>>
> 

feel free to include my reviewed by.

Reviewed-by: Sinan Kaya <okaya@kernel.org>

Re: [PATCH v2] ACPI: PCI: Remove ASPM text from _OSC failure message

[Rafael J. Wysocki]

On Wed, Jun 3, 2020 at 2:15 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>
> Previously the _OSC failed message is rather confusing, as if we
> forcibly enable ASPM by set pcie_aspm=force, we'll get the message
> below, which doesn't the reflect the real status.
>
>   acpi PNP0A08:02: _OSC failed (AE_NOT_FOUND); disabling ASPM
>
> Reword the _OSC failure message and remove the ASPM text to make
> it clear. As if _OSC failed we're not supposed to take over any
> PCIe features including ASPM. After the Patch it'll look like:
>
>   acpi PNP0A08:02: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
>
> No functional change intended.
>
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> Reviewed-by: Sinan Kaya <okaya@kernel.org>

This is a Bjorn's patch to which you have added a changelog and posted
as yours.  It is not OK to do things like that.

> ---
>  drivers/acpi/pci_root.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ac8ad6c..8dd7f14 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -454,9 +454,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>   if ((status == AE_NOT_FOUND) && !is_pcie)
>   return;
>
> - dev_info(&device->dev, "_OSC failed (%s)%s\n",
> - acpi_format_exception(status),
> - pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
> + dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> + acpi_format_exception(status));
>   return;
>   }
>
> @@ -517,7 +516,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>   } else {
>   decode_osc_control(root, "OS requested", requested);
>   decode_osc_control(root, "platform willing to grant", control);
> - dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
> + dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
>   acpi_format_exception(status));
>
>   /*
> --
> 2.8.1
>
> .
>

Re: [PATCH v2] ACPI: PCI: Remove ASPM text from _OSC failure message

[Yicong Yang]

On 2020/6/3 21:02, Rafael J. Wysocki wrote:
> On Wed, Jun 3, 2020 at 2:15 PM Yicong Yang <yangyicong@hisilicon.com> wrote:
>> Previously the _OSC failed message is rather confusing, as if we
>> forcibly enable ASPM by set pcie_aspm=force, we'll get the message
>> below, which doesn't the reflect the real status.
>>
>>   acpi PNP0A08:02: _OSC failed (AE_NOT_FOUND); disabling ASPM
>>
>> Reword the _OSC failure message and remove the ASPM text to make
>> it clear. As if _OSC failed we're not supposed to take over any
>> PCIe features including ASPM. After the Patch it'll look like:
>>
>>   acpi PNP0A08:02: _OSC: platform retains control of PCIe features (AE_NOT_FOUND)
>>
>> No functional change intended.
>>
>> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> Reviewed-by: Sinan Kaya <okaya@kernel.org>
> This is a Bjorn's patch to which you have added a changelog and posted
> as yours.  It is not OK to do things like that.

Please ignore this Patch !

Sorry for my mistake and thanks for pointing it out.

>
>> ---
>>  drivers/acpi/pci_root.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index ac8ad6c..8dd7f14 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -454,9 +454,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>   if ((status == AE_NOT_FOUND) && !is_pcie)
>>   return;
>>
>> - dev_info(&device->dev, "_OSC failed (%s)%s\n",
>> - acpi_format_exception(status),
>> - pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
>> + dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
>> + acpi_format_exception(status));
>>   return;
>>   }
>>
>> @@ -517,7 +516,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>>   } else {
>>   decode_osc_control(root, "OS requested", requested);
>>   decode_osc_control(root, "platform willing to grant", control);
>> - dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
>> + dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
>>   acpi_format_exception(status));
>>
>>   /*
>> --
>> 2.8.1
>>
>> .
>>
> .
>

Re: [PATCH] PCI/ASPM: Print correct ASPM status when _OSC failed

[Rafael J. Wysocki]

On Wed, Jun 3, 2020 at 12:36 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Jun 02, 2020 at 01:50:37PM -0400, Sinan Kaya wrote:
> > Bjorn,
> >
> > On 6/1/2020 9:57 PM, Yicong Yang wrote:
> > > well, Sinan's words make sense to me. But I'm still confused that, the message
> > > says we're "disabling ASPM" but ASPM maybe enabled if we designate
> > > pcie_aspm=force as I mentioned in the commit message. Will it be possible if
> > > we replace "disabling" to "disabled" or we can do something else to make
> > > the message reflect the real status of ASPM?
> >
> > What do you think?
>
> ASPM is a mess in general, and the whole "no_aspm" dance for delaying
> setting of aspm_disabled is ... well, it's confusing at best.
>
> These "_OSC failed" messages are confusing to users as well.  They
> lead to bug reports against Linux (when it's usually a BIOS problem)
> and users booting with "pcie_aspm=force" (which is a poor user
> experience and potentially dangerous since the platform hasn't granted
> us control of the PCIe Capability).
>
> And it's not even specific to ASPM; when _OSC fails, we don't take
> over *any* PCIe features.  At least, we're not *supposed* to -- I
> don't think we're very careful about random things in the PCIe
> capability.
>
> What if we just removed the ASPM text from the message completely,
> e.g., something like this:
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 800a3d26d24b..49fdb07061b1 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -453,9 +453,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>                 if ((status == AE_NOT_FOUND) && !is_pcie)
>                         return;
>
> -               dev_info(&device->dev, "_OSC failed (%s)%s\n",
> -                        acpi_format_exception(status),
> -                        pcie_aspm_support_enabled() ? "; disabling ASPM" : "");
> +               dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> +                        acpi_format_exception(status));
>                 return;
>         }
>
> @@ -516,7 +515,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>         } else {
>                 decode_osc_control(root, "OS requested", requested);
>                 decode_osc_control(root, "platform willing to grant", control);
> -               dev_info(&device->dev, "_OSC failed (%s); disabling ASPM\n",
> +               dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
>                         acpi_format_exception(status));
>
>                 /*

Looks good to me.

Cheers!