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
/*
