RE: [External] Re: [PATCH v1] PCI/DPC: Skip EDR init when BIOS disable OS native DPC

[Xiaochun XC17 Li]

Hi, 
> -----Original Message-----
> From: Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> Sent: Wednesday, July 27, 2022 10:24 PM
> To: Xiaochun Lee <lixiaochun.2888@xxxxxxx>; linux-pci@xxxxxxxxxxxxxxx
> Cc: bhelgaas@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Xiaochun XC17 Li
> <lixc17@xxxxxxxxxx>
> Subject: [External] Re: [PATCH v1] PCI/DPC: Skip EDR init when BIOS disable
> OS native DPC
> 
> Hi,
> 
> On 7/27/22 4:05 AM, Xiaochun Lee wrote:
> > From: Xiaochun Lee <lixc17@xxxxxxxxxx>
> >
> > ACPI BIOS may disable OS native AER and DPC support to notify OS that
> > our platform doesn't support AER and DPC via the _OSC method.
> > BIOS also might leave the containment be accomplished purely in HW.
> > When firmware is set to non-aware OS DPC, we skip to install EDR
> > handler to an ACPI device.
> 
> No, EDR is used when firmware controls the DPC.
> 
> When the Firmware owns Downstream Port Containment, it is expected to
> use the new “Error Disconnect Recover” notification to alert OSPM of a
> Downstream Port Containment event.

Thank you for correcting me on that. Could you please share more information
about the below questions? Many thanks!
As you mentioned, when Firmware is set to the platform not to support
OS native DPC,  should OS still have to handle DPC flow from an EDR event?
In my systems, when I disable native DPC in UEFI BIOS, kernel messages
show the "platform does not support [SHPCHotplug AER DPC]" as follows,
and it says OS now controls capabilities that do not include AER DPC.

[    2.400996] acpi PNP0A08:04: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
[    2.402227] acpi PNP0A08:04: _OSC: platform does not support [SHPCHotplug AER DPC]
[    2.402520] acpi PNP0A08:04: _OSC: OS now controls [PCIeHotplug PME PCIeCapability LTR]
[    2.402521] acpi PNP0A08:04: FADT indicates ASPM is unsupported, using BIOS configuration

After I injected a PCIE CTO UCE DER event received and DPC started
running as you said, But there is a little bit of confusion as to why I
disable OS native DCP, it still be triggered. 
The injection message listed as below.

[  832.834785] pcieport 0000:a7:01.0: EDR: EDR event received
[  832.835232] pcieport 0000:a7:01.0: DPC: containment event, status:0x1f09 source:0x0000
[  832.835239] pcieport 0000:a7:01.0: DPC: unmasked uncorrectable error detected
[  832.835246] pcieport 0000:a7:01.0: PCIe Bus Error: severity=Uncorrected (Non-Fatal), type=Transaction Layer, (Requester ID)
[  832.835253] pcieport 0000:a7:01.0:   device [8086:352a] error status/mask=00004000/00180020
[  832.835258] pcieport 0000:a7:01.0:    [14] CmpltTO                (First)
[  903.394837] pcieport 0000:a7:01.0: AER: device recovery successful

On the contrary, if we keep OS native AER DPC enabled on BIOS,
we can see the message as below, OS now controls AER DPC. 
Under these settings, who should  handle DPC if an error is coming?
Is it the EDR event or the DPC interrupt (dpc_irq)? 
Does the BIOS participate in the DPC process in this situation? If BIOS
do not notify OS EDR via send WHEASCI, do we need to  install edr notifier
handler in function pci_acpi_add_edr_notifier? 
How about we skip EDR init when OS native AER/DPC enabled? Because we
now trigger DPC that be notified by an interrupt of DPC Control (DPCCTL)
register, install EDR handler seems redundant on OS native AER/DPC enabled.
Thanks!
[    2.350709] acpi PNP0A08:04: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3]
[    2.351799] acpi PNP0A08:04: _OSC: platform does not support [SHPCHotplug]
[    2.353144] acpi PNP0A08:04: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability LTR DPC]
[    2.353145] acpi PNP0A08:04: FADT indicates ASPM is unsupported, using BIOS configuration

> 
> >
> > Signed-off-by: Xiaochun Lee <lixc17@xxxxxxxxxx>
> > ---
> >  drivers/pci/pcie/edr.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index
> > a6b9b47..97a680b 100644
> > --- a/drivers/pci/pcie/edr.c
> > +++ b/drivers/pci/pcie/edr.c
> > @@ -19,6 +19,17 @@
> >  #define EDR_OST_SUCCESS			0x80
> >  #define EDR_OST_FAILED			0x81
> >
> > +static int pcie_dpc_is_native(struct pci_dev *dev) {
> > +	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> > +
> > +	if (!dev->dpc_cap)
> > +		return 0;
> > +
> > +	return pcie_ports_dpc_native || host->native_dpc; }
> > +
> > +
> >  /*
> >   * _DSM wrapper function to enable/disable DPC
> >   * @pdev   : PCI device structure
> > @@ -212,6 +223,11 @@ void pci_acpi_add_edr_notifier(struct pci_dev
> *pdev)
> >  		return;
> >  	}
> >
> > +	if (!pcie_dpc_is_native(pdev) && !pcie_aer_is_native(pdev)) {
> > +		pci_dbg(pdev, "OS doesn't control DPC, skipping EDR init\n");
> > +		return;
> > +	}
> > +
> >  	status = acpi_install_notify_handler(adev->handle,
> ACPI_SYSTEM_NOTIFY,
> >  					     edr_handle_event, pdev);
> >  	if (ACPI_FAILURE(status)) {
> 
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer