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!
