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

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

[Matthew W Carlis]

During our kernel version migration we encountered an issue preventing DPC
from being used by the kernel on the downstream ports of PCIe switches.
After some review of the patch history & code we found that the kernel
now required pcie_ports=dpc-native command line argument for such devices.
This came after we picked up:
    "PCI/portdrv: Allow AER service only for Root Ports & RCECs"

Belief is that the above patch is 'more correct' on the AER side, but perhaps
'less correct' on DPC. I initially reached out in the patch's email
thread to see whether others had thoughts & whether this change might be
desireable after which I was asked if I would submit a patch. Took a little
time to read some of the ACPI/PCIe specifications which are the basis of my
beliefs about the correctness of this patch. That being, if a user builds the
kernel with DPC/AER & the kernel receives OS control over AER, then it should
automatically use AER & DPC where applicable in the PCIe hierarchy.

Matthew W Carlis (1):
  PCI/portdrv: Allow DPC if the OS controls AER natively.

 drivers/pci/pcie/portdrv.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)


base-commit: ceb6a6f023fd3e8b07761ed900352ef574010bcb
-- 
2.17.1

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

[Matthew W Carlis]

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;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
-- 
2.17.1

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

[kernel test robot]

Hi Matthew,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.7-rc7 next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthew-W-Carlis/PCI-portdrv-Allow-DPC-if-the-OS-controls-AER-natively/20231225-154046
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20231223212235.34293-2-mattc%40purestorage.com
patch subject: [PATCH 1/1] PCI/portdrv: Allow DPC if the OS controls AER natively.
config: arc-randconfig-002-20231225 (https://download.01.org/0day-ci/archive/20231226/202312260105.Gu2Z5jdw-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/202312260105.Gu2Z5jdw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312260105.Gu2Z5jdw-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/pci/pcie/portdrv.c: In function 'get_port_device_capability':
>> drivers/pci/pcie/portdrv.c:272:19: error: 'struct pci_dev' has no member named 'aer_cap'; did you mean 'ats_cap'?
     272 |             (dev->aer_cap && host->native_aer)))
         |                   ^~~~~~~
         |                   ats_cap


vim +272 drivers/pci/pcie/portdrv.c

   244	
   245		/* Root Ports and Root Complex Event Collectors may generate PMEs */
   246		if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
   247		     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
   248		    (pcie_ports_native || host->native_pme)) {
   249			services |= PCIE_PORT_SERVICE_PME;
   250	
   251			/*
   252			 * Disable PME interrupt on this port in case it's been enabled
   253			 * by the BIOS (the PME service driver will enable it when
   254			 * necessary).
   255			 */
   256			pcie_pme_interrupt_enable(dev, false);
   257		}
   258	
   259		/*
   260		 * _OSC AER Control is required by the OS & requires OS to control AER,
   261		 * but _OSC DPC Control isn't required by the OS to control DPC; however
   262		 * it does require the OS to control DPC. _OSC DPC Control also requres
   263		 * _OSC EDR Control (Error Disconnect Recovery) (PCI Firmware - DPC ECN rev3.2)
   264		 * PCI_Express_Base 6.1, 6.2.11 Determination of DPC Control recommends
   265		 * platform fw or OS always link control of DPC to AER.
   266		 *
   267		 * With dpc-native, allow Linux to use DPC even if it doesn't have
   268		 * permission to use AER.
   269		 */
   270		if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
   271		    pci_aer_available() && (pcie_ports_dpc_native ||
 > 272		    (dev->aer_cap && host->native_aer)))
   273			services |= PCIE_PORT_SERVICE_DPC;
   274	
   275		if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
   276		    pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
   277			u32 linkcap;
   278	
   279			pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap);
   280			if (linkcap & PCI_EXP_LNKCAP_LBNC)
   281				services |= PCIE_PORT_SERVICE_BWNOTIF;
   282		}
   283	
   284		return services;
   285	}
   286	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Matthew,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.7-rc7 next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Matthew-W-Carlis/PCI-portdrv-Allow-DPC-if-the-OS-controls-AER-natively/20231225-154046
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20231223212235.34293-2-mattc%40purestorage.com
patch subject: [PATCH 1/1] PCI/portdrv: Allow DPC if the OS controls AER natively.
config: arm-defconfig (https://download.01.org/0day-ci/archive/20231226/202312260431.ZXppQZ1I-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/202312260431.ZXppQZ1I-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312260431.ZXppQZ1I-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pci/pcie/portdrv.c:272:12: error: no member named 'aer_cap' in 'struct pci_dev'; did you mean 'acs_cap'?
               (dev->aer_cap && host->native_aer)))
                     ^~~~~~~
                     acs_cap
   include/linux/pci.h:519:7: note: 'acs_cap' declared here
           u16             acs_cap;        /* ACS Capability offset */
                           ^
   1 error generated.


vim +272 drivers/pci/pcie/portdrv.c

   244	
   245		/* Root Ports and Root Complex Event Collectors may generate PMEs */
   246		if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
   247		     pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
   248		    (pcie_ports_native || host->native_pme)) {
   249			services |= PCIE_PORT_SERVICE_PME;
   250	
   251			/*
   252			 * Disable PME interrupt on this port in case it's been enabled
   253			 * by the BIOS (the PME service driver will enable it when
   254			 * necessary).
   255			 */
   256			pcie_pme_interrupt_enable(dev, false);
   257		}
   258	
   259		/*
   260		 * _OSC AER Control is required by the OS & requires OS to control AER,
   261		 * but _OSC DPC Control isn't required by the OS to control DPC; however
   262		 * it does require the OS to control DPC. _OSC DPC Control also requres
   263		 * _OSC EDR Control (Error Disconnect Recovery) (PCI Firmware - DPC ECN rev3.2)
   264		 * PCI_Express_Base 6.1, 6.2.11 Determination of DPC Control recommends
   265		 * platform fw or OS always link control of DPC to AER.
   266		 *
   267		 * With dpc-native, allow Linux to use DPC even if it doesn't have
   268		 * permission to use AER.
   269		 */
   270		if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
   271		    pci_aer_available() && (pcie_ports_dpc_native ||
 > 272		    (dev->aer_cap && host->native_aer)))
   273			services |= PCIE_PORT_SERVICE_DPC;
   274	
   275		if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
   276		    pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
   277			u32 linkcap;
   278	
   279			pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap);
   280			if (linkcap & PCI_EXP_LNKCAP_LBNC)
   281				services |= PCIE_PORT_SERVICE_BWNOTIF;
   282		}
   283	
   284		return services;
   285	}
   286	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Matthew W Carlis]

I guess we don't build aer_cap into pci_dev without CONFIG_PCIEAER...
Wonder if I should just drop the check for aer_cap since its likely
not necessary anyways.

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

[Bjorn Helgaas]

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

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

[Kuppuswamy Sathyanarayanan]

On 12/28/2023 1:23 PM, Bjorn Helgaas wrote:
> [+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.

Change looks fine to me. Once concern is, what if we are dealing with
a buggy firmware which give DPC native control, but retains AER? Do
you think it makes sense to have a sanity check to make sure this
does not happen?

> 
> 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 ||

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Matthew W Carlis]

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
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 ||

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

[Kuppuswamy Sathyanarayanan]

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

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

[Matthew W Carlis]

A small part is probably historical; we've been using DPC on PCIe switches
since before there was any EDR support in the kernel. It looks like there
was a PCIe DPC ECN as early as Feb 2012, but this EDR/DPC fw ECN didn't come in
till Jan 2019 & kernel support for ECN was even later. Its not immediately
clear I would want to use EDR in my newer architecures & then there are also
the older architecures still requiring support. When I submitted this patch I
came at it with the approach of trying to keep the old behavior & still support
the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to change
the behavior for both root ports & switch ports, requiring them to set
_OSC Control Field bit 7 (DPC) and _OSC Support Field bit 7 (EDR) or a kernel
command line value. I think no matter what, we want to ensure that PCIe Root
Ports and PCIe switches arrive at the same policy here.

Should we consider CONFIG_PCIEAER or CONFIG_PCIEDPC as any amount of directive
for the OS to use AER/DPC? In addition we have kernel command line arguments
for pcieports=(compat/native/dpc-native) and pci=noaer. There are perhaps some
others I'm not aware of. Then, there are the PCIe capabilities of the devies
& bios settings for AER FW/OS controls, etc. I'm not sure if it strikes me as the
right thing to now require users to specify additional fields to use DPC when
they had been using it happily before.

Perhaps the condition should be:
>         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 && !IS_ENABLED(CONFIG_PCIE_EDR))))

i.e: Use DPC if we set the command line argument or use DPC if we are are using
EDR's _OSC DPC field, or use DPC if we have AER & there isn't EDR support?

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

[Kuppuswamy Sathyanarayanan]

On 1/8/2024 4:15 PM, Matthew W Carlis wrote:
> A small part is probably historical; we've been using DPC on PCIe switches
> since before there was any EDR support in the kernel. It looks like there
> was a PCIe DPC ECN as early as Feb 2012, but this EDR/DPC fw ECN didn't come in
> till Jan 2019 & kernel support for ECN was even later. Its not immediately
> clear I would want to use EDR in my newer architecures & then there are also
> the older architecures still requiring support. When I submitted this patch I
> came at it with the approach of trying to keep the old behavior & still support
> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to change

Just advertising the support for EDR in OS should not change any functional
behavior. EDR will be used only if your firmware take DPC control and sends
EDR notification. Since your kernel has EDR source support, why not enable
the relevant config? or did I not understand the issue correctly?

> the behavior for both root ports & switch ports, requiring them to set
> _OSC Control Field bit 7 (DPC) and _OSC Support Field bit 7 (EDR) or a kernel
> command line value. I think no matter what, we want to ensure that PCIe Root
> Ports and PCIe switches arrive at the same policy here.
> 
> Should we consider CONFIG_PCIEAER or CONFIG_PCIEDPC as any amount of directive
> for the OS to use AER/DPC? In addition we have kernel command line arguments

No, I don't think we should use CONFIG options in actual support check.

> for pcieports=(compat/native/dpc-native) and pci=noaer. There are perhaps some
> others I'm not aware of. Then, there are the PCIe capabilities of the devies
> & bios settings for AER FW/OS controls, etc. I'm not sure if it strikes me as the
> right thing to now require users to specify additional fields to use DPC when
> they had been using it happily before.
> 
> Perhaps the condition should be:
>>         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 && !IS_ENABLED(CONFIG_PCIE_EDR))))
> 
> i.e: Use DPC if we set the command line argument or use DPC if we are are using
> EDR's _OSC DPC field, or use DPC if we have AER & there isn't EDR support?
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Kuppuswamy Sathyanarayanan]

On 1/10/2024 8:41 AM, Kuppuswamy Sathyanarayanan wrote:
> 
> 
> On 1/8/2024 4:15 PM, Matthew W Carlis wrote:
>> A small part is probably historical; we've been using DPC on PCIe switches
>> since before there was any EDR support in the kernel. It looks like there
>> was a PCIe DPC ECN as early as Feb 2012, but this EDR/DPC fw ECN didn't come in
>> till Jan 2019 & kernel support for ECN was even later. Its not immediately
>> clear I would want to use EDR in my newer architecures & then there are also
>> the older architecures still requiring support. When I submitted this patch I
>> came at it with the approach of trying to keep the old behavior & still support
>> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to change
> 
> Just advertising the support for EDR in OS should not change any functional
> behavior. EDR will be used only if your firmware take DPC control and sends
> EDR notification. Since your kernel has EDR source support, why not enable
> the relevant config? or did I not understand the issue correctly?
> 

Hi Bjorn,

Since requesting DPC control needs both EDR and DPC support in kernel, I am
wondering whether we need two different configs for it. IMO, it makes sense to
merge them under one config. I don't see EDR working without DPC support. Since
DPC control also need support for EDR, I think we don't need to differentiate
them. What you think?

file: drivers/acpi/pci_root.c
+       /*
+        * Per the Downstream Port Containment Related Enhancements ECN to
+        * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
+        * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
+        * and EDR.
+        */
+       if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
+               control |= OSC_PCI_EXPRESS_DPC_CONTROL;


>> the behavior for both root ports & switch ports, requiring them to set
>> _OSC Control Field bit 7 (DPC) and _OSC Support Field bit 7 (EDR) or a kernel
>> command line value. I think no matter what, we want to ensure that PCIe Root
>> Ports and PCIe switches arrive at the same policy here.
>>
>> Should we consider CONFIG_PCIEAER or CONFIG_PCIEDPC as any amount of directive
>> for the OS to use AER/DPC? In addition we have kernel command line arguments
> 
> No, I don't think we should use CONFIG options in actual support check.
> 
>> for pcieports=(compat/native/dpc-native) and pci=noaer. There are perhaps some
>> others I'm not aware of. Then, there are the PCIe capabilities of the devies
>> & bios settings for AER FW/OS controls, etc. I'm not sure if it strikes me as the
>> right thing to now require users to specify additional fields to use DPC when
>> they had been using it happily before.
>>
>> Perhaps the condition should be:
>>>         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 && !IS_ENABLED(CONFIG_PCIE_EDR))))
>>
>> i.e: Use DPC if we set the command line argument or use DPC if we are are using
>> EDR's _OSC DPC field, or use DPC if we have AER & there isn't EDR support?
>>
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Matthew W Carlis]

On 1/10/2024 8:41 AM, Kuppuswamy Sathyanarayanan wrote:
> Since your kernel has EDR source support, why not enable
> the relevant config? or did I not understand the issue correctly?

As I had said we never intend to use EDR or firmware controlled DPC. Why should
I have to build in more code that I wouldn't use? I'm not sure that we're
both using the same lense to consider how & whether _OSC DPC Control supports
the old world of DPC. i.e OS never required firmware's permission to control
DPC & it was not tied to EDR. I wish I had been working in this area at the
time they were drafting the 2019 FW ECN then I might have known to try & influence
its details.

I think the first & primary problem for the kernel here is not treating root
ports & switch ports the same when deciding to use DPC. I would like to hear
from Bjorn first, but I guess I could be open to just relying on the cli
pcie_ports=dpc-native field if there is reason for the kernel to only support
the latest additions of the PCIe FW DPC ECN by default. In this case the patch
would look as the one in Bjorns response on Dec 28 2023 at 1:23PM, included
again below. We may find others who are using Root Port DPC now needing to
change their config to support EDR or use the cli pcie_ports=dpc-native as
a result however.

If this is the route that makes the most sense, I can update my patch & resubmit
as below if that is ok. Perhaps there might also be some docs/notes to be created
around all of this.

-Matt

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))

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

[Matthew W Carlis]

On 1/10/2024 8:41 AM, Kuppuswamy Sathyanarayanan wrote:
> Since your kernel has EDR source support, why not enable
> the relevant config? or did I not understand the issue correctly?

As I had said we never intend to use EDR or firmware controlled DPC. Why should
I have to build in more code that I wouldn't use? I'm not sure that we're
both using the same lense to consider how & whether _OSC DPC Control supports
the old world of DPC. i.e OS never required firmware's permission to control
DPC & it was not tied to EDR. I wish I had been working in this area at the
time they were drafting the 2019 FW ECN then I might have known to try & influence
its details.

I think the first & primary problem for the kernel here is not treating root
ports & switch ports the same when deciding to use DPC. I would like to hear
from Bjorn first, but I guess I could be open to just relying on the cli
pcie_ports=dpc-native field if there is reason for the kernel to only support
the latest additions of the PCIe FW DPC ECN by default. In this case the patch
would look as the one in Bjorns response on Dec 28 2023 at 1:23PM, included
again below. We may find others who are using Root Port DPC now needing to
change their config to support EDR or use the cli pcie_ports=dpc-native as
a result however.

If this is the route that makes the most sense, I can update my patch & resubmit
as below if that is ok. Perhaps there might also be some docs/notes to be created
around all of this.

-Matt

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))

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

[Bjorn Helgaas]

On Mon, Jan 08, 2024 at 05:15:08PM -0700, Matthew W Carlis wrote:
> A small part is probably historical; we've been using DPC on PCIe
> switches since before there was any EDR support in the kernel. It
> looks like there was a PCIe DPC ECN as early as Feb 2012, but this
> EDR/DPC fw ECN didn't come in till Jan 2019 & kernel support for ECN
> was even later. Its not immediately clear I would want to use EDR in
> my newer architecures & then there are also the older architecures
> still requiring support. When I submitted this patch I came at it
> with the approach of trying to keep the old behavior & still support
> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to
> change the behavior for both root ports & switch ports, requiring
> them to set _OSC Control Field bit 7 (DPC) and _OSC Support Field
> bit 7 (EDR) or a kernel command line value. I think no matter what,
> we want to ensure that PCIe Root Ports and PCIe switches arrive at
> the same policy here.

Is there an approved DPC ECN to the PCI Firmware spec that adds DPC
control negotiation, but does *not* add the EDR requirement?

I'm looking at
https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/12888, which
seems to be the final "Downstream Port Containment Related
Enhancements" ECN, which is dated 1/28/2019 and applies to the PCI
Firmware spec r3.2.

It adds bit 7, "PCI Express Downstream Port Containment Configuration
control", to the passed-in _OSC Control field, which indicates that
the OS supports both "native OS control and firmware ownership models
(i.e. Error Disconnect Recover notification) of Downstream Port
Containment."

It also adds the dependency that "If the OS sets bit 7 of the Control
field, it must set bit 7 of the Support field, indicating support for
the Error Disconnect Recover event."

So I'm trying to figure out if the "support DPC but not EDR" situation
was ever a valid place to be.  Maybe it's a mistake to have separate
CONFIG_PCIE_DPC and CONFIG_PCIE_EDR options.

CONFIG_PCIE_EDR depends on CONFIG_ACPI, so the situation is a little
bit murky on non-ACPI systems that support DPC.

Bjorn

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

[Kuppuswamy Sathyanarayanan]

On 1/22/24 11:32 AM, Bjorn Helgaas wrote:
> On Mon, Jan 08, 2024 at 05:15:08PM -0700, Matthew W Carlis wrote:
>> A small part is probably historical; we've been using DPC on PCIe
>> switches since before there was any EDR support in the kernel. It
>> looks like there was a PCIe DPC ECN as early as Feb 2012, but this
>> EDR/DPC fw ECN didn't come in till Jan 2019 & kernel support for ECN
>> was even later. Its not immediately clear I would want to use EDR in
>> my newer architecures & then there are also the older architecures
>> still requiring support. When I submitted this patch I came at it
>> with the approach of trying to keep the old behavior & still support
>> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to
>> change the behavior for both root ports & switch ports, requiring
>> them to set _OSC Control Field bit 7 (DPC) and _OSC Support Field
>> bit 7 (EDR) or a kernel command line value. I think no matter what,
>> we want to ensure that PCIe Root Ports and PCIe switches arrive at
>> the same policy here.
> Is there an approved DPC ECN to the PCI Firmware spec that adds DPC
> control negotiation, but does *not* add the EDR requirement?
>
> I'm looking at
> https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/12888, which
> seems to be the final "Downstream Port Containment Related
> Enhancements" ECN, which is dated 1/28/2019 and applies to the PCI
> Firmware spec r3.2.
>
> It adds bit 7, "PCI Express Downstream Port Containment Configuration
> control", to the passed-in _OSC Control field, which indicates that
> the OS supports both "native OS control and firmware ownership models
> (i.e. Error Disconnect Recover notification) of Downstream Port
> Containment."
>
> It also adds the dependency that "If the OS sets bit 7 of the Control
> field, it must set bit 7 of the Support field, indicating support for
> the Error Disconnect Recover event."
>
> So I'm trying to figure out if the "support DPC but not EDR" situation
> was ever a valid place to be.  Maybe it's a mistake to have separate
> CONFIG_PCIE_DPC and CONFIG_PCIE_EDR options.

My understanding is also similar. I have raised the same point in
https://lore.kernel.org/all/3c02a6d6-917e-486c-ad41-bdf176639ff2@linux.intel.com/

IMO, we don't need a separate config for EDR. I don't think user can gain anything
with disabling EDR and enabling DPC. As long as firmware does not user EDR
support, just compiling the code should be harmless.

So we can either remove it, or select it by default if user selects DPC config.

>
> CONFIG_PCIE_EDR depends on CONFIG_ACPI, so the situation is a little
> bit murky on non-ACPI systems that support DPC.

If we are going to remove the EDR config, it might need #ifdef CONFIG_ACPI changes
in edr.c to not compile ACPI specific code. Alternative choice is to compile edr.c
with CONFIG_ACPI.

>
> Bjorn
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Bjorn Helgaas]

On Mon, Jan 22, 2024 at 06:37:48PM -0800, Kuppuswamy Sathyanarayanan wrote:
> 
> On 1/22/24 11:32 AM, Bjorn Helgaas wrote:
> > On Mon, Jan 08, 2024 at 05:15:08PM -0700, Matthew W Carlis wrote:
> >> A small part is probably historical; we've been using DPC on PCIe
> >> switches since before there was any EDR support in the kernel. It
> >> looks like there was a PCIe DPC ECN as early as Feb 2012, but this
> >> EDR/DPC fw ECN didn't come in till Jan 2019 & kernel support for ECN
> >> was even later. Its not immediately clear I would want to use EDR in
> >> my newer architecures & then there are also the older architecures
> >> still requiring support. When I submitted this patch I came at it
> >> with the approach of trying to keep the old behavior & still support
> >> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to
> >> change the behavior for both root ports & switch ports, requiring
> >> them to set _OSC Control Field bit 7 (DPC) and _OSC Support Field
> >> bit 7 (EDR) or a kernel command line value. I think no matter what,
> >> we want to ensure that PCIe Root Ports and PCIe switches arrive at
> >> the same policy here.
> > Is there an approved DPC ECN to the PCI Firmware spec that adds DPC
> > control negotiation, but does *not* add the EDR requirement?
> >
> > I'm looking at
> > https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/12888, which
> > seems to be the final "Downstream Port Containment Related
> > Enhancements" ECN, which is dated 1/28/2019 and applies to the PCI
> > Firmware spec r3.2.
> >
> > It adds bit 7, "PCI Express Downstream Port Containment Configuration
> > control", to the passed-in _OSC Control field, which indicates that
> > the OS supports both "native OS control and firmware ownership models
> > (i.e. Error Disconnect Recover notification) of Downstream Port
> > Containment."
> >
> > It also adds the dependency that "If the OS sets bit 7 of the Control
> > field, it must set bit 7 of the Support field, indicating support for
> > the Error Disconnect Recover event."
> >
> > So I'm trying to figure out if the "support DPC but not EDR" situation
> > was ever a valid place to be.  Maybe it's a mistake to have separate
> > CONFIG_PCIE_DPC and CONFIG_PCIE_EDR options.
> 
> My understanding is also similar. I have raised the same point in
> https://lore.kernel.org/all/3c02a6d6-917e-486c-ad41-bdf176639ff2@linux.intel.com/

Ah, sorry, I missed that.

> IMO, we don't need a separate config for EDR. I don't think user can
> gain anything with disabling EDR and enabling DPC. As long as
> firmware does not user EDR support, just compiling the code should
> be harmless.
> 
> So we can either remove it, or select it by default if user selects
> DPC config.
> 
> > CONFIG_PCIE_EDR depends on CONFIG_ACPI, so the situation is a little
> > bit murky on non-ACPI systems that support DPC.
> 
> If we are going to remove the EDR config, it might need #ifdef
> CONFIG_ACPI changes in edr.c to not compile ACPI specific code.
> Alternative choice is to compile edr.c with CONFIG_ACPI.

Right.  I think we should probably remove CONFIG_PCIE_EDR completely
and make everything controlled by CONFIG_PCIE_DPC.

edr.c only provides two interfaces: pci_acpi_add_edr_notifier() and
pci_acpi_remove_edr_notifier(), which are only used by pci-acpi.c,
which is only compiled if CONFIG_ACPI, so we could probably also
compile edr.c only if CONFIG_ACPI.

And the declarations in include/linux/pci-acpi.h could probably be
moved to drivers/pci/pci.h since they're never used outside
drivers/pci/.

Bjorn

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

[Matthew W Carlis]

Hello again! I'm glad that I'm not the only person with a little confusion
about the FW ECN regarding DPC/EDR. I would argue that DPC wasn't tied to EDR
& shouldn't have been because DPC was added in PCI Base Spec Rev 3.1 in 2014,
but there wasn't an EDR ECN till ~2020. Anyway, that's the way it goes..

I don't want to burden the kernel with making some impossible boot time decision
here. Perhaps most of the machines in the world using DPC will soon use EDR/SFI
etc. My use cases are a bit out of the ordinary & the ACPI specifications don't
seem to have given us a mechanism for the kernel to conclude it can use DPC
without EDR support...

Shall I submit a patch removing CONFIG_PCIE_EDR? Perhaps the exercise would
inform me about whether its code should be in CONFIG_PCIE_DPC or CONFIG_ACPI.

Thanks,
-Matt

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

[Bjorn Helgaas]

On Tue, Jan 23, 2024 at 04:18:34PM -0700, Matthew W Carlis wrote:
> Hello again! I'm glad that I'm not the only person with a little
> confusion about the FW ECN regarding DPC/EDR. I would argue that DPC
> wasn't tied to EDR & shouldn't have been because DPC was added in
> PCI Base Spec Rev 3.1 in 2014, but there wasn't an EDR ECN till
> ~2020. Anyway, that's the way it goes..

It does involve several different specs (PCIe Base for the DPC
hardware feature, ACPI for the OS/firmware EDR interface, PCI Firmware
for the EDR support and DPC ownership negotiation), so maybe that
helps explain the muddle.

> I don't want to burden the kernel with making some impossible boot
> time decision here. Perhaps most of the machines in the world using
> DPC will soon use EDR/SFI etc. My use cases are a bit out of the
> ordinary & the ACPI specifications don't seem to have given us a
> mechanism for the kernel to conclude it can use DPC without EDR
> support...

I don't know anything about SFI and I don't see any required
connection between DPC/EDR/SFI in the PCI Firmware spec.

The PCI Firmware spec requires ACPI OSes to support EDR if they want
to use DPC.  But I don't know that the firmware is required to
actually implement the EDR functionality.

Non-ACPI OSes are presumed to own DPC and all other PCIe hardware
features, and I don't think EDR would be in the picture since it's an
ACPI thing.

> Shall I submit a patch removing CONFIG_PCIE_EDR? Perhaps the
> exercise would inform me about whether its code should be in
> CONFIG_PCIE_DPC or CONFIG_ACPI.

That would be great!  I would say CONFIG_PCIE_EDR should go away and
edr.c should only be compiled if CONFIG_PCIE_DPC=y and CONFIG_ACPI=y.

Bjorn

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

[Bjorn Helgaas]

On Tue, Jan 23, 2024 at 09:59:21AM -0600, Bjorn Helgaas wrote:
> On Mon, Jan 22, 2024 at 06:37:48PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > On 1/22/24 11:32 AM, Bjorn Helgaas wrote:
> > > On Mon, Jan 08, 2024 at 05:15:08PM -0700, Matthew W Carlis wrote:
> > >> A small part is probably historical; we've been using DPC on PCIe
> > >> switches since before there was any EDR support in the kernel. It
> > >> looks like there was a PCIe DPC ECN as early as Feb 2012, but this
> > >> EDR/DPC fw ECN didn't come in till Jan 2019 & kernel support for ECN
> > >> was even later. Its not immediately clear I would want to use EDR in
> > >> my newer architecures & then there are also the older architecures
> > >> still requiring support. When I submitted this patch I came at it
> > >> with the approach of trying to keep the old behavior & still support
> > >> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to
> > >> change the behavior for both root ports & switch ports, requiring
> > >> them to set _OSC Control Field bit 7 (DPC) and _OSC Support Field
> > >> bit 7 (EDR) or a kernel command line value. I think no matter what,
> > >> we want to ensure that PCIe Root Ports and PCIe switches arrive at
> > >> the same policy here.
> > > Is there an approved DPC ECN to the PCI Firmware spec that adds DPC
> > > control negotiation, but does *not* add the EDR requirement?
> > >
> > > I'm looking at
> > > https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/12888, which
> > > seems to be the final "Downstream Port Containment Related
> > > Enhancements" ECN, which is dated 1/28/2019 and applies to the PCI
> > > Firmware spec r3.2.
> > >
> > > It adds bit 7, "PCI Express Downstream Port Containment Configuration
> > > control", to the passed-in _OSC Control field, which indicates that
> > > the OS supports both "native OS control and firmware ownership models
> > > (i.e. Error Disconnect Recover notification) of Downstream Port
> > > Containment."
> > >
> > > It also adds the dependency that "If the OS sets bit 7 of the Control
> > > field, it must set bit 7 of the Support field, indicating support for
> > > the Error Disconnect Recover event."
> > >
> > > So I'm trying to figure out if the "support DPC but not EDR" situation
> > > was ever a valid place to be.  Maybe it's a mistake to have separate
> > > CONFIG_PCIE_DPC and CONFIG_PCIE_EDR options.
> > 
> > My understanding is also similar. I have raised the same point in
> > https://lore.kernel.org/all/3c02a6d6-917e-486c-ad41-bdf176639ff2@linux.intel.com/
> 
> Ah, sorry, I missed that.
> 
> > IMO, we don't need a separate config for EDR. I don't think user can
> > gain anything with disabling EDR and enabling DPC. As long as
> > firmware does not user EDR support, just compiling the code should
> > be harmless.
> > 
> > So we can either remove it, or select it by default if user selects
> > DPC config.
> > 
> > > CONFIG_PCIE_EDR depends on CONFIG_ACPI, so the situation is a little
> > > bit murky on non-ACPI systems that support DPC.
> > 
> > If we are going to remove the EDR config, it might need #ifdef
> > CONFIG_ACPI changes in edr.c to not compile ACPI specific code.
> > Alternative choice is to compile edr.c with CONFIG_ACPI.
> 
> Right.  I think we should probably remove CONFIG_PCIE_EDR completely
> and make everything controlled by CONFIG_PCIE_DPC.

In the PCI Firmware spec, r3.3, sec 4.5.1, table 4-4, the description
of "Error Disconnect Recover Supported" hints at the possibility for
an OS to support EDR but not DPC:

  In the context of PCIe, support for Error Disconnect Recover implies
  that the operating system will invalidate the software state
  associated with child devices of the port without attempting to
  access the child device hardware. *If* the operating system supports
  Downstream Port Containment (DPC), ..., the operating system shall
  attempt to recover the child devices if the port implements the
  Downstream Port Containment Extended Capability.

On the other hand, sec 4.6.12 and the implementation note there with
the EDR flow seem to assume the OS *does* support DPC and can clear
error status and bring ports out of DPC even if the OS hasn't been
granted control of DPC.

EDR is an ACPI feature, and I guess it might be theoretically possible
to use EDR on an ACPI system with some non-DPC error containment
feature like powerpc EEH.  But obviously powerpc doesn't use ACPI, and
a hypothetical ACPI system with non-DPC error containment would have
to add support for that containment in edr_handle_event().

So while I'm not 100% sure that making CONFIG_PCIE_DPC control both
DPC and EDR is completely correct, I guess for now I still think using
CONFIG_PCIE_DPC for both DPC and EDR seems like a reasonable plan
because we have no support for EDR *without* DPC.

Bjorn

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

[Bjorn Helgaas]

[+cc Mahesh, Oliver, linuxppc-dev, since I mentioned powerpc below.
Probably not of interest since this is about the ACPI EDR feature, but
just FYI]

On Wed, Feb 21, 2024 at 05:11:04PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 23, 2024 at 09:59:21AM -0600, Bjorn Helgaas wrote:
> > On Mon, Jan 22, 2024 at 06:37:48PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > > On 1/22/24 11:32 AM, Bjorn Helgaas wrote:
> > > > On Mon, Jan 08, 2024 at 05:15:08PM -0700, Matthew W Carlis wrote:
> > > >> A small part is probably historical; we've been using DPC on PCIe
> > > >> switches since before there was any EDR support in the kernel. It
> > > >> looks like there was a PCIe DPC ECN as early as Feb 2012, but this
> > > >> EDR/DPC fw ECN didn't come in till Jan 2019 & kernel support for ECN
> > > >> was even later. Its not immediately clear I would want to use EDR in
> > > >> my newer architecures & then there are also the older architecures
> > > >> still requiring support. When I submitted this patch I came at it
> > > >> with the approach of trying to keep the old behavior & still support
> > > >> the newer EDR behavior. Bjorns patch from Dec 28 2023 would seem to
> > > >> change the behavior for both root ports & switch ports, requiring
> > > >> them to set _OSC Control Field bit 7 (DPC) and _OSC Support Field
> > > >> bit 7 (EDR) or a kernel command line value. I think no matter what,
> > > >> we want to ensure that PCIe Root Ports and PCIe switches arrive at
> > > >> the same policy here.
> > > > Is there an approved DPC ECN to the PCI Firmware spec that adds DPC
> > > > control negotiation, but does *not* add the EDR requirement?
> > > >
> > > > I'm looking at
> > > > https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/12888, which
> > > > seems to be the final "Downstream Port Containment Related
> > > > Enhancements" ECN, which is dated 1/28/2019 and applies to the PCI
> > > > Firmware spec r3.2.
> > > >
> > > > It adds bit 7, "PCI Express Downstream Port Containment Configuration
> > > > control", to the passed-in _OSC Control field, which indicates that
> > > > the OS supports both "native OS control and firmware ownership models
> > > > (i.e. Error Disconnect Recover notification) of Downstream Port
> > > > Containment."
> > > >
> > > > It also adds the dependency that "If the OS sets bit 7 of the Control
> > > > field, it must set bit 7 of the Support field, indicating support for
> > > > the Error Disconnect Recover event."
> > > >
> > > > So I'm trying to figure out if the "support DPC but not EDR" situation
> > > > was ever a valid place to be.  Maybe it's a mistake to have separate
> > > > CONFIG_PCIE_DPC and CONFIG_PCIE_EDR options.
> > > 
> > > My understanding is also similar. I have raised the same point in
> > > https://lore.kernel.org/all/3c02a6d6-917e-486c-ad41-bdf176639ff2@linux.intel.com/
> > 
> > Ah, sorry, I missed that.
> > 
> > > IMO, we don't need a separate config for EDR. I don't think user can
> > > gain anything with disabling EDR and enabling DPC. As long as
> > > firmware does not user EDR support, just compiling the code should
> > > be harmless.
> > > 
> > > So we can either remove it, or select it by default if user selects
> > > DPC config.
> > > 
> > > > CONFIG_PCIE_EDR depends on CONFIG_ACPI, so the situation is a little
> > > > bit murky on non-ACPI systems that support DPC.
> > > 
> > > If we are going to remove the EDR config, it might need #ifdef
> > > CONFIG_ACPI changes in edr.c to not compile ACPI specific code.
> > > Alternative choice is to compile edr.c with CONFIG_ACPI.
> > 
> > Right.  I think we should probably remove CONFIG_PCIE_EDR completely
> > and make everything controlled by CONFIG_PCIE_DPC.
> 
> In the PCI Firmware spec, r3.3, sec 4.5.1, table 4-4, the description
> of "Error Disconnect Recover Supported" hints at the possibility for
> an OS to support EDR but not DPC:
> 
>   In the context of PCIe, support for Error Disconnect Recover implies
>   that the operating system will invalidate the software state
>   associated with child devices of the port without attempting to
>   access the child device hardware. *If* the operating system supports
>   Downstream Port Containment (DPC), ..., the operating system shall
>   attempt to recover the child devices if the port implements the
>   Downstream Port Containment Extended Capability.
> 
> On the other hand, sec 4.6.12 and the implementation note there with
> the EDR flow seem to assume the OS *does* support DPC and can clear
> error status and bring ports out of DPC even if the OS hasn't been
> granted control of DPC.
> 
> EDR is an ACPI feature, and I guess it might be theoretically possible
> to use EDR on an ACPI system with some non-DPC error containment
> feature like powerpc EEH.  But obviously powerpc doesn't use ACPI, and
> a hypothetical ACPI system with non-DPC error containment would have
> to add support for that containment in edr_handle_event().
> 
> So while I'm not 100% sure that making CONFIG_PCIE_DPC control both
> DPC and EDR is completely correct, I guess for now I still think using
> CONFIG_PCIE_DPC for both DPC and EDR seems like a reasonable plan
> because we have no support for EDR *without* DPC.
> 
> Bjorn