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

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 1/8/2024 11:46 AM, Matthew W Carlis wrote:
> Hello again, sorry for the delayed response.. I have been on PTO. The above patch
> doesn't fix the problem in our systems as host->native_dpc is not set due to
> not using or having support for Error Disconnect Recovery (EDR). I wonder if

The only condition we check before requesting DPC control is whether OS
supports both EDR and DPC. As long as you have enabled related configs
(IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR)), this will be true.
Enabling these configs does not mean you are using it. Any reason why you don't
enable it in your case?

> host->native_dpc is a misleading name a way... Misleading in the sense that
> setting host->native_aer implies firmware intends the OS to control AER, whereas
> host->native_dpc being set appears to have an additional requirement on the
> use/support of EDR in addition to DPC. When I was working on the patch as
> submitted I had been thinking about all of these fields & my thinking was
> as follows.. The kernel requires host->native_aer in order to control AER, but
> it could control DPC whether host->native_dpc is set or unset. Therefore, if
> the kernel will control AER it should also control DPC on any capable devices.
> Of course there is also the requirement of having built with CONFIG_PCIE_AER
> & CONFIG_PCIE_DPC. Please advise if my understanding of all this is incorrect..
> 
> Thanks,
> -Matt
> 
> I included an update to the patch submitted in chain which should remove the
> build error that occured when CONFIG_PCIE_AER was not set. Including it
> in case my understanding of EDR/DPC/etc is correct.
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 14a4b89a3b83..2fc006f12988 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -257,12 +257,18 @@ 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 || host->native_aer))
>                 services |= PCIE_PORT_SERVICE_DPC;
> 
>         if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer