Re: [PATCH 1/1] PCI/portdrv: Allow DPC if the OS controls AER natively.

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Keith, Lukas]

Hi Matthew, thanks for your work and the patch.

On Sat, Dec 23, 2023 at 02:22:35PM -0700, Matthew W Carlis wrote:
> This change ensures the kernel will use DPC on a supporting device if
> the kernel will also control AER on the Root Ports & RCECs.
> 
> The rules around controlling DPC/AER are somewhat clear in PCIe/ACPI
> specifications. It is recommended to always link control of both to the
> same entity, being the OS or system firmware. The kernel wants to be
> flexible by first having a default policy, but also by providing command
> line parameters to enable us all to do what we want even if it might
> violate the recommendations.
> 
> The following mentioned patch brought the kernels default behavior
> more in line with the specification around AER, but changed its behavior
> around DPC on PCIe Downstream Switch Ports; preventing the kernel from
> controlling DPC on them unless using pcie_ports=dpc-native.
>     * "PCI/portdrv: Allow AER service only for Root Ports & RCECs"
> After this change the behavior around using DPC on PCIe switch ports
> and Root Ports should be as it was before.
> 
> Signed-off-by: Matthew W Carlis <mattc@purestorage.com>
> ---
>  drivers/pci/pcie/portdrv.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 14a4b89a3b83..8e023aa97672 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -257,12 +257,19 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	}
>  
>  	/*
> +	 * _OSC AER Control is required by the OS & requires OS to control AER,
> +	 * but _OSC DPC Control isn't required by the OS to control DPC; however
> +	 * it does require the OS to control DPC. _OSC DPC Control also requres
> +	 * _OSC EDR Control (Error Disconnect Recovery) (PCI Firmware - DPC ECN rev3.2)
> +	 * PCI_Express_Base 6.1, 6.2.11 Determination of DPC Control recommends
> +	 * platform fw or OS always link control of DPC to AER.
> +	 *
>  	 * With dpc-native, allow Linux to use DPC even if it doesn't have
>  	 * permission to use AER.
>  	 */
>  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> -	    pci_aer_available() &&
> -	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> +	    pci_aer_available() && (pcie_ports_dpc_native ||
> +	    (dev->aer_cap && host->native_aer)))
>  		services |= PCIE_PORT_SERVICE_DPC;

This is easier to read if we retain the original line breaks, i.e.,

  -     (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
  +     (pcie_ports_dpc_native || (dev->aer_cap && host->native_aer)))

Prior to d8d2b65a940b, we set PCIE_PORT_SERVICE_AER for a device
whenever it had an AER Capability.  If it had a DPC Capability, we
also set PCIE_PORT_SERVICE_DPC so DPC would work on it.

After d8d2b65a940b, we only set PCIE_PORT_SERVICE_AER for Root Ports
and RCECs because the AER driver only binds to those devices.  We no
longer set PCIE_PORT_SERVICE_DPC for Switch Downstream Ports because
they don't have PCIE_PORT_SERVICE_AER set.

The result is that you need "pcie_ports=dpc-native" to make DPC work
on those devices when you didn't need it before d8d2b65a940b.

That's a regression that we need to fix:
#regzbot introduced: d8d2b65a940b ("PCI/portdrv: Allow AER service only for Root Ports & RCECs")

_OSC directly supports negotiation of DPC ownership, and I think we
should pay attention to what it tell us.  We already request DPC
control and set native_dpc accordingly, but we don't use it here;
currently we only look at it in the unrelated pciehp_ist() path.

Can you try the patch below and see if it resolves the problem?

I don't think we need to complicate this by trying to enforce the
AER/DPC dependencies in the OS.  The firmware spec already requires
platforms to either retain ownership of both AER and DPC, or grant
ownership of both to the OS.

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 14a4b89a3b83..423dadd6727e 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -262,7 +262,7 @@ static int get_port_device_capability(struct pci_dev *dev)
 	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
 	    pci_aer_available() &&
-	    (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
+	    (pcie_ports_dpc_native || host->native_dpc))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||