[v2,1/2] PCI/ACPI: Move _OSC test for native services to top

Move the simple test for when PCIe native services are disabled
closer to the top, prior to where things get more complicated.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/acpi/pci_root.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Hi Aaron,

On Thu, Oct 25, 2018 at 11:01:31AM -0500, Aaron Sierra wrote:
> Move the simple test for when PCIe native services are disabled
> closer to the top, prior to where things get more complicated.
> 
> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
> ---
>  drivers/acpi/pci_root.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 707aafc..eb9f14e 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -441,6 +441,12 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  		return;
>  	}
>  
> +	if (pcie_ports_disabled) {
> +		dev_info(&device->dev,
> +			 "PCIe port services disabled; not requesting _OSC control\n");
> +		return;
> +	}

Today we always set "*no_aspm = 1" if _OSC fails, which means we later
call pcie_no_aspm().

After this patch, when pcie_ports_disabled is "true", we don't even try to
evaluate _OSC, and we will never set *no_aspm, so we will never call
pcie_no_aspm() when pcie_ports_disabled is "true", which happens in these
cases:

  1) CONFIG_PCIEPORTBUS is unset, or
  2) CONFIG_PCIEPORTBUS=y and we booted with "pcie_ports=compat"

Case 1) isn't a problem because pcie_no_aspm() is only implemented when
CONFIG_PCIEASPM=y, and CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, so in
this case today we only call the empty stub pcie_no_aspm() function.

But case 2) is a behavior change that seems unintended.

Even though CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, ASPM doesn't
actually *use* anything provided by PCIEPORTBUS, so I think the ASPM code
is still active and useful even when we boot with "pcie_ports=compat".

Whether CONFIG_PCIEASPM should depend on CONFIG_PCIEPORTBUS is another
question.  I tend to think maybe it should not, but that's an orthogonal
question.

>  	/*
>  	 * All supported architectures that use ACPI have support for
>  	 * PCI domains, so we indicate this in _OSC support capabilities.
> @@ -468,11 +474,6 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
>  		return;
>  	}
>  
> -	if (pcie_ports_disabled) {
> -		dev_info(&device->dev, "PCIe port services disabled; not requesting _OSC control\n");
> -		return;
> -	}
> -
>  	if ((support & ACPI_PCIE_REQ_SUPPORT) != ACPI_PCIE_REQ_SUPPORT) {
>  		decode_osc_support(root, "not requesting OS control; OS requires",
>  				   ACPI_PCIE_REQ_SUPPORT);
> -- 
> 2.7.4
>

----- Original Message -----
> From: "Bjorn Helgaas" <helgaas@kernel.org>
> To: "Aaron Sierra" <asierra@xes-inc.com>
> Sent: Wednesday, January 30, 2019 4:44:15 PM
> Subject: Re: [PATCH v2 1/2] PCI/ACPI: Move _OSC test for native services to top

> Hi Aaron,
> 
> On Thu, Oct 25, 2018 at 11:01:31AM -0500, Aaron Sierra wrote:
>> Move the simple test for when PCIe native services are disabled
>> closer to the top, prior to where things get more complicated.
>> 
>> Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
>> ---
>>  drivers/acpi/pci_root.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 707aafc..eb9f14e 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -441,6 +441,12 @@ static void negotiate_os_control(struct acpi_pci_root
>> *root, int *no_aspm,
>>  		return;
>>  	}
>>  
>> +	if (pcie_ports_disabled) {
>> +		dev_info(&device->dev,
>> +			 "PCIe port services disabled; not requesting _OSC control\n");
>> +		return;
>> +	}
> 
> Today we always set "*no_aspm = 1" if _OSC fails, which means we later
> call pcie_no_aspm().
> 
> After this patch, when pcie_ports_disabled is "true", we don't even try to
> evaluate _OSC, and we will never set *no_aspm, so we will never call
> pcie_no_aspm() when pcie_ports_disabled is "true", which happens in these
> cases:
> 
>  1) CONFIG_PCIEPORTBUS is unset, or
>  2) CONFIG_PCIEPORTBUS=y and we booted with "pcie_ports=compat"
> 
> Case 1) isn't a problem because pcie_no_aspm() is only implemented when
> CONFIG_PCIEASPM=y, and CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, so in
> this case today we only call the empty stub pcie_no_aspm() function.
> 
> But case 2) is a behavior change that seems unintended.
> 
> Even though CONFIG_PCIEASPM depends on CONFIG_PCIEPORTBUS, ASPM doesn't
> actually *use* anything provided by PCIEPORTBUS, so I think the ASPM code
> is still active and useful even when we boot with "pcie_ports=compat".
> 
> Whether CONFIG_PCIEASPM should depend on CONFIG_PCIEPORTBUS is another
> question.  I tend to think maybe it should not, but that's an orthogonal
> question.
> 

Bjorn, thanks for the review. I certainly did not mean to change behavior
to the extent that you describe. This patch is also not really needed by
the second patch in the series, so I will drop this from v3. Sorry for
the noise.

-Aaron