Re: [PATCH] PCI/APCI: Move acpi_pci_osc_support() check to negotiation phase

[Rafael J. Wysocki]

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.