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