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

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

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