PCI/APCI: Move acpi_pci_osc_support() check to negotiation phase

Commit Message

Joerg Roedel June 3, 2021, 12:48 p.m. UTC

From: Joerg Roedel <[e-mail]>

The acpi_pci_osc_support() does an _OSC query with _OSC supported set
to what the OS supports but a zero _OSC control value. This is
problematic on some platforms where the firmware allows to configure
whether DPC is under OS or Firmware control.

When DPC is configured to be under OS control these platforms will
issue a warning in the firmware log that the OS does not support DPC.

Avoid an _OSC query with _OSC control set to zero by moving the
supported check into the acpi_pci_osc_control_set() path. This is
still early enough to fail as nothing before that depends on the
results of acpi_pci_osc_support().

As a result the acpi_pci_osc_support() function can be removed and
acpi_pci_query_osc() be simplified because it no longer called with a
NULL pointer for *control.

Signed-off-by: Joerg Roedel <[e-mail]>
---
 drivers/acpi/pci_root.c | 50 ++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

Comments

Bjorn Helgaas June 3, 2021, 8:50 p.m. UTC | #1

On Thu, Jun 03, 2021 at 02:48:14PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[e-mail]>

I like this patch a lot and I plan to apply it because you've managed
to simplify the nasty _OSC path a little bit.  But I'm confused about
the justification.

> The acpi_pci_osc_support() does an _OSC query with _OSC supported set
> to what the OS supports but a zero _OSC control value. This is
> problematic on some platforms where the firmware allows to configure
> whether DPC is under OS or Firmware control.
>
> When DPC is configured to be under OS control these platforms will
> issue a warning in the firmware log that the OS does not support DPC.

My understanding is that DPC is under platform control until the OS
requests it via _OSC(Request, Control & OSC_PCI_EXPRESS_DPC_CONTROL)
and the platform grants it.  And after the OS is granted control of
DPC, it must preserve OSC_PCI_EXPRESS_DPC_CONTROL in all subsequent
_OSC calls (i.e., there is no way for the OS to relinquish DPC
control).

So what does it mean for "DPC to be under OS control, but the OS does
_OSC(Query, Control=0)"?  That doesn't sound like a legal sequence:
the OS has already been granted DPC control, but it failed to preserve
OSC_PCI_EXPRESS_DPC_CONTROL?

If instead you mean that the OS has *not* been granted DPC control,
but does _OSC(Query, SUPPORT=x, CONTROL=0), I think that means the OS
is telling the platform what it supports but not requesting anything.
That sounds legal to me, so if firmware complains about it, I would
say it's a firmware problem.

> Avoid an _OSC query with _OSC control set to zero by moving the
> supported check into the acpi_pci_osc_control_set() path. This is
> still early enough to fail as nothing before that depends on the
> results of acpi_pci_osc_support().
> 
> As a result the acpi_pci_osc_support() function can be removed and
> acpi_pci_query_osc() be simplified because it no longer called with a
> NULL pointer for *control.

So I think we should do this, but not because it avoids a firmware
warning, which looks like a firmware bug to me.  We should do it just
because it simplifies this ugly code.

But please help me out if I'm misunderstanding something above.  I'm
never confident that I really understand _OSC.

> Signed-off-by: Joerg Roedel <[e-mail]>
> ---
>  drivers/acpi/pci_root.c | 50 ++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index dcd593766a64..530ecf4970b1 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -199,16 +199,11 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
>  
>  	support &= OSC_PCI_SUPPORT_MASKS;
>  	support |= root->osc_support_set;
> +	*control &= OSC_PCI_CONTROL_MASKS;

Unrelated to *this* patch, but I don't understand the point of
OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS.  These are all
internal static functions and it looks like pointless work to apply
masks here and in acpi_pci_osc_control_set().

I'm happy to make this change, but if you do it, please make it a
separate patch for bisection purposes.

>  	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
>  	capbuf[OSC_SUPPORT_DWORD] = support;
> -	if (control) {
> -		*control &= OSC_PCI_CONTROL_MASKS;
> -		capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
> -	} else {
> -		/* Run _OSC query only with existing controls. */
> -		capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
> -	}
> +	capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
>  
>  	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
>  	if (ACPI_SUCCESS(status)) {

We can also drop the "if (control)" check inside the ACPI_SUCCESS()
block, can't we?

> @@ -219,11 +214,6 @@ static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
>  	return status;
>  }
>  
> -static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
> -{
> -	return acpi_pci_query_osc(root, flags, NULL);
> -}
> -
>  struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
>  {
>  	struct acpi_pci_root *root;
> @@ -346,7 +336,8 @@ EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
>   * _OSC bits the BIOS has granted control of, but its contents are meaningless
>   * on failure.
>   **/
> -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).

Again, I'm happy to do this if it makes sense to you.

> @@ -483,7 +462,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  
>  	requested = control;
>  	status = acpi_pci_osc_control_set(handle, &control,
> -					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
> +					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL,
> +					  support);
>  	if (ACPI_SUCCESS(status)) {
>  		decode_osc_control(root, "OS now controls", control);
>  		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
> @@ -496,10 +476,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  			*no_aspm = 1;
>  		}
>  	} else {
> -		decode_osc_control(root, "OS requested", requested);
> -		decode_osc_control(root, "platform willing to grant", control);
> -		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> -			acpi_format_exception(status));
> +		/* Platform wants to control PCIe features */

Or _OSC just failed because of an OS or firmware defect ;)

> +		root->osc_support_set = 0;
>  
>  		/*
>  		 * We want to disable ASPM here, but aspm_disabled
> @@ -509,6 +487,16 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  		 * root scan.
>  		 */
>  		*no_aspm = 1;
> +
> +		/* _OSC is optional for PCI host bridges */
> +		if ((status == AE_NOT_FOUND) && !is_pcie)
> +			return;
> +
> +		decode_osc_control(root, "OS requested", requested);
> +		decode_osc_control(root, "platform willing to grant", control);
> +		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
> +			acpi_format_exception(status));
> +
>  	}
>  }
>  
> -- 
> 2.31.1
>

Bjorn Helgaas June 4, 2021, 5:09 p.m. UTC | #2

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 <[e-mail]>
> > ...

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

Rafael J. Wysocki June 7, 2021, 12:56 p.m. UTC | #3

On Fri, Jun 4, 2021 at 7:09 PM Bjorn Helgaas <[e-mail]> 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 <[e-mail]>
> > > ...
>
> > > -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.

Joerg Roedel June 7, 2021, 2:10 p.m. UTC | #4

Hi Bjorn,

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:

> If instead you mean that the OS has *not* been granted DPC control,
> but does _OSC(Query, SUPPORT=x, CONTROL=0), I think that means the OS
> is telling the platform what it supports but not requesting anything.
> That sounds legal to me, so if firmware complains about it, I would
> say it's a firmware problem.

I think it depends on how you look at it. The machine I was working with
has a BIOS setting where one can configure that DPC is controlled by the
OS. When it is configured that way, then the BIOS will issue an error
when an _OSC query is made with control set to 0. This is because it
indicates to the BIOS that the OS does not take control over DPC and
thus that the OS does not support it. The BIOS will issue a warning into
its log and when the Linux later takes control the warning is already
there.

> But please help me out if I'm misunderstanding something above.  I'm
> never confident that I really understand _OSC.

I am also not an _OSC expert, but you an Rafael already provided good
feedback on the necessity of at least one _OSC call, even when Linux
does not want to take control.

> Unrelated to *this* patch, but I don't understand the point of
> OSC_PCI_SUPPORT_MASKS and OSC_PCI_CONTROL_MASKS.  These are all
> internal static functions and it looks like pointless work to apply
> masks here and in acpi_pci_osc_control_set().

Okay, I will add a separate patch removing thos after this change.

> >  	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
> >  	if (ACPI_SUCCESS(status)) {
> 
> We can also drop the "if (control)" check inside the ACPI_SUCCESS()
> block, can't we?

Right, fixed that up.

Regards,

	Joerg

Joerg Roedel June 7, 2021, 2:14 p.m. UTC | #5

On Mon, Jun 07, 2021 at 02:56:24PM +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 4, 2021 at 7:09 PM Bjorn Helgaas <[e-mail]> wrote:
> >  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.

Thanks Bjorn and Rafael. So I think the important thing to do is to
issue at least one _OSC call even when Linux is not trying to take
control of anything.

I look into a clean way to do this and get the kernel messages right.
One thing to change is probably only calculating 'control' if
!pcie_ports_disabled in negotiate_os_control().

Regards,

	Joerg

Rafael J. Wysocki June 7, 2021, 2:18 p.m. UTC | #6

On Mon, Jun 7, 2021 at 4:14 PM Joerg Roedel <[e-mail]> wrote:
>
> On Mon, Jun 07, 2021 at 02:56:24PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Jun 4, 2021 at 7:09 PM Bjorn Helgaas <[e-mail]> wrote:
> > >  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.
>
> Thanks Bjorn and Rafael. So I think the important thing to do is to
> issue at least one _OSC call even when Linux is not trying to take
> control of anything.
>
> I look into a clean way to do this and get the kernel messages right.
> One thing to change is probably only calculating 'control' if
> !pcie_ports_disabled in negotiate_os_control().

Please also see
https://lore.kernel.org/linux-acpi/[e-mail]/
for possible clashes etc.

Bjorn Helgaas June 7, 2021, 3:43 p.m. UTC | #7

On Mon, Jun 07, 2021 at 04:10:30PM +0200, Joerg Roedel wrote:
> Hi Bjorn,
> 
> 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:
> 
> > If instead you mean that the OS has *not* been granted DPC control,
> > but does _OSC(Query, SUPPORT=x, CONTROL=0), I think that means the OS
> > is telling the platform what it supports but not requesting anything.
> > That sounds legal to me, so if firmware complains about it, I would
> > say it's a firmware problem.
> 
> I think it depends on how you look at it. The machine I was working with
> has a BIOS setting where one can configure that DPC is controlled by the
> OS. When it is configured that way, then the BIOS will issue an error
> when an _OSC query is made with control set to 0. This is because it
> indicates to the BIOS that the OS does not take control over DPC and
> thus that the OS does not support it. The BIOS will issue a warning into
> its log and when the Linux later takes control the warning is already
> there.

I think that BIOS setting is misguided.  _OSC is designed around the
assumption that features in the Control field start out being
controlled by the platform, and they stay that way until the OS
requests control of a feature and the platform grants it.

It makes no sense to me to configure the BIOS in advance to say "the
OS controls DPC."  The BIOS has no control over what the OS will do,
and it can't behave as though the OS controls DPC until the OS
actually requests that.

I also think the warning is overly aggressive.  _OSC is clearly
designed to be evaluated multiple times, and the OS is allowed to
request control of more features each time (ACPI v6.3, sec 6.2.11.1.1,
6.2.11.1.3).

Bjorn

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index dcd593766a64..530ecf4970b1 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -199,16 +199,11 @@  static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
 
 	support &= OSC_PCI_SUPPORT_MASKS;
 	support |= root->osc_support_set;
+	*control &= OSC_PCI_CONTROL_MASKS;
 
 	capbuf[OSC_QUERY_DWORD] = OSC_QUERY_ENABLE;
 	capbuf[OSC_SUPPORT_DWORD] = support;
-	if (control) {
-		*control &= OSC_PCI_CONTROL_MASKS;
-		capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
-	} else {
-		/* Run _OSC query only with existing controls. */
-		capbuf[OSC_CONTROL_DWORD] = root->osc_control_set;
-	}
+	capbuf[OSC_CONTROL_DWORD] = *control | root->osc_control_set;
 
 	status = acpi_pci_run_osc(root->device->handle, capbuf, &result);
 	if (ACPI_SUCCESS(status)) {
@@ -219,11 +214,6 @@  static acpi_status acpi_pci_query_osc(struct acpi_pci_root *root,
 	return status;
 }
 
-static acpi_status acpi_pci_osc_support(struct acpi_pci_root *root, u32 flags)
-{
-	return acpi_pci_query_osc(root, flags, NULL);
-}
-
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
 {
 	struct acpi_pci_root *root;
@@ -346,7 +336,8 @@  EXPORT_SYMBOL_GPL(acpi_get_pci_dev);
  * _OSC bits the BIOS has granted control of, but its contents are meaningless
  * on failure.
  **/
-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");
@@ -483,7 +462,8 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 
 	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
-					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);
+					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL,
+					  support);
 	if (ACPI_SUCCESS(status)) {
 		decode_osc_control(root, "OS now controls", control);
 		if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM) {
@@ -496,10 +476,8 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			*no_aspm = 1;
 		}
 	} else {
-		decode_osc_control(root, "OS requested", requested);
-		decode_osc_control(root, "platform willing to grant", control);
-		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
-			acpi_format_exception(status));
+		/* Platform wants to control PCIe features */
+		root->osc_support_set = 0;
 
 		/*
 		 * We want to disable ASPM here, but aspm_disabled
@@ -509,6 +487,16 @@  static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		 * root scan.
 		 */
 		*no_aspm = 1;
+
+		/* _OSC is optional for PCI host bridges */
+		if ((status == AE_NOT_FOUND) && !is_pcie)
+			return;
+
+		decode_osc_control(root, "OS requested", requested);
+		decode_osc_control(root, "platform willing to grant", control);
+		dev_info(&device->dev, "_OSC: platform retains control of PCIe features (%s)\n",
+			acpi_format_exception(status));
+
 	}
 }