[PATCH v12 0/5] Simplify PCIe native ownership

[PATCH v12 0/5] Simplify PCIe native ownership

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is Sathy's work with a few tweaks on top.

I dropped the DPC pcie_ports_dpc_native changes for now just because I
haven't had time to understand it all.  We currently ignore the
OSC_PCI_EXPRESS_DPC_CONTROL bit, which seems wrong.  We might want to start
looking at it, but we should try to make that a separate patch that's as
small as possible.

Changes since v11:
 * Add bugfix for DPC with no AER Capability
 * Split OSC_OWNER trivial changes from pcie_ports_native changes
 * Temporarily drop pcie_ports_dpc_native changes

v11 posting: https://lore.kernel.org/r/cover.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com

Bjorn Helgaas (2):
  PCI/DPC: Ignore devices with no AER Capability
  PCI/ACPI: Centralize pci_aer_available() checking

Kuppuswamy Sathyanarayanan (3):
  PCI: Assume control of portdrv-related features only when portdrv
    enabled
  PCI/ACPI: Tidy _OSC control bit checking
  PCI/ACPI: Centralize pcie_ports_native checking

 drivers/acpi/pci_root.c           | 49 ++++++++++++++++++++++++-------
 drivers/pci/hotplug/pciehp_core.c |  2 +-
 drivers/pci/pci-acpi.c            |  3 --
 drivers/pci/pcie/aer.c            |  2 +-
 drivers/pci/pcie/dpc.c            |  3 ++
 drivers/pci/pcie/portdrv_core.c   |  9 ++----
 drivers/pci/probe.c               |  6 ++--
 7 files changed, 50 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Downstream Ports may support DPC regardless of whether they support AER
(see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
"pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
depends on the AER Capability.

dpc_probe() previously failed if:

  !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
  !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law

so it succeeded if:

  pcie_aer_is_native() || pcie_ports_dpc_native

Fail dpc_probe() if the device has no AER Capability.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Olof Johansson <olof@lixom.net>
---
 drivers/pci/pcie/dpc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e05aba86a317..ed0dbc43d018 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
+	if (!pdev->aer_cap)
+		return -ENOTSUPP;
+
 	if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
 		return -ENOTSUPP;
 
-- 
2.25.1

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Kuppuswamy, Sathyanarayanan]

On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Downstream Ports may support DPC regardless of whether they support AER
> (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
> "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
> the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
> depends on the AER Capability.
> 
> dpc_probe() previously failed if:
> 
>    !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
>    !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
> 
> so it succeeded if:
> 
>    pcie_aer_is_native() || pcie_ports_dpc_native
> 
> Fail dpc_probe() if the device has no AER Capability.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Olof Johansson <olof@lixom.net>
> ---
>   drivers/pci/pcie/dpc.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index e05aba86a317..ed0dbc43d018 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
>   	int status;
>   	u16 ctl, cap;
>   
> +	if (!pdev->aer_cap)
> +		return -ENOTSUPP;
Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?

We don't enable DPC service, if AER service is not enabled. And AER
service is only enabled if AER capability is supported.

So dpc_probe() should not happen if AER capability is not supported?

206 static int get_port_device_capability(struct pci_dev *dev)
...
...
251         if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
252             host->native_dpc &&
253             (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
254                 services |= PCIE_PORT_SERVICE_DPC;
255

> +
>   	if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
>   		return -ENOTSUPP;
>   
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Bjorn Helgaas]

On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Downstream Ports may support DPC regardless of whether they support AER
> > (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
> > "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
> > the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
> > depends on the AER Capability.
> > 
> > dpc_probe() previously failed if:
> > 
> >    !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
> >    !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
> > 
> > so it succeeded if:
> > 
> >    pcie_aer_is_native() || pcie_ports_dpc_native
> > 
> > Fail dpc_probe() if the device has no AER Capability.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Olof Johansson <olof@lixom.net>
> > ---
> >   drivers/pci/pcie/dpc.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index e05aba86a317..ed0dbc43d018 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
> >   	int status;
> >   	u16 ctl, cap;
> > +	if (!pdev->aer_cap)
> > +		return -ENOTSUPP;
> Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
> 
> We don't enable DPC service, if AER service is not enabled. And AER
> service is only enabled if AER capability is supported.
> 
> So dpc_probe() should not happen if AER capability is not supported?

I don't think that's always true.  If I'm reading this right, we have
this:

  get_port_device_capability(...)
  {
  #ifdef CONFIG_PCIEAER
    if (dev->aer_cap && ...)
      services |= PCIE_PORT_SERVICE_AER;
  #endif

    if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
        pci_aer_available() &&
        (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
      services |= PCIE_PORT_SERVICE_DPC;
  }

and in the case where:

  - CONFIG_PCIEAER=y
  - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
  - "dev" has no AER capability
  - "dev" has DPC capability

I think we do enable PCIE_PORT_SERVICE_DPC.

> 206 static int get_port_device_capability(struct pci_dev *dev)
> ...
> ...
> 251         if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> 252             host->native_dpc &&
> 253             (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
> 254                 services |= PCIE_PORT_SERVICE_DPC;
> 255
> 
> > +
> >   	if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
> >   		return -ENOTSUPP;
> > 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Kuppuswamy, Sathyanarayanan]

On 11/28/20 12:24 PM, Bjorn Helgaas wrote:
> On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>> On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Downstream Ports may support DPC regardless of whether they support AER
>>> (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
>>> "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
>>> the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
>>> depends on the AER Capability.
>>>
>>> dpc_probe() previously failed if:
>>>
>>>     !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
>>>     !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
>>>
>>> so it succeeded if:
>>>
>>>     pcie_aer_is_native() || pcie_ports_dpc_native
>>>
>>> Fail dpc_probe() if the device has no AER Capability.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> Cc: Olof Johansson <olof@lixom.net>
>>> ---
>>>    drivers/pci/pcie/dpc.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>> index e05aba86a317..ed0dbc43d018 100644
>>> --- a/drivers/pci/pcie/dpc.c
>>> +++ b/drivers/pci/pcie/dpc.c
>>> @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
>>>    	int status;
>>>    	u16 ctl, cap;
>>> +	if (!pdev->aer_cap)
>>> +		return -ENOTSUPP;
>> Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
>>
>> We don't enable DPC service, if AER service is not enabled. And AER
>> service is only enabled if AER capability is supported.
>>
>> So dpc_probe() should not happen if AER capability is not supported?
> 
> I don't think that's always true.  If I'm reading this right, we have
> this:
> 
>    get_port_device_capability(...)
>    {
>    #ifdef CONFIG_PCIEAER
>      if (dev->aer_cap && ...)
>        services |= PCIE_PORT_SERVICE_AER;
>    #endif
> 
>      if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>          pci_aer_available() &&
>          (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>        services |= PCIE_PORT_SERVICE_DPC;
>    }
> 
> and in the case where:
> 
>    - CONFIG_PCIEAER=y
>    - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
>    - "dev" has no AER capability
>    - "dev" has DPC capability
> 
> I think we do enable PCIE_PORT_SERVICE_DPC.
Got it. But further looking into it, I am wondering whether
we should keep this dependency? Currently we just use it to
dump the error information. Do we need to create dependency
between DPC and AER (which is functionality not dependent) just
to see more details about the error?
> 
>> 206 static int get_port_device_capability(struct pci_dev *dev)
>> ...
>> ...
>> 251         if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>> 252             host->native_dpc &&
>> 253             (host->native_dpc || (services & PCIE_PORT_SERVICE_AER)))
>> 254                 services |= PCIE_PORT_SERVICE_DPC;
>> 255
>>
>>> +
>>>    	if (!pcie_aer_is_native(pdev) && !pcie_ports_dpc_native)
>>>    		return -ENOTSUPP;
>>>
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Bjorn Helgaas]

On Sat, Nov 28, 2020 at 01:49:46PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> On 11/28/20 12:24 PM, Bjorn Helgaas wrote:
> > On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > 
> > > > Downstream Ports may support DPC regardless of whether they support AER
> > > > (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
> > > > "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
> > > > the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
> > > > depends on the AER Capability.
> > > > 
> > > > dpc_probe() previously failed if:
> > > > 
> > > >     !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
> > > >     !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
> > > > 
> > > > so it succeeded if:
> > > > 
> > > >     pcie_aer_is_native() || pcie_ports_dpc_native
> > > > 
> > > > Fail dpc_probe() if the device has no AER Capability.
> > > > 
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Olof Johansson <olof@lixom.net>
> > > > ---
> > > >    drivers/pci/pcie/dpc.c | 3 +++
> > > >    1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > index e05aba86a317..ed0dbc43d018 100644
> > > > --- a/drivers/pci/pcie/dpc.c
> > > > +++ b/drivers/pci/pcie/dpc.c
> > > > @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
> > > >    	int status;
> > > >    	u16 ctl, cap;
> > > > +	if (!pdev->aer_cap)
> > > > +		return -ENOTSUPP;
> > > Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
> > > 
> > > We don't enable DPC service, if AER service is not enabled. And AER
> > > service is only enabled if AER capability is supported.
> > > 
> > > So dpc_probe() should not happen if AER capability is not supported?
> > 
> > I don't think that's always true.  If I'm reading this right, we have
> > this:
> > 
> >    get_port_device_capability(...)
> >    {
> >    #ifdef CONFIG_PCIEAER
> >      if (dev->aer_cap && ...)
> >        services |= PCIE_PORT_SERVICE_AER;
> >    #endif
> > 
> >      if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> >          pci_aer_available() &&
> >          (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> >        services |= PCIE_PORT_SERVICE_DPC;
> >    }
> > 
> > and in the case where:
> > 
> >    - CONFIG_PCIEAER=y
> >    - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
> >    - "dev" has no AER capability
> >    - "dev" has DPC capability
> > 
> > I think we do enable PCIE_PORT_SERVICE_DPC.
> Got it. But further looking into it, I am wondering whether
> we should keep this dependency? Currently we just use it to
> dump the error information. Do we need to create dependency
> between DPC and AER (which is functionality not dependent) just
> to see more details about the error?

That's a good question, but I don't really want to get into the actual
operation of the AER and DPC drivers in this series, so maybe
something we should explore later.

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Kuppuswamy, Sathyanarayanan]

On 11/28/20 1:53 PM, Bjorn Helgaas wrote:
> On Sat, Nov 28, 2020 at 01:49:46PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>> On 11/28/20 12:24 PM, Bjorn Helgaas wrote:
>>> On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>>>> On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>>
>>>>> Downstream Ports may support DPC regardless of whether they support AER
>>>>> (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
>>>>> "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
>>>>> the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
>>>>> depends on the AER Capability.
>>>>>
>>>>> dpc_probe() previously failed if:
>>>>>
>>>>>      !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
>>>>>      !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
>>>>>
>>>>> so it succeeded if:
>>>>>
>>>>>      pcie_aer_is_native() || pcie_ports_dpc_native
>>>>>
>>>>> Fail dpc_probe() if the device has no AER Capability.
>>>>>
>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>> Cc: Olof Johansson <olof@lixom.net>
>>>>> ---
>>>>>     drivers/pci/pcie/dpc.c | 3 +++
>>>>>     1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>>>> index e05aba86a317..ed0dbc43d018 100644
>>>>> --- a/drivers/pci/pcie/dpc.c
>>>>> +++ b/drivers/pci/pcie/dpc.c
>>>>> @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
>>>>>     	int status;
>>>>>     	u16 ctl, cap;
>>>>> +	if (!pdev->aer_cap)
>>>>> +		return -ENOTSUPP;
>>>> Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
>>>>
>>>> We don't enable DPC service, if AER service is not enabled. And AER
>>>> service is only enabled if AER capability is supported.
>>>>
>>>> So dpc_probe() should not happen if AER capability is not supported?
>>>
>>> I don't think that's always true.  If I'm reading this right, we have
>>> this:
>>>
>>>     get_port_device_capability(...)
>>>     {
>>>     #ifdef CONFIG_PCIEAER
>>>       if (dev->aer_cap && ...)
>>>         services |= PCIE_PORT_SERVICE_AER;
>>>     #endif
>>>
>>>       if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>>>           pci_aer_available() &&
>>>           (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>>>         services |= PCIE_PORT_SERVICE_DPC;
>>>     }
>>>
>>> and in the case where:
>>>
>>>     - CONFIG_PCIEAER=y
>>>     - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
>>>     - "dev" has no AER capability
>>>     - "dev" has DPC capability
>>>
>>> I think we do enable PCIE_PORT_SERVICE_DPC.
>> Got it. But further looking into it, I am wondering whether
>> we should keep this dependency? Currently we just use it to
>> dump the error information. Do we need to create dependency
>> between DPC and AER (which is functionality not dependent) just
>> to see more details about the error?
> 
> That's a good question, but I don't really want to get into the actual
> operation of the AER and DPC drivers in this series, so maybe
> something we should explore later.
In that case, can you move this check to drivers/pci/pcie/portdrv_core.c?
I don't see the point of distributed checks in both get_port_device_capability()
and dpc_probe().
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Bjorn Helgaas]

On Sat, Nov 28, 2020 at 01:56:23PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> On 11/28/20 1:53 PM, Bjorn Helgaas wrote:
> > On Sat, Nov 28, 2020 at 01:49:46PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > On 11/28/20 12:24 PM, Bjorn Helgaas wrote:
> > > > On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > > > On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> > > > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > 
> > > > > > Downstream Ports may support DPC regardless of whether they support AER
> > > > > > (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
> > > > > > "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
> > > > > > the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
> > > > > > depends on the AER Capability.
> > > > > > 
> > > > > > dpc_probe() previously failed if:
> > > > > > 
> > > > > >      !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
> > > > > >      !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
> > > > > > 
> > > > > > so it succeeded if:
> > > > > > 
> > > > > >      pcie_aer_is_native() || pcie_ports_dpc_native
> > > > > > 
> > > > > > Fail dpc_probe() if the device has no AER Capability.
> > > > > > 
> > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > Cc: Olof Johansson <olof@lixom.net>
> > > > > > ---
> > > > > >     drivers/pci/pcie/dpc.c | 3 +++
> > > > > >     1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > > index e05aba86a317..ed0dbc43d018 100644
> > > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > > @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
> > > > > >     	int status;
> > > > > >     	u16 ctl, cap;
> > > > > > +	if (!pdev->aer_cap)
> > > > > > +		return -ENOTSUPP;
> > > > > Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
> > > > > 
> > > > > We don't enable DPC service, if AER service is not enabled. And AER
> > > > > service is only enabled if AER capability is supported.
> > > > > 
> > > > > So dpc_probe() should not happen if AER capability is not supported?
> > > > 
> > > > I don't think that's always true.  If I'm reading this right, we have
> > > > this:
> > > > 
> > > >     get_port_device_capability(...)
> > > >     {
> > > >     #ifdef CONFIG_PCIEAER
> > > >       if (dev->aer_cap && ...)
> > > >         services |= PCIE_PORT_SERVICE_AER;
> > > >     #endif
> > > > 
> > > >       if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> > > >           pci_aer_available() &&
> > > >           (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> > > >         services |= PCIE_PORT_SERVICE_DPC;
> > > >     }
> > > > 
> > > > and in the case where:
> > > > 
> > > >     - CONFIG_PCIEAER=y
> > > >     - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
> > > >     - "dev" has no AER capability
> > > >     - "dev" has DPC capability
> > > > 
> > > > I think we do enable PCIE_PORT_SERVICE_DPC.
> > > Got it. But further looking into it, I am wondering whether
> > > we should keep this dependency? Currently we just use it to
> > > dump the error information. Do we need to create dependency
> > > between DPC and AER (which is functionality not dependent) just
> > > to see more details about the error?
> > 
> > That's a good question, but I don't really want to get into the actual
> > operation of the AER and DPC drivers in this series, so maybe
> > something we should explore later.

> In that case, can you move this check to
> drivers/pci/pcie/portdrv_core.c?  I don't see the point of
> distributed checks in both get_port_device_capability() and
> dpc_probe().

I totally agree that these distributed checks are terrible, but my
long-term hope is to get rid of portdrv and handle these "services"
more like we handle other capabilities.  For example, maybe we can
squash dpc_probe() into pci_dpc_init(), so I'd actually like to move
things from get_port_device_capability() into dpc_probe().

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Kuppuswamy, Sathyanarayanan]

On 11/28/20 3:25 PM, Bjorn Helgaas wrote:
> On Sat, Nov 28, 2020 at 01:56:23PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>> On 11/28/20 1:53 PM, Bjorn Helgaas wrote:
>>> On Sat, Nov 28, 2020 at 01:49:46PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>>>> On 11/28/20 12:24 PM, Bjorn Helgaas wrote:
>>>>> On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>>>>>> On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
>>>>>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>>
>>>>>>> Downstream Ports may support DPC regardless of whether they support AER
>>>>>>> (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
>>>>>>> "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
>>>>>>> the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
>>>>>>> depends on the AER Capability.
>>>>>>>
>>>>>>> dpc_probe() previously failed if:
>>>>>>>
>>>>>>>       !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
>>>>>>>       !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
>>>>>>>
>>>>>>> so it succeeded if:
>>>>>>>
>>>>>>>       pcie_aer_is_native() || pcie_ports_dpc_native
>>>>>>>
>>>>>>> Fail dpc_probe() if the device has no AER Capability.
>>>>>>>
>>>>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>> Cc: Olof Johansson <olof@lixom.net>
>>>>>>> ---
>>>>>>>      drivers/pci/pcie/dpc.c | 3 +++
>>>>>>>      1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>>>>>> index e05aba86a317..ed0dbc43d018 100644
>>>>>>> --- a/drivers/pci/pcie/dpc.c
>>>>>>> +++ b/drivers/pci/pcie/dpc.c
>>>>>>> @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
>>>>>>>      	int status;
>>>>>>>      	u16 ctl, cap;
>>>>>>> +	if (!pdev->aer_cap)
>>>>>>> +		return -ENOTSUPP;
>>>>>> Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
>>>>>>
>>>>>> We don't enable DPC service, if AER service is not enabled. And AER
>>>>>> service is only enabled if AER capability is supported.
>>>>>>
>>>>>> So dpc_probe() should not happen if AER capability is not supported?
>>>>>
>>>>> I don't think that's always true.  If I'm reading this right, we have
>>>>> this:
>>>>>
>>>>>      get_port_device_capability(...)
>>>>>      {
>>>>>      #ifdef CONFIG_PCIEAER
>>>>>        if (dev->aer_cap && ...)
>>>>>          services |= PCIE_PORT_SERVICE_AER;
>>>>>      #endif
>>>>>
>>>>>        if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>>>>>            pci_aer_available() &&
>>>>>            (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>>>>>          services |= PCIE_PORT_SERVICE_DPC;
>>>>>      }
>>>>>
>>>>> and in the case where:
>>>>>
>>>>>      - CONFIG_PCIEAER=y
>>>>>      - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
>>>>>      - "dev" has no AER capability
>>>>>      - "dev" has DPC capability
>>>>>
>>>>> I think we do enable PCIE_PORT_SERVICE_DPC.
>>>> Got it. But further looking into it, I am wondering whether
>>>> we should keep this dependency? Currently we just use it to
>>>> dump the error information. Do we need to create dependency
>>>> between DPC and AER (which is functionality not dependent) just
>>>> to see more details about the error?
>>>
>>> That's a good question, but I don't really want to get into the actual
>>> operation of the AER and DPC drivers in this series, so maybe
>>> something we should explore later.
> 
>> In that case, can you move this check to
>> drivers/pci/pcie/portdrv_core.c?  I don't see the point of
>> distributed checks in both get_port_device_capability() and
>> dpc_probe().
> 
> I totally agree that these distributed checks are terrible, but my
> long-term hope is to get rid of portdrv and handle these "services"
> more like we handle other capabilities.  For example, maybe we can
> squash dpc_probe() into pci_dpc_init(), so I'd actually like to move
> things from get_port_device_capability() into dpc_probe().
Removing the service driver model will be a major overhaul. It would
affect even the error recovery drivers. You can find motivation
for service drivers in Documentation/PCI/pciebus-howto.rst.

But till we fix this part, I recommend grouping all dependency checks
to one place (either dpc_probe() or portdrv service driver).
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/5] PCI/DPC: Ignore devices with no AER Capability

[Bjorn Helgaas]

On Sat, Nov 28, 2020 at 08:32:32PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> On 11/28/20 3:25 PM, Bjorn Helgaas wrote:
> > On Sat, Nov 28, 2020 at 01:56:23PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > On 11/28/20 1:53 PM, Bjorn Helgaas wrote:
> > > > On Sat, Nov 28, 2020 at 01:49:46PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > > > On 11/28/20 12:24 PM, Bjorn Helgaas wrote:
> > > > > > On Wed, Nov 25, 2020 at 06:01:57PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > > > > > On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> > > > > > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > > > 
> > > > > > > > Downstream Ports may support DPC regardless of whether they support AER
> > > > > > > > (see PCIe r5.0, sec 6.2.10.2).  Previously, if the user booted with
> > > > > > > > "pcie_ports=dpc-native", it was possible for dpc_probe() to succeed even if
> > > > > > > > the device had no AER Capability, but dpc_get_aer_uncorrect_severity()
> > > > > > > > depends on the AER Capability.
> > > > > > > > 
> > > > > > > > dpc_probe() previously failed if:
> > > > > > > > 
> > > > > > > >       !pcie_aer_is_native(pdev) && !pcie_ports_dpc_native
> > > > > > > >       !(pcie_aer_is_native() || pcie_ports_dpc_native)    # by De Morgan's law
> > > > > > > > 
> > > > > > > > so it succeeded if:
> > > > > > > > 
> > > > > > > >       pcie_aer_is_native() || pcie_ports_dpc_native
> > > > > > > > 
> > > > > > > > Fail dpc_probe() if the device has no AER Capability.
> > > > > > > > 
> > > > > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > > > > Cc: Olof Johansson <olof@lixom.net>
> > > > > > > > ---
> > > > > > > >      drivers/pci/pcie/dpc.c | 3 +++
> > > > > > > >      1 file changed, 3 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > > > > index e05aba86a317..ed0dbc43d018 100644
> > > > > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > > > > @@ -287,6 +287,9 @@ static int dpc_probe(struct pcie_device *dev)
> > > > > > > >      	int status;
> > > > > > > >      	u16 ctl, cap;
> > > > > > > > +	if (!pdev->aer_cap)
> > > > > > > > +		return -ENOTSUPP;
> > > > > > > Don't we check aer_cap support in drivers/pci/pcie/portdrv_core.c ?
> > > > > > > 
> > > > > > > We don't enable DPC service, if AER service is not enabled. And AER
> > > > > > > service is only enabled if AER capability is supported.
> > > > > > > 
> > > > > > > So dpc_probe() should not happen if AER capability is not supported?
> > > > > > 
> > > > > > I don't think that's always true.  If I'm reading this right, we have
> > > > > > this:
> > > > > > 
> > > > > >      get_port_device_capability(...)
> > > > > >      {
> > > > > >      #ifdef CONFIG_PCIEAER
> > > > > >        if (dev->aer_cap && ...)
> > > > > >          services |= PCIE_PORT_SERVICE_AER;
> > > > > >      #endif
> > > > > > 
> > > > > >        if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> > > > > >            pci_aer_available() &&
> > > > > >            (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> > > > > >          services |= PCIE_PORT_SERVICE_DPC;
> > > > > >      }
> > > > > > 
> > > > > > and in the case where:
> > > > > > 
> > > > > >      - CONFIG_PCIEAER=y
> > > > > >      - booted with "pcie_ports=dpc-native" (pcie_ports_dpc_native is true)
> > > > > >      - "dev" has no AER capability
> > > > > >      - "dev" has DPC capability
> > > > > > 
> > > > > > I think we do enable PCIE_PORT_SERVICE_DPC.
> > > > > Got it. But further looking into it, I am wondering whether
> > > > > we should keep this dependency? Currently we just use it to
> > > > > dump the error information. Do we need to create dependency
> > > > > between DPC and AER (which is functionality not dependent) just
> > > > > to see more details about the error?
> > > > 
> > > > That's a good question, but I don't really want to get into the actual
> > > > operation of the AER and DPC drivers in this series, so maybe
> > > > something we should explore later.
> > 
> > > In that case, can you move this check to
> > > drivers/pci/pcie/portdrv_core.c?  I don't see the point of
> > > distributed checks in both get_port_device_capability() and
> > > dpc_probe().
> > 
> > I totally agree that these distributed checks are terrible, but my
> > long-term hope is to get rid of portdrv and handle these "services"
> > more like we handle other capabilities.  For example, maybe we can
> > squash dpc_probe() into pci_dpc_init(), so I'd actually like to move
> > things from get_port_device_capability() into dpc_probe().
> Removing the service driver model will be a major overhaul. It would
> affect even the error recovery drivers. You can find motivation
> for service drivers in Documentation/PCI/pciebus-howto.rst.

Part of that motivation for portdrv is to allow multiple service
drivers to run simultaneously on the same PCIe port.  That is also
part of the *problem* with portdrv.  There are several PCIe ports that
implement device-specific functionality via their BARs, and there is
currently no way for drivers to cleanly claim those ports because
they're already claimed by portdrv.

> But till we fix this part, I recommend grouping all dependency checks
> to one place (either dpc_probe() or portdrv service driver).

I think they should go in the driver, e.g., dpc_probe().  The typical
model is that bus drivers only match a device with the appropriate
driver, and the device driver does more specialized checks.  For
example, the PCI bus driver only looks at the Vendor ID, Device ID,
and Class code.

But we don't have to solve all of this right now.

Bjorn

[PATCH 2/5] PCI: Assume control of portdrv-related features only when portdrv enabled

[Bjorn Helgaas]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Native control of PME, AER, DPC, and PCIe hotplug depends on the portdrv,
so default to native handling of them only when CONFIG_PCIEPORTBUS is
enabled.

Native control LTR and SHPC hotplug does not depend on portdrv, so we can
always take control of them unless some platform interface, e.g., _OSC,
tells us otherwise.

[bhelgaas: commit log]
Link: https://lore.kernel.org/r/fcbe8a624166a1101a755edfef44a185d32ff493.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/probe.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4289030b0fff..756fa60ca708 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -588,12 +588,14 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
 	 * may implement its own AER handling and use _OSC to prevent the
 	 * OS from interfering.
 	 */
+#ifdef CONFIG_PCIEPORTBUS
 	bridge->native_aer = 1;
 	bridge->native_pcie_hotplug = 1;
-	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
-	bridge->native_ltr = 1;
 	bridge->native_dpc = 1;
+#endif
+	bridge->native_ltr = 1;
+	bridge->native_shpc_hotplug = 1;
 
 	device_initialize(&bridge->dev);
 }
-- 
2.25.1

[PATCH 3/5] PCI/ACPI: Tidy _OSC control bit checking

[Bjorn Helgaas]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Add OSC_OWNER() helper to prettify checking the _OSC control bits to learn
whether the platform has granted us control of PCI features.  No functional
change intended.

[bhelgaas: split to separate patch, commit log]
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index c12b5fb3e8fb..6db071038fd5 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -876,6 +876,12 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
 	__acpi_pci_root_release_info(bridge->release_data);
 }
 
+#define OSC_OWNER(ctrl, bit, flag)	\
+	do {				\
+		if (!(ctrl & bit))	\
+			flag = 0;	\
+	} while (0)
+
 struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 				     struct acpi_pci_root_ops *ops,
 				     struct acpi_pci_root_info *info,
@@ -887,6 +893,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	struct pci_bus *bus;
 	struct pci_host_bridge *host_bridge;
 	union acpi_object *obj;
+	u32 ctrl;
 
 	info->root = root;
 	info->bridge = device;
@@ -912,18 +919,16 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		goto out_release_info;
 
 	host_bridge = to_pci_host_bridge(bus->bridge);
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		host_bridge->native_pcie_hotplug = 0;
-	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
-		host_bridge->native_shpc_hotplug = 0;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
-		host_bridge->native_aer = 0;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
-		host_bridge->native_pme = 0;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
-		host_bridge->native_ltr = 0;
-	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
-		host_bridge->native_dpc = 0;
+
+	ctrl = root->osc_control_set;
+	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_NATIVE_HP_CONTROL,
+		  host_bridge->native_pcie_hotplug);
+	OSC_OWNER(ctrl, OSC_PCI_SHPC_NATIVE_HP_CONTROL,
+		  host_bridge->native_shpc_hotplug);
+	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_AER_CONTROL, host_bridge->native_aer);
+	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_PME_CONTROL, host_bridge->native_pme);
+	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_LTR_CONTROL, host_bridge->native_ltr);
+	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_DPC_CONTROL, host_bridge->native_dpc);
 
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
-- 
2.25.1

[PATCH 4/5] PCI/ACPI: Centralize pcie_ports_native checking

[Bjorn Helgaas]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

If the user booted with "pcie_ports=native", we take control of the PCIe
features unconditionally, regardless of what _OSC says.

Centralize the testing of pcie_ports_native in acpi_pci_root_create(),
where we interpret the _OSC results, so other places only have to check
host_bridge->native_X and we don't have to sprinkle tests of
pcie_ports_native everywhere.

[bhelgaas: commit log, rework OSC_PCI_EXPRESS_CONTROL_MASKS, logging]
Link: https://lore.kernel.org/r/bc87c9e675118960949043a832bed86bc22becbd.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c           | 19 +++++++++++++++++++
 drivers/pci/hotplug/pciehp_core.c |  2 +-
 drivers/pci/pci-acpi.c            |  3 ---
 drivers/pci/pcie/aer.c            |  2 +-
 drivers/pci/pcie/portdrv_core.c   |  9 +++------
 5 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6db071038fd5..36142ed7b8f8 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -882,6 +882,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
 			flag = 0;	\
 	} while (0)
 
+#define FLAG(x)		((x) ? '+' : '-')
+
 struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 				     struct acpi_pci_root_ops *ops,
 				     struct acpi_pci_root_info *info,
@@ -930,6 +932,23 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_LTR_CONTROL, host_bridge->native_ltr);
 	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_DPC_CONTROL, host_bridge->native_dpc);
 
+	if (pcie_ports_native) {
+		dev_info(&root->device->dev, "Taking control of PCIe-related features because \"pcie_ports=native\" specified; may conflict with firmware\n");
+		host_bridge->native_pcie_hotplug = 1;
+		host_bridge->native_aer = 1;
+		host_bridge->native_pme = 1;
+		host_bridge->native_ltr = 1;
+		host_bridge->native_dpc = 1;
+	}
+
+	dev_info(&root->device->dev, "OS native features: SHPCHotplug%c PCIeHotplug%c PME%c AER%c DPC%c LTR%c\n",
+		FLAG(host_bridge->native_shpc_hotplug),
+		FLAG(host_bridge->native_pcie_hotplug),
+		FLAG(host_bridge->native_pme),
+		FLAG(host_bridge->native_aer),
+		FLAG(host_bridge->native_dpc),
+		FLAG(host_bridge->native_ltr));
+
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
 	 * exists and returns 0, we must preserve any PCI resource
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index ad3393930ecb..d1831e6bf60a 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -256,7 +256,7 @@ static bool pme_is_native(struct pcie_device *dev)
 	const struct pci_host_bridge *host;
 
 	host = pci_find_host_bridge(dev->port->bus);
-	return pcie_ports_native || host->native_pme;
+	return host->native_pme;
 }
 
 static void pciehp_disable_interrupt(struct pcie_device *dev)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index bf03648c2072..a84f75ec6df8 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -800,9 +800,6 @@ bool pciehp_is_native(struct pci_dev *bridge)
 	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
 		return false;
 
-	if (pcie_ports_native)
-		return true;
-
 	host = pci_find_host_bridge(bridge->bus);
 	return host->native_pcie_hotplug;
 }
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..79bb441139c2 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -219,7 +219,7 @@ int pcie_aer_is_native(struct pci_dev *dev)
 	if (!dev->aer_cap)
 		return 0;
 
-	return pcie_ports_native || host->native_aer;
+	return host->native_aer;
 }
 
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 50a9522ab07d..2a1190e8db60 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -208,8 +208,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
 	int services = 0;
 
-	if (dev->is_hotplug_bridge &&
-	    (pcie_ports_native || host->native_pcie_hotplug)) {
+	if (host->native_pcie_hotplug && dev->is_hotplug_bridge) {
 		services |= PCIE_PORT_SERVICE_HP;
 
 		/*
@@ -221,8 +220,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 #ifdef CONFIG_PCIEAER
-	if (dev->aer_cap && pci_aer_available() &&
-	    (pcie_ports_native || host->native_aer)) {
+	if (host->native_aer && dev->aer_cap && pci_aer_available()) {
 		services |= PCIE_PORT_SERVICE_AER;
 
 		/*
@@ -238,8 +236,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 * Event Collectors can also generate PMEs, but we don't handle
 	 * those yet.
 	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
-	    (pcie_ports_native || host->native_pme)) {
+	if (host->native_pme && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
 		services |= PCIE_PORT_SERVICE_PME;
 
 		/*
-- 
2.25.1

Re: [PATCH 4/5] PCI/ACPI: Centralize pcie_ports_native checking

[Kuppuswamy, Sathyanarayanan]

On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> If the user booted with "pcie_ports=native", we take control of the PCIe
> features unconditionally, regardless of what _OSC says.
> 
> Centralize the testing of pcie_ports_native in acpi_pci_root_create(),
> where we interpret the _OSC results, so other places only have to check
> host_bridge->native_X and we don't have to sprinkle tests of
> pcie_ports_native everywhere.
> 
> [bhelgaas: commit log, rework OSC_PCI_EXPRESS_CONTROL_MASKS, logging]
> Link: https://lore.kernel.org/r/bc87c9e675118960949043a832bed86bc22becbd.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/acpi/pci_root.c           | 19 +++++++++++++++++++
>   drivers/pci/hotplug/pciehp_core.c |  2 +-
>   drivers/pci/pci-acpi.c            |  3 ---
>   drivers/pci/pcie/aer.c            |  2 +-
>   drivers/pci/pcie/portdrv_core.c   |  9 +++------
>   5 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 6db071038fd5..36142ed7b8f8 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -882,6 +882,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
>   			flag = 0;	\
>   	} while (0)
>   
> +#define FLAG(x)		((x) ? '+' : '-')
> +
>   struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>   				     struct acpi_pci_root_ops *ops,
>   				     struct acpi_pci_root_info *info,
> @@ -930,6 +932,23 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>   	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_LTR_CONTROL, host_bridge->native_ltr);
>   	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_DPC_CONTROL, host_bridge->native_dpc);
>   
> +	if (pcie_ports_native) {
> +		dev_info(&root->device->dev, "Taking control of PCIe-related features because \"pcie_ports=native\" specified; may conflict with firmware\n");
> +		host_bridge->native_pcie_hotplug = 1;
> +		host_bridge->native_aer = 1;
> +		host_bridge->native_pme = 1;
> +		host_bridge->native_ltr = 1;
> +		host_bridge->native_dpc = 1;
> +	}
Won't it be better if its merged with above OSC_OWNER() calls? If pcie_ports_native
is set, then above OSC_OWNER() calls for PCIe related features are redundant. Let me
know.
> +
> +	dev_info(&root->device->dev, "OS native features: SHPCHotplug%c PCIeHotplug%c PME%c AER%c DPC%c LTR%c\n",
> +		FLAG(host_bridge->native_shpc_hotplug),
> +		FLAG(host_bridge->native_pcie_hotplug),
> +		FLAG(host_bridge->native_pme),
> +		FLAG(host_bridge->native_aer),
> +		FLAG(host_bridge->native_dpc),
> +		FLAG(host_bridge->native_ltr));
> +
>   	/*
>   	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it
>   	 * exists and returns 0, we must preserve any PCI resource
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index ad3393930ecb..d1831e6bf60a 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -256,7 +256,7 @@ static bool pme_is_native(struct pcie_device *dev)
>   	const struct pci_host_bridge *host;
>   
>   	host = pci_find_host_bridge(dev->port->bus);
> -	return pcie_ports_native || host->native_pme;
> +	return host->native_pme;
>   }
>   
>   static void pciehp_disable_interrupt(struct pcie_device *dev)
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index bf03648c2072..a84f75ec6df8 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -800,9 +800,6 @@ bool pciehp_is_native(struct pci_dev *bridge)
>   	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
>   		return false;
>   
> -	if (pcie_ports_native)
> -		return true;
> -
>   	host = pci_find_host_bridge(bridge->bus);
>   	return host->native_pcie_hotplug;
>   }
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 65dff5f3457a..79bb441139c2 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -219,7 +219,7 @@ int pcie_aer_is_native(struct pci_dev *dev)
>   	if (!dev->aer_cap)
>   		return 0;
>   
> -	return pcie_ports_native || host->native_aer;
> +	return host->native_aer;
>   }
>   
>   int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 50a9522ab07d..2a1190e8db60 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -208,8 +208,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>   	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>   	int services = 0;
>   
> -	if (dev->is_hotplug_bridge &&
> -	    (pcie_ports_native || host->native_pcie_hotplug)) {
> +	if (host->native_pcie_hotplug && dev->is_hotplug_bridge) {
May be this reordering can be split into a different patch ?
>   		services |= PCIE_PORT_SERVICE_HP;
>   
>   		/*
> @@ -221,8 +220,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>   	}
>   
>   #ifdef CONFIG_PCIEAER
> -	if (dev->aer_cap && pci_aer_available() &&
> -	    (pcie_ports_native || host->native_aer)) {
> +	if (host->native_aer && dev->aer_cap && pci_aer_available()) {
same as above.
>   		services |= PCIE_PORT_SERVICE_AER;
>   
>   		/*
> @@ -238,8 +236,7 @@ static int get_port_device_capability(struct pci_dev *dev)
>   	 * Event Collectors can also generate PMEs, but we don't handle
>   	 * those yet.
>   	 */
> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> -	    (pcie_ports_native || host->native_pme)) {
> +	if (host->native_pme && pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
>   		services |= PCIE_PORT_SERVICE_PME;
>   
>   		/*
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 4/5] PCI/ACPI: Centralize pcie_ports_native checking

[Bjorn Helgaas]

On Wed, Nov 25, 2020 at 07:20:53PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > If the user booted with "pcie_ports=native", we take control of the PCIe
> > features unconditionally, regardless of what _OSC says.
> > 
> > Centralize the testing of pcie_ports_native in acpi_pci_root_create(),
> > where we interpret the _OSC results, so other places only have to check
> > host_bridge->native_X and we don't have to sprinkle tests of
> > pcie_ports_native everywhere.
> > 
> > [bhelgaas: commit log, rework OSC_PCI_EXPRESS_CONTROL_MASKS, logging]
> > Link: https://lore.kernel.org/r/bc87c9e675118960949043a832bed86bc22becbd.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >   drivers/acpi/pci_root.c           | 19 +++++++++++++++++++
> >   drivers/pci/hotplug/pciehp_core.c |  2 +-
> >   drivers/pci/pci-acpi.c            |  3 ---
> >   drivers/pci/pcie/aer.c            |  2 +-
> >   drivers/pci/pcie/portdrv_core.c   |  9 +++------
> >   5 files changed, 24 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 6db071038fd5..36142ed7b8f8 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -882,6 +882,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> >   			flag = 0;	\
> >   	} while (0)
> > +#define FLAG(x)		((x) ? '+' : '-')
> > +
> >   struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >   				     struct acpi_pci_root_ops *ops,
> >   				     struct acpi_pci_root_info *info,
> > @@ -930,6 +932,23 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >   	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_LTR_CONTROL, host_bridge->native_ltr);
> >   	OSC_OWNER(ctrl, OSC_PCI_EXPRESS_DPC_CONTROL, host_bridge->native_dpc);
> > +	if (pcie_ports_native) {
> > +		dev_info(&root->device->dev, "Taking control of PCIe-related features because \"pcie_ports=native\" specified; may conflict with firmware\n");
> > +		host_bridge->native_pcie_hotplug = 1;
> > +		host_bridge->native_aer = 1;
> > +		host_bridge->native_pme = 1;
> > +		host_bridge->native_ltr = 1;
> > +		host_bridge->native_dpc = 1;
> > +	}
> Won't it be better if its merged with above OSC_OWNER() calls? If
> pcie_ports_native is set, then above OSC_OWNER() calls for PCIe
> related features are redundant. Let me know.

Yes, I think you're right.  I think it would also be nice to know
exactly which services we're overriding _OSC for, i.e., when we turn
native_aer from 0 to 1 and so on.  If we merge this back in, we'll
have a way to keep track of those.

> > @@ -208,8 +208,7 @@ static int get_port_device_capability(struct pci_dev *dev)
> >   	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> >   	int services = 0;
> > -	if (dev->is_hotplug_bridge &&
> > -	    (pcie_ports_native || host->native_pcie_hotplug)) {
> > +	if (host->native_pcie_hotplug && dev->is_hotplug_bridge) {
> May be this reordering can be split into a different patch ?

Good point, that makes the diff a little easier to read, thanks!

[PATCH 5/5] PCI/ACPI: Centralize pci_aer_available() checking

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Check pci_aer_available() in acpi_pci_root_create() when we're interpreting
_OSC results so host_bridge->native_aer becomes the single way to determine
whether we control AER capabilities.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c         | 3 +++
 drivers/pci/pcie/portdrv_core.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 36142ed7b8f8..f9969b86d3b6 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -941,6 +941,9 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		host_bridge->native_dpc = 1;
 	}
 
+	if (!pci_aer_available())
+		host_bridge->native_aer = 0;
+
 	dev_info(&root->device->dev, "OS native features: SHPCHotplug%c PCIeHotplug%c PME%c AER%c DPC%c LTR%c\n",
 		FLAG(host_bridge->native_shpc_hotplug),
 		FLAG(host_bridge->native_pcie_hotplug),
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 2a1190e8db60..48c14c2471ef 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -220,7 +220,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 #ifdef CONFIG_PCIEAER
-	if (host->native_aer && dev->aer_cap && pci_aer_available()) {
+	if (host->native_aer && dev->aer_cap) {
 		services |= PCIE_PORT_SERVICE_AER;
 
 		/*
-- 
2.25.1

Re: [PATCH v12 0/5] Simplify PCIe native ownership

[Kuppuswamy, Sathyanarayanan]

On 11/25/20 5:18 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> This is Sathy's work with a few tweaks on top.
> 
> I dropped the DPC pcie_ports_dpc_native changes for now just because I
> haven't had time to understand it all.  We currently ignore the
> OSC_PCI_EXPRESS_DPC_CONTROL bit, which seems wrong.  We might want to start
> looking at it, but we should try to make that a separate patch that's as
> small as possible.
Along with above patch, you also left following two cleanup patches. Is this
intentional? Following fixes have no dependency on pcie_ports_dpc_native change.

[PATCH v11 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic
[PATCH v11 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver
> 
> Changes since v11:
>   * Add bugfix for DPC with no AER Capability
>   * Split OSC_OWNER trivial changes from pcie_ports_native changes
>   * Temporarily drop pcie_ports_dpc_native changes
> 
> v11 posting: https://lore.kernel.org/r/cover.1603766889.git.sathyanarayanan.kuppuswamy@linux.intel.com
> 
> Bjorn Helgaas (2):
>    PCI/DPC: Ignore devices with no AER Capability
>    PCI/ACPI: Centralize pci_aer_available() checking
> 
> Kuppuswamy Sathyanarayanan (3):
>    PCI: Assume control of portdrv-related features only when portdrv
>      enabled
>    PCI/ACPI: Tidy _OSC control bit checking
>    PCI/ACPI: Centralize pcie_ports_native checking
> 
>   drivers/acpi/pci_root.c           | 49 ++++++++++++++++++++++++-------
>   drivers/pci/hotplug/pciehp_core.c |  2 +-
>   drivers/pci/pci-acpi.c            |  3 --
>   drivers/pci/pcie/aer.c            |  2 +-
>   drivers/pci/pcie/dpc.c            |  3 ++
>   drivers/pci/pcie/portdrv_core.c   |  9 ++----
>   drivers/pci/probe.c               |  6 ++--
>   7 files changed, 50 insertions(+), 24 deletions(-)
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v12 0/5] Simplify PCIe native ownership

[Kuppuswamy, Sathyanarayanan]

Hi Bjorn,

On 11/25/20 7:48 PM, Kuppuswamy, Sathyanarayanan wrote:
> Along with above patch, you also left following two cleanup patches. Is this
> intentional? Following fixes have no dependency on pcie_ports_dpc_native change.
> 
> [PATCH v11 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic
> [PATCH v11 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver

Any comment? If you want me to add these patches in my re-submission, please
let me know.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v12 0/5] Simplify PCIe native ownership

[Kuppuswamy, Sathyanarayanan]

Hi Bjorn,

On 11/30/20 5:11 PM, Kuppuswamy, Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On 11/25/20 7:48 PM, Kuppuswamy, Sathyanarayanan wrote:
>> Along with above patch, you also left following two cleanup patches. Is this
>> intentional? Following fixes have no dependency on pcie_ports_dpc_native change.
>>
>> [PATCH v11 4/5] PCI/portdrv: Remove redundant pci_aer_available() check in DPC enable logic
>> [PATCH v11 5/5] PCI/DPC: Move AER/DPC dependency checks out of DPC driver
> 
> Any comment? If you want me to add these patches in my re-submission, please
> let me know.
Gentle ping. Any comments?
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer