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

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

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