On Fri, Jun 4, 2021 at 7:09 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > 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. No, it isn't AFAICS. [I was about to comment on this, but you were faster.] > 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. Right.