On Thu, Jun 03, 2021 at 03:50:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 02:48:14PM +0200, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel@xxxxxxx>
> > ...
> > -static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 req)
> > +static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32
> > + *mask, u32 req, u32 support)
> > {
> > struct acpi_pci_root *root;
> > acpi_status status;
> > @@ -370,7 +361,7 @@ static acpi_status acpi_pci_osc_control_set(acpi_handle handle, u32 *mask, u32 r
> >
> > /* Need to check the available controls bits before requesting them. */
> > while (*mask) {
> > - status = acpi_pci_query_osc(root, root->osc_support_set, mask);
> > + status = acpi_pci_query_osc(root, support, mask);
> > if (ACPI_FAILURE(status))
> > return status;
> > if (ctrl == *mask)
> > @@ -433,18 +424,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
> > support |= OSC_PCI_EDR_SUPPORT;
> >
> > decode_osc_support(root, "OS supports", support);
> > - status = acpi_pci_osc_support(root, support);
> > - if (ACPI_FAILURE(status)) {
> > - *no_aspm = 1;
> > -
> > - /* _OSC is optional for PCI host bridges */
> > - if ((status == AE_NOT_FOUND) && !is_pcie)
> > - return;
> > -
> > - dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> > - acpi_format_exception(status));
> > - return;
> > - }
> >
> > if (pcie_ports_disabled) {
> > dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
>
> Also not related to this patch, but it seems pointless to compute and
> decode "support" above when we're not going to use _OSC at all. I
> think the "pcie_ports_disabled" test should be the very first thing in
> this function (I'm assuming the "pcie_ports=compat" command line
> argument *should* apply even on x86_apple_machine, which it doesn't
> today).
I think I was wrong about this. Even when "pcie_ports_disabled", the
current code *does* evaluate "_OSC(Query, SUPPORT=x, CONTROL=0)",
i.e., it tells the platform what Linux supports, but doesn't request
control of anything.
I think the platform may rely on this knowledge of what the OS
supports. For example, if we tell the platform that Linux has
OSC_PCI_EXT_CONFIG_SUPPORT, that may change the behavior of ASL that
accesses config space.
So I don't think it's safe to move this test to the beginning of the
function as I proposed.
For the same reason, I'm not sure that it's safe to remove
acpi_pci_osc_support() as in this patch. If either
"pcie_ports_disabled" or Linux doesn't support everything in
ACPI_PCIE_REQ_SUPPORT, we will never evaluate _OSC at all, so
the platform won't know that Linux has OSC_PCI_SEGMENT_GROUPS_SUPPORT,
OSC_PCI_HPX_TYPE_3_SUPPORT, OSC_PCI_EXT_CONFIG_SUPPORT, etc.
Bjorn
