[+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@xxxxxxxxxxxxxxx>
> ---
> 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 ||
