| Message ID | 1590655125-23949-1-git-send-email-yangyicong@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/ASPM: Print correct ASPM status when _OSC failed | expand |
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!
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?
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!
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 > > > > . >
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
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)); /*
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 > /*
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>
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!
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 > > . >
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 >> >> . >> > . >
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; }
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(-)