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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 3, 2020 at 12:36 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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!



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux