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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Joerg Roedel <joro@8bytes.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	rjw@rjwysocki.net, Len Brown <lenb@kernel.org>,
	linux-pci@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-kernel@vger.kernel.org, jroedel@suse.de
Subject: Re: [PATCH] PCI/APCI: Move acpi_pci_osc_support() check to negotiation phase
Date: Fri, 4 Jun 2021 12:09:38 -0500	[thread overview]
Message-ID: <20210604170938.GA2218177@bjorn-Precision-5520> (raw)
In-Reply-To: <20210603205047.GA2135380@bjorn-Precision-5520>

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@suse.de>
> > ...

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