diff mbox series

[v17,11/12] PCI/DPC: Add Error Disconnect Recover (EDR) support

Message ID 9ae1d3285beeb81bbf85571a89b8f3d4451eae8f.1583286655.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State Superseded, archived
Delegated to: Bjorn Helgaas
Headers show
Series Add Error Disconnect Recover (EDR) support | expand

Commit Message

Kuppuswamy Sathyanarayanan March 4, 2020, 2:36 a.m. UTC 
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

As per ACPI specification r6.3, sec 5.6.6, when firmware owns Downstream
Port Containment (DPC), its expected to use the "Error Disconnect
Recover" (EDR) notification to alert OSPM of a DPC event and if OS
supports EDR, its expected to handle the software state invalidation and
port recovery in OS, and also let firmware know the recovery status via
_OST ACPI call. Related _OST status codes can be found in ACPI
specification r6.3, sec 6.3.5.2.

Also, as per PCI firmware specification r3.2 Downstream Port Containment
Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by
firmware (firmware first mode), firmware is responsible for
configuring the DPC and OS is responsible for error recovery. Also, OS
is allowed to modify DPC registers only during the EDR notification
window.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci-acpi.c    |   3 +
 drivers/pci/pcie/Kconfig  |  10 ++
 drivers/pci/pcie/Makefile |   1 +
 drivers/pci/pcie/edr.c    | 249 ++++++++++++++++++++++++++++++++++++++
 include/linux/pci-acpi.h  |   8 ++
 5 files changed, 271 insertions(+)
 create mode 100644 drivers/pci/pcie/edr.c

Comments

Bjorn Helgaas March 6, 2020, 3:47 a.m. UTC | #1 
[+cc Olof for pcie_ports=dpc-native question]

On Tue, Mar 03, 2020 at 06:36:34PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> +void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +	acpi_status astatus;
> +
> +	if (!adev) {
> +		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Per the Downstream Port Containment Related Enhancements ECN to
> +	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
> +	 * can only be enabled if DPC is controlled by firmware.
> +	 *
> +	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
> +	 * determine ownership of DPC between firmware or OS.
> +	 * Per the Downstream Port Containment Related Enhancements
> +	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> +	 * OS can use bit 7 of _OSC control field to negotiate control
> +	 * over DPC Capability.
> +	 */
> +	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
> +		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
> +		return;
> +	}
> +
> +	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> +					      edr_handle_event, pdev);

I think this is still problematic.  You mentioned Alex's work [1,2].
We do need to revisit those patches, but I don't really want to defer
*this* question of the EDR notify handler.  Negotiating support of
AER/DPC/EDR is already complicated, and I don't want to complicate it
even more by merging something we already know is not quite right.

I don't understand your comment that "EDR can only be enabled if DPC
is controlled by firmware."  I don't see anything in table 4-6 to that
effect.  The only mention of EDR there is to say that the OS can
access the DPC capability in the EDR processing window, i.e., after
the OS receives the EDR notification and before it clears DPC Trigger
Status.

EDR is a general ACPI feature that is not PCI-specific.  For EDR on
PCI devices, OS support is advertised via _OSC *Support* (table 4-4),
which says:

  Error Disconnect Recover Supported

  The OS sets this bit to 1 if it supports Error Disconnect Recover
  notification on PCI Express Host Bridges, Root Ports and Switch
  Downstream Ports. Otherwise, the OS sets this bit to 0.

I think that means that if we set the "Error Disconnect Recover
Supported" _OSC bit (OSC_PCI_EDR_SUPPORT), we must install a handler
for EDR notifications.  We set OSC_PCI_EDR_SUPPORT whenever
CONFIG_PCIE_EDR=y, so I think we should install the notify handler
here unconditionally (since this file is compiled only when
CONFIG_PCIE_EDR=y).

I don't think we should even test pcie_ports_dpc_native here.  If we
told the platform we can handle EDR notifications, we should be
prepared to get them, regardless of whether the user booted with
"pcie_ports=dpc-native".

It's conceivable that pcie_ports_dpc_native should make us do
something different in the notify handler after we *get* a
notification, but I doubt we should even worry about that.

IIUC, pcie_ports_dpc_native exists because Linux DPC originally worked
even if the OS didn't have control of AER.  eed85ff4c0da7 ("PCI/DPC:
Enable DPC only if AER is available") meant that if Linux didn't have
control of AER, DPC no longer worked.  "pcie_ports=dpc-native" is
basically a way to get that previous behavior of Linux DPC regardless
of AER control.

I don't think that issue applies to EDR.  There's no concept of an OS
"enabling" or "being granted control of" EDR.  The OS merely
advertises that "yes, I'm prepared to handle EDR notifications".
AFAICT, the ECR says nothing about EDR support being conditional on OS
control of AER or DPC.  The notify *handler* might need to do
different things depending on whether we have AER or DPC control, but
the handler itself should be registered regardless.

[1] https://lore.kernel.org/linux-pci/20181115231605.24352-1-mr.nuke.me@gmail.com/
[2] https://lore.kernel.org/linux-pci/20190326172343.28946-1-mr.nuke.me@gmail.com/
Kuppuswamy Sathyanarayanan March 6, 2020, 6:32 a.m. UTC | #2 
Hi Bjorn,

On 3/5/2020 7:47 PM, Bjorn Helgaas wrote:
> [+cc Olof for pcie_ports=dpc-native question]
> 
> On Tue, Mar 03, 2020 at 06:36:34PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
>> +void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
>> +{
>> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> +	acpi_status astatus;
>> +
>> +	if (!adev) {
>> +		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Per the Downstream Port Containment Related Enhancements ECN to
>> +	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
>> +	 * can only be enabled if DPC is controlled by firmware.
>> +	 *
>> +	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
>> +	 * determine ownership of DPC between firmware or OS.
>> +	 * Per the Downstream Port Containment Related Enhancements
>> +	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
>> +	 * OS can use bit 7 of _OSC control field to negotiate control
>> +	 * over DPC Capability.
>> +	 */
>> +	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
>> +		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
>> +		return;
>> +	}
>> +
>> +	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>> +					      edr_handle_event, pdev);
> 
> I think this is still problematic.  You mentioned Alex's work [1,2].
> We do need to revisit those patches, but I don't really want to defer
> *this* question of the EDR notify handler.  Negotiating support of
> AER/DPC/EDR is already complicated, and I don't want to complicate it
> even more by merging something we already know is not quite right.
> 
> I don't understand your comment that "EDR can only be enabled if DPC
> is controlled by firmware."  I don't see anything in table 4-6 to that
> effect.  The only mention of EDR there is to say that the OS can
> access the DPC capability in the EDR processing window, i.e., after
> the OS receives the EDR notification and before it clears DPC Trigger
> Status.

Please check the following spec reference (from table 4-6).

     If control of this feature was requested and denied, firmware is
     responsible for initializing Downstream Port Containment Extended
     Capability Structures per firmware policy. Further, the OS is
     permitted to read or write DPC Control and Status registers of a
     port while processing an Error Disconnect Recover notification from
     firmware on that port.

It specifies firmware is expected to use EDR notification *only* when 
the control of DPC is requested and denied ( which means firmware owns 
the DPC). Although it does not explicitly state that we should install 
EDR notification handler only if firmware owns DPC, it mentions that EDR 
notification is only used if firmware owns DPC. So why should we install 
it if its not going to be used when OS owns DPC.

Also check the following reference from section 2 of EDR ECN. It also 
clarifies EDR feature is only used when firmware owns DPC.

     PCIe Base Specification suggests that Downstream Port Containment
     may be controlled either by the Firmware or the Operating System. It
     also suggests that the Firmware retain ownership of Downstream Port
     Containment if it also owns AER. 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*.


> 
> EDR is a general ACPI feature that is not PCI-specific.  For EDR on
> PCI devices, OS support is advertised via _OSC *Support* (table 4-4),
> which says:
> 
>    Error Disconnect Recover Supported
> 
>    The OS sets this bit to 1 if it supports Error Disconnect Recover
>    notification on PCI Express Host Bridges, Root Ports and Switch
>    Downstream Ports. Otherwise, the OS sets this bit to 0.
> 
> I think that means that if we set the "Error Disconnect Recover
> Supported" _OSC bit (OSC_PCI_EDR_SUPPORT), we must install a handler
> for EDR notifications.  We set OSC_PCI_EDR_SUPPORT whenever
> CONFIG_PCIE_EDR=y, so I think we should install the notify handler
> here unconditionally (since this file is compiled only when
> CONFIG_PCIE_EDR=y).

Although spec does not provide any restrictions on when to install EDR 
notification, it provides reference that notification is only used if 
firmware owns DPC. So when OS owns DPC, there is no need to install them 
at all.

Although installing them when OS owns DPC should not affect anything, it 
also opens up a additional way for firmware to mess up things. For 
example, consider a case when firmware gives OS control of DPC, but 
still sends EDR notification to OS. Although its unrealistic, I am just 
giving an example.

> 
> I don't think we should even test pcie_ports_dpc_native here.  If we
> told the platform we can handle EDR notifications, we should be
> prepared to get them, regardless of whether the user booted with
> "pcie_ports=dpc-native".

As per the command line parameter documentation, setting 
pcie_ports=dpc-native means, we will be using native PCIe service for 
DPC. So if DPC is handled by OS, as per my argument mentioned above (EDR 
is only useful if
DPC handled by firmware), there is no use in installing EDR notification.

https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L3642

dpc-native - Use native PCIe service for DPC only.

> 
> It's conceivable that pcie_ports_dpc_native should make us do
> something different in the notify handler after we *get* a
> notification, but I doubt we should even worry about that.
> 
> IIUC, pcie_ports_dpc_native exists because Linux DPC originally worked
> even if the OS didn't have control of AER.  eed85ff4c0da7 ("PCI/DPC:
> Enable DPC only if AER is available") meant that if Linux didn't have
> control of AER, DPC no longer worked.  "pcie_ports=dpc-native" is
> basically a way to get that previous behavior of Linux DPC regardless
> of AER control.
> 
> I don't think that issue applies to EDR.  There's no concept of an OS
> "enabling" or "being granted control of" EDR.  The OS merely
> advertises that "yes, I'm prepared to handle EDR notifications".
> AFAICT, the ECR says nothing about EDR support being conditional on OS
> control of AER or DPC.  The notify *handler* might need to do
> different things depending on whether we have AER or DPC control, but
> the handler itself should be registered regardless.
> 
> [1] https://lore.kernel.org/linux-pci/20181115231605.24352-1-mr.nuke.me@gmail.com/
> [2] https://lore.kernel.org/linux-pci/20190326172343.28946-1-mr.nuke.me@gmail.com/
>
Bjorn Helgaas March 6, 2020, 9 p.m. UTC | #3 
On Thu, Mar 05, 2020 at 10:32:33PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> On 3/5/2020 7:47 PM, Bjorn Helgaas wrote:
> > On Tue, Mar 03, 2020 at 06:36:34PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > > +void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
> > > +{
> > > +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> > > +	acpi_status astatus;
> > > +
> > > +	if (!adev) {
> > > +		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Per the Downstream Port Containment Related Enhancements ECN to
> > > +	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
> > > +	 * can only be enabled if DPC is controlled by firmware.
> > > +	 *
> > > +	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
> > > +	 * determine ownership of DPC between firmware or OS.
> > > +	 * Per the Downstream Port Containment Related Enhancements
> > > +	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> > > +	 * OS can use bit 7 of _OSC control field to negotiate control
> > > +	 * over DPC Capability.
> > > +	 */
> > > +	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
> > > +		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
> > > +		return;
> > > +	}
> > > +
> > > +	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> > > +					      edr_handle_event, pdev);
> > 
> > I think this is still problematic.  You mentioned Alex's work
> > [1,2].  We do need to revisit those patches, but I don't really
> > want to defer *this* question of the EDR notify handler.
> > Negotiating support of AER/DPC/EDR is already complicated, and I
> > don't want to complicate it even more by merging something we
> > already know is not quite right.
> > 
> > I don't understand your comment that "EDR can only be enabled if
> > DPC is controlled by firmware."  I don't see anything in table 4-6
> > to that effect.  The only mention of EDR there is to say that the
> > OS can access the DPC capability in the EDR processing window,
> > i.e., after the OS receives the EDR notification and before it
> > clears DPC Trigger Status.
> 
> Please check the following spec reference (from table 4-6).
> 
>     If control of this feature was requested and denied, firmware is
>     responsible for initializing Downstream Port Containment
>     Extended Capability Structures per firmware policy. Further, the
>     OS is permitted to read or write DPC Control and Status
>     registers of a port while processing an Error Disconnect Recover
>     notification from firmware on that port.
> 
> It specifies firmware is expected to use EDR notification *only*
> when the control of DPC is requested and denied (which means
> firmware owns the DPC).

No, I don't think it says that.  This section tells us how to use _OSC
to negotiate ownership of DPC.  The first sentence you quoted
basically says that if firmware retains control of DPC, firmware is
responsible for initializing the DPC capability.  That part is pretty
obvious: "firmware owns it, so firmware is responsible for configuring
it."

The second sentence is important because it's an exception to the
general rule of "the OS can't touch things owned by the firmware." The
exception is that even if firmware retains control of DPC, the OS is
allowed to access DPC registers during the EDR notification window.

There is nothing here about when firmware is allowed to use EDR.

> Although it does not explicitly state that we should install EDR
> notification handler only if firmware owns DPC, it mentions that EDR
> notification is only used if firmware owns DPC. So why should we
> install it if it's not going to be used when OS owns DPC.

It does not say anything about "EDR notification only being used if
firmware owns DPC."

We should install an EDR notify handler because we told the firmware
that we support EDR notifications.  I don't think we should make it
any more complicated than that.

> Also check the following reference from section 2 of EDR ECN. It also
> clarifies EDR feature is only used when firmware owns DPC.
> 
>     PCIe Base Specification suggests that Downstream Port Containment
>     may be controlled either by the Firmware or the Operating System. It
>     also suggests that the Firmware retain ownership of Downstream Port
>     Containment if it also owns AER. 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*.

The text in section 2 will not become part of the spec, so we can't
rely on it to tell us how to implement things.  Even if it did, this
section does not say "OS should only install an EDR notify handler if
firmware owns DPC."  It just means that if firmware owns DPC, the OS
will not learn about DPC events directly via DPC interrupts, so
firmware has to use another mechanism, e.g., EDR, to tell the OS about
them.

If an OS requests DPC control, it must support both DPC and EDR (sec
4.5.2.4).  However, I think an OS may support EDR but not DPC
(although your patches don't support this configuration).  In that
case the OS would set the _OSC "EDR Supported" bit, but it would not
request DPC control.  Then the EDR notify handler would "invalidate
the software state associated with child devices of the port" (table
4-4), but it would not "attempt to recover the child devices of ports
implementing DPC."

> > EDR is a general ACPI feature that is not PCI-specific.  For EDR
> > on PCI devices, OS support is advertised via _OSC *Support* (table
> > 4-4), which says:
> > 
> >    Error Disconnect Recover Supported
> > 
> >    The OS sets this bit to 1 if it supports Error Disconnect
> >    Recover notification on PCI Express Host Bridges, Root Ports
> >    and Switch Downstream Ports. Otherwise, the OS sets this bit to
> >    0.
> > 
> > I think that means that if we set the "Error Disconnect Recover
> > Supported" _OSC bit (OSC_PCI_EDR_SUPPORT), we must install a
> > handler for EDR notifications.  We set OSC_PCI_EDR_SUPPORT
> > whenever CONFIG_PCIE_EDR=y, so I think we should install the
> > notify handler here unconditionally (since this file is compiled
> > only when CONFIG_PCIE_EDR=y).
> 
> Although spec does not provide any restrictions on when to install
> EDR notification, it provides reference that notification is only
> used if firmware owns DPC. So when OS owns DPC, there is no need to
> install them at all.

I disagree that the spec tells us that EDR is only used when firmware
owns DPC.

Even if it did, pcie_aer_get_firmware_first() only looks at HEST
tables.  There *might* be some connection between those and DPC
ownership, but that's internal to firmware and I think it's just
asking for trouble if we rely on that connection.

> Although installing them when OS owns DPC should not affect
> anything, it also opens up a additional way for firmware to mess up
> things. For example, consider a case when firmware gives OS control
> of DPC, but still sends EDR notification to OS. Although it's
> unrealistic, I am just giving an example.

Can you outline the problem that occurs in this scenario?  It seems
like the EDR notify handler could still work.  The OS can access DPC
at any time (not just during the EDR window).

> > I don't think we should even test pcie_ports_dpc_native here.  If we
> > told the platform we can handle EDR notifications, we should be
> > prepared to get them, regardless of whether the user booted with
> > "pcie_ports=dpc-native".
> 
> As per the command line parameter documentation, setting
> pcie_ports=dpc-native means, we will be using native PCIe service
> for DPC.  So if DPC is handled by OS, as per my argument mentioned
> above (EDR is only useful if DPC handled by firmware), there is no
> use in installing EDR notification.
> 
> https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L3642
> 
> dpc-native - Use native PCIe service for DPC only.

It doesn't hurt anything to install a notify handler that never
receives a notification.  It might be an issue if we tell firmware
we're prepared for notifications but we don't install a handler.

> > It's conceivable that pcie_ports_dpc_native should make us do
> > something different in the notify handler after we *get* a
> > notification, but I doubt we should even worry about that.
> > 
> > IIUC, pcie_ports_dpc_native exists because Linux DPC originally worked
> > even if the OS didn't have control of AER.  eed85ff4c0da7 ("PCI/DPC:
> > Enable DPC only if AER is available") meant that if Linux didn't have
> > control of AER, DPC no longer worked.  "pcie_ports=dpc-native" is
> > basically a way to get that previous behavior of Linux DPC regardless
> > of AER control.
> > 
> > I don't think that issue applies to EDR.  There's no concept of an OS
> > "enabling" or "being granted control of" EDR.  The OS merely
> > advertises that "yes, I'm prepared to handle EDR notifications".
> > AFAICT, the ECR says nothing about EDR support being conditional on OS
> > control of AER or DPC.  The notify *handler* might need to do
> > different things depending on whether we have AER or DPC control, but
> > the handler itself should be registered regardless.
> > 
> > [1] https://lore.kernel.org/linux-pci/20181115231605.24352-1-mr.nuke.me@gmail.com/
> > [2] https://lore.kernel.org/linux-pci/20190326172343.28946-1-mr.nuke.me@gmail.com/
> > 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
Kuppuswamy Sathyanarayanan March 6, 2020, 10:42 p.m. UTC | #4 
Hi Bjorn,

On 3/6/20 1:00 PM, Bjorn Helgaas wrote:
> On Thu, Mar 05, 2020 at 10:32:33PM -0800, Kuppuswamy, Sathyanarayanan wrote:
>> On 3/5/2020 7:47 PM, Bjorn Helgaas wrote:
>>> On Tue, Mar 03, 2020 at 06:36:34PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> +void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
>>>> +{
>>>> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>>>> +	acpi_status astatus;
>>>> +
>>>> +	if (!adev) {
>>>> +		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Per the Downstream Port Containment Related Enhancements ECN to
>>>> +	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
>>>> +	 * can only be enabled if DPC is controlled by firmware.
>>>> +	 *
>>>> +	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
>>>> +	 * determine ownership of DPC between firmware or OS.
>>>> +	 * Per the Downstream Port Containment Related Enhancements
>>>> +	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
>>>> +	 * OS can use bit 7 of _OSC control field to negotiate control
>>>> +	 * over DPC Capability.
>>>> +	 */
>>>> +	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
>>>> +		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
>>>> +					      edr_handle_event, pdev);
>>>
>>> It does not say anything about "EDR notification only being used if
>>> firmware owns DPC."
>>>
>>> We should install an EDR notify handler because we told the firmware
>>> that we support EDR notifications.  I don't think we should make it
>>> any more complicated than that.
I agree with your above statement. Since we told firmware *we support*
EDR notification, we should make that true by installing the notification
handler unconditionally.

But, based on inferences from PCI FW 3.2 ECN-DPC spec, current use case 
of EDR
notification is only to handle error recovery for the case where DPC is 
owned by firmware
and firmware sends EDR event. if you agree with above comment, is it 
alright if we add
the following check in EDR notification handler ?

Although spec does not restrict it, current tested use case of EDR is to 
handle notification
for firmware DPC case.

218         if (!pcie_aer_get_firmware_first(pdev) || 
pcie_ports_dpc_native || (host->native_dpc))
219                 return;



>
>> Also check the following reference from section 2 of EDR ECN. It also
>> clarifies EDR feature is only used when firmware owns DPC.
>>
>>      PCIe Base Specification suggests that Downstream Port Containment
>>      may be controlled either by the Firmware or the Operating System. It
>>      also suggests that the Firmware retain ownership of Downstream Port
>>      Containment if it also owns AER. 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*.
> The text in section 2 will not become part of the spec, so we can't
> rely on it to tell us how to implement things.  Even if it did, this
> section does not say "OS should only install an EDR notify handler if
> firmware owns DPC."  It just means that if firmware owns DPC, the OS
> will not learn about DPC events directly via DPC interrupts, so
> firmware has to use another mechanism, e.g., EDR, to tell the OS about
> them.
>
> If an OS requests DPC control, it must support both DPC and EDR (sec
> 4.5.2.4).  However, I think an OS may support EDR but not DPC
> (although your patches don't support this configuration).
Any use cases for above configuration ? Current PCI FW 3.2 ECN-DPC
spec does not mention any uses cases where EDR can be used outside
the scope of DPC ?

If required I can add this support. It should be easy to add it. In non 
DPC case,
EDR notification handler would mostly be empty. Please let me know if you
want me add this part of next patch set.

> In that
> case the OS would set the _OSC "EDR Supported" bit, but it would not
> request DPC control.  Then the EDR notify handler would "invalidate
> the software state associated with child devices of the port" (table
> 4-4), but it would not "attempt to recover the child devices of ports
> implementing DPC."
>
>>> EDR is a general ACPI feature that is not PCI-specific.  For EDR
>>> on PCI devices, OS support is advertised via _OSC *Support* (table
>>> 4-4), which says:
>>>
>>>     Error Disconnect Recover Supported
>>>
>>>     The OS sets this bit to 1 if it supports Error Disconnect
>>>     Recover notification on PCI Express Host Bridges, Root Ports
>>>     and Switch Downstream Ports. Otherwise, the OS sets this bit to
>>>     0.
>>>
>>> I think that means that if we set the "Error Disconnect Recover
>>> Supported" _OSC bit (OSC_PCI_EDR_SUPPORT), we must install a
>>> handler for EDR notifications.  We set OSC_PCI_EDR_SUPPORT
>>> whenever CONFIG_PCIE_EDR=y, so I think we should install the
>>> notify handler here unconditionally (since this file is compiled
>>> only when CONFIG_PCIE_EDR=y).
>> Although spec does not provide any restrictions on when to install
>> EDR notification, it provides reference that notification is only
>> used if firmware owns DPC. So when OS owns DPC, there is no need to
>> install them at all.
> I disagree that the spec tells us that EDR is only used when firmware
> owns DPC.
Agreed.
>
> Even if it did, pcie_aer_get_firmware_first() only looks at HEST
> tables.  There *might* be some connection between those and DPC
> ownership, but that's internal to firmware and I think it's just
> asking for trouble if we rely on that connection.
>
>> Although installing them when OS owns DPC should not affect
>> anything, it also opens up a additional way for firmware to mess up
>> things. For example, consider a case when firmware gives OS control
>> of DPC, but still sends EDR notification to OS. Although it's
>> unrealistic, I am just giving an example.
> Can you outline the problem that occurs in this scenario?  It seems
> like the EDR notify handler could still work.  The OS can access DPC
> at any time (not just during the EDR window).
When OS owns DPC and firmware sends  a EDR event, it could
create race between DPC interrupt handler and EDR
event handler. Although from hardware perspective it should
not make difference, since both code paths does the same thing.
>
>>> I don't think we should even test pcie_ports_dpc_native here.  If we
>>> told the platform we can handle EDR notifications, we should be
>>> prepared to get them, regardless of whether the user booted with
>>> "pcie_ports=dpc-native".
>> As per the command line parameter documentation, setting
>> pcie_ports=dpc-native means, we will be using native PCIe service
>> for DPC.  So if DPC is handled by OS, as per my argument mentioned
>> above (EDR is only useful if DPC handled by firmware), there is no
>> use in installing EDR notification.
>>
>> https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L3642
>>
>> dpc-native - Use native PCIe service for DPC only.
> It doesn't hurt anything to install a notify handler that never
> receives a notification.  It might be an issue if we tell firmware
> we're prepared for notifications but we don't install a handler.
Agreed. Shall I send another version with this and "static inline" fix ?
>
>>> It's conceivable that pcie_ports_dpc_native should make us do
>>> something different in the notify handler after we *get* a
>>> notification, but I doubt we should even worry about that.
>>>
>>> IIUC, pcie_ports_dpc_native exists because Linux DPC originally worked
>>> even if the OS didn't have control of AER.  eed85ff4c0da7 ("PCI/DPC:
>>> Enable DPC only if AER is available") meant that if Linux didn't have
>>> control of AER, DPC no longer worked.  "pcie_ports=dpc-native" is
>>> basically a way to get that previous behavior of Linux DPC regardless
>>> of AER control.
>>>
>>> I don't think that issue applies to EDR.  There's no concept of an OS
>>> "enabling" or "being granted control of" EDR.  The OS merely
>>> advertises that "yes, I'm prepared to handle EDR notifications".
>>> AFAICT, the ECR says nothing about EDR support being conditional on OS
>>> control of AER or DPC.  The notify *handler* might need to do
>>> different things depending on whether we have AER or DPC control, but
>>> the handler itself should be registered regardless.
>>>
>>> [1] https://lore.kernel.org/linux-pci/20181115231605.24352-1-mr.nuke.me@gmail.com/
>>> [2] https://lore.kernel.org/linux-pci/20190326172343.28946-1-mr.nuke.me@gmail.com/
>>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
Bjorn Helgaas March 6, 2020, 11:23 p.m. UTC | #5 
On Fri, Mar 06, 2020 at 02:42:14PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 3/6/20 1:00 PM, Bjorn Helgaas wrote:
> > On Thu, Mar 05, 2020 at 10:32:33PM -0800, Kuppuswamy, Sathyanarayanan wrote:
> > > On 3/5/2020 7:47 PM, Bjorn Helgaas wrote:
> > > > On Tue, Mar 03, 2020 at 06:36:34PM -0800, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > +void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
> > > > > +{
> > > > > +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> > > > > +	acpi_status astatus;
> > > > > +
> > > > > +	if (!adev) {
> > > > > +		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Per the Downstream Port Containment Related Enhancements ECN to
> > > > > +	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
> > > > > +	 * can only be enabled if DPC is controlled by firmware.
> > > > > +	 *
> > > > > +	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
> > > > > +	 * determine ownership of DPC between firmware or OS.
> > > > > +	 * Per the Downstream Port Containment Related Enhancements
> > > > > +	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
> > > > > +	 * OS can use bit 7 of _OSC control field to negotiate control
> > > > > +	 * over DPC Capability.
> > > > > +	 */
> > > > > +	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
> > > > > +		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
> > > > > +					      edr_handle_event, pdev);
> > > > 
> > > > It does not say anything about "EDR notification only being
> > > > used if firmware owns DPC."
> > > > 
> > > > We should install an EDR notify handler because we told the
> > > > firmware that we support EDR notifications.  I don't think we
> > > > should make it any more complicated than that.
>
> I agree with your above statement. Since we told firmware *we
> support* EDR notification, we should make that true by installing
> the notification handler unconditionally.
> 
> But, based on inferences from PCI FW 3.2 ECN-DPC spec, current use
> case of EDR notification is only to handle error recovery for the
> case where DPC is owned by firmware and firmware sends EDR event. if
> you agree with above comment, is it alright if we add the following
> check in EDR notification handler ?
> 
> Although spec does not restrict it, current tested use case of EDR
> is to handle notification for firmware DPC case.
> 
> 218         if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native
> || (host->native_dpc))
> 219                 return;

No, I do not think we should add a check like this.  There's no basis
in the spec for doing this.  pcie_aer_get_firmware_first() looks at
HEST, which isn't mentioned at all in relation to EDR.  Checks like
this make it really hard to understand the code, and I don't believe
in making things fail simply because we haven't tested the scenario.

> > > Also check the following reference from section 2 of EDR ECN. It also
> > > clarifies EDR feature is only used when firmware owns DPC.
> > > 
> > >      PCIe Base Specification suggests that Downstream Port Containment
> > >      may be controlled either by the Firmware or the Operating System. It
> > >      also suggests that the Firmware retain ownership of Downstream Port
> > >      Containment if it also owns AER. 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*.
> > The text in section 2 will not become part of the spec, so we can't
> > rely on it to tell us how to implement things.  Even if it did, this
> > section does not say "OS should only install an EDR notify handler if
> > firmware owns DPC."  It just means that if firmware owns DPC, the OS
> > will not learn about DPC events directly via DPC interrupts, so
> > firmware has to use another mechanism, e.g., EDR, to tell the OS about
> > them.
> > 
> > If an OS requests DPC control, it must support both DPC and EDR
> > (sec 4.5.2.4).  However, I think an OS may support EDR but not DPC
> > (although your patches don't support this configuration).
>
> Any use cases for above configuration ? Current PCI FW 3.2 ECN-DPC
> spec does not mention any uses cases where EDR can be used outside
> the scope of DPC ?
> 
> If required I can add this support. It should be easy to add it. In
> non DPC case, EDR notification handler would mostly be empty. Please
> let me know if you want me add this part of next patch set.

I don't think there's a need to add support for this.  I just
mentioned it as part of the point that we shouldn't tie EDR to DPC
unnecessarily.

> > > Although installing them when OS owns DPC should not affect
> > > anything, it also opens up a additional way for firmware to mess
> > > up things. For example, consider a case when firmware gives OS
> > > control of DPC, but still sends EDR notification to OS. Although
> > > it's unrealistic, I am just giving an example.
>
> > Can you outline the problem that occurs in this scenario?  It
> > seems like the EDR notify handler could still work.  The OS can
> > access DPC at any time (not just during the EDR window).
>
> When OS owns DPC and firmware sends a EDR event, it could create
> race between DPC interrupt handler and EDR event handler. Although
> from hardware perspective it should not make difference, since both
> code paths does the same thing.

Yes, that's true.  I think we should wait until there is a problem
here before doing anything.

> > > > I don't think we should even test pcie_ports_dpc_native here.  If we
> > > > told the platform we can handle EDR notifications, we should be
> > > > prepared to get them, regardless of whether the user booted with
> > > > "pcie_ports=dpc-native".
> > > As per the command line parameter documentation, setting
> > > pcie_ports=dpc-native means, we will be using native PCIe service
> > > for DPC.  So if DPC is handled by OS, as per my argument mentioned
> > > above (EDR is only useful if DPC handled by firmware), there is no
> > > use in installing EDR notification.
> > > 
> > > https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L3642
> > > 
> > > dpc-native - Use native PCIe service for DPC only.
> > It doesn't hurt anything to install a notify handler that never
> > receives a notification.  It might be an issue if we tell firmware
> > we're prepared for notifications but we don't install a handler.
> Agreed. Shall I send another version with this and "static inline" fix ?

No need.  Just take a look at my review/edr branch.  I intend to tweak
some commit logs and (maybe) make the "clear status" functions void
since there are only one or two minor uses of the return values.  But
it's pretty much what I hope to merge.

Bjorn
Kuppuswamy Sathyanarayanan March 7, 2020, 12:19 a.m. UTC | #6 
Hi Bjorn,

On 3/6/20 3:23 PM, Bjorn Helgaas wrote:
> No need.  Just take a look at my review/edr branch.  I intend to tweak
> some commit logs and (maybe) make the "clear status" functions void
> since there are only one or two minor uses of the return values.  But
> it's pretty much what I hope to merge.

Your review/edr branch seems to have covered everything.
Thanks for working on it.

>
> Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0c02d500158f..6af5d6a04990 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -1258,6 +1258,7 @@  static void pci_acpi_setup(struct device *dev)
 
 	acpi_pci_wakeup(pci_dev, false);
 	acpi_device_power_add_dependent(adev, dev);
+	pci_acpi_add_edr_notifier(pci_dev);
 }
 
 static void pci_acpi_cleanup(struct device *dev)
@@ -1276,6 +1277,8 @@  static void pci_acpi_cleanup(struct device *dev)
 
 		device_set_wakeup_capable(dev, false);
 	}
+
+	pci_acpi_remove_edr_notifier(pci_dev);
 }
 
 static bool pci_acpi_bus_match(struct device *dev)
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 6e3c04b46fb1..772b1f4cb19e 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -140,3 +140,13 @@  config PCIE_BW
 	  This enables PCI Express Bandwidth Change Notification.  If
 	  you know link width or rate changes occur only to correct
 	  unreliable links, you may answer Y.
+
+config PCIE_EDR
+	bool "PCI Express Error Disconnect Recover support"
+	depends on PCIE_DPC && ACPI
+	help
+	  This option adds Error Disconnect Recover support as specified
+	  in the Downstream Port Containment Related Enhancements ECN to
+	  the PCI Firmware Specification r3.2.  Enable this if you want to
+	  support hybrid DPC model which uses both firmware and OS to
+	  implement DPC.
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index efb9d2e71e9e..68da9280ff11 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -13,3 +13,4 @@  obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
 obj-$(CONFIG_PCIE_PTM)		+= ptm.o
 obj-$(CONFIG_PCIE_BW)		+= bw_notification.o
+obj-$(CONFIG_PCIE_EDR)		+= edr.o
diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c
new file mode 100644
index 000000000000..2d8680be0302
--- /dev/null
+++ b/drivers/pci/pcie/edr.c
@@ -0,0 +1,249 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI DPC Error Disconnect Recover support driver
+ * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
+ *
+ * Copyright (C) 2020 Intel Corp.
+ */
+
+#define dev_fmt(fmt) "EDR: " fmt
+
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+
+#include "portdrv.h"
+#include "../pci.h"
+
+#define EDR_PORT_ENABLE_DSM		0x0C
+#define EDR_PORT_LOCATE_DSM		0x0D
+#define EDR_OST_SUCCESS			0x80
+#define EDR_OST_FAILED			0x81
+
+/*
+ * _DSM wrapper function to enable/disable DPC port.
+ * @pdev   : PCI device structure.
+ *
+ * returns 0 on success or errno on failure.
+ */
+static int acpi_enable_dpc_port(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj, argv4, req;
+	int status = 0;
+
+	req.type = ACPI_TYPE_INTEGER;
+	req.integer.value = 1;
+
+	argv4.type = ACPI_TYPE_PACKAGE;
+	argv4.package.count = 1;
+	argv4.package.elements = &req;
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Specification r3.2, sec 4.6.12,
+	 * EDR_PORT_ENABLE_DSM is optional.  Return success if it's not
+	 * implemented.
+	 */
+	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+				EDR_PORT_ENABLE_DSM, &argv4);
+	if (!obj)
+		return 0;
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		pci_err(pdev, "_DSM 0x0C returns non integer value\n");
+		status = -EIO;
+	}
+
+	if (obj->integer.value != 1) {
+		pci_err(pdev, "failed to enable DPC port\n");
+		status = -EIO;
+	}
+
+	ACPI_FREE(obj);
+
+	return status;
+}
+
+/*
+ * _DSM wrapper function to locate DPC port.
+ * @pdev   : Device which received EDR event.
+ *
+ * returns pci_dev or NULL.
+ */
+static struct pci_dev *acpi_dpc_port_get(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	union acpi_object *obj;
+	u16 port;
+
+	obj = acpi_evaluate_dsm(adev->handle, &pci_acpi_dsm_guid, 5,
+				EDR_PORT_LOCATE_DSM, NULL);
+	if (!obj)
+		return pci_dev_get(pdev);
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		ACPI_FREE(obj);
+		pci_err(pdev, "_DSM 0x0D returns non integer value\n");
+		return NULL;
+	}
+
+	/*
+	 * Firmware returns DPC port BDF details in following format:
+	 *	15:8 = bus
+	 *	 7:3 = device
+	 *	 2:0 = function
+	 */
+	port = obj->integer.value;
+
+	ACPI_FREE(obj);
+
+	return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
+					   PCI_BUS_NUM(port), port & 0xff);
+}
+
+/*
+ * _OST wrapper function to let firmware know the status of EDR event.
+ * @pdev   : Device used to send _OST.
+ * @edev   : Device which experienced EDR event.
+ * @status: Status of EDR event.
+ */
+static int acpi_send_edr_status(struct pci_dev *pdev, struct pci_dev *edev,
+				u16 status)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	u32 ost_status;
+
+	pci_dbg(pdev, "Sending EDR status :%#x\n", status);
+
+	ost_status =  PCI_DEVID(edev->bus->number, edev->devfn);
+	ost_status = (ost_status << 16) | status;
+
+	status = acpi_evaluate_ost(adev->handle,
+				   ACPI_NOTIFY_DISCONNECT_RECOVER,
+				   ost_status, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
+}
+
+static void edr_handle_event(acpi_handle handle, u32 event, void *data)
+{
+	struct pci_dev *pdev = data, *edev;
+	pci_ers_result_t estate = PCI_ERS_RESULT_DISCONNECT;
+	u16 status;
+
+	pci_info(pdev, "ACPI event %#x received\n", event);
+
+	if (event != ACPI_NOTIFY_DISCONNECT_RECOVER)
+		return;
+
+	/*
+	 * Check if _DSM(0xD) is available, and if present locate the
+	 * port which issued EDR event.
+	 */
+	edev = acpi_dpc_port_get(pdev);
+	if (!edev) {
+		pci_err(pdev, "Firmware failed to locate DPC port\n");
+		return;
+	}
+
+	pci_dbg(pdev, "Reported EDR dev: %s\n", pci_name(edev));
+
+	/*
+	 * If port does not support DPC, just send the OST:
+	 */
+	if (!edev->dpc_cap) {
+		pci_err(edev, "Firmware BUG, located port doesn't support DPC\n");
+		goto send_ost;
+	}
+
+	/* Check if there is a valid DPC trigger */
+	pci_read_config_word(edev, edev->dpc_cap + PCI_EXP_DPC_STATUS, &status);
+	if (!(status & PCI_EXP_DPC_STATUS_TRIGGER)) {
+		pci_err(edev, "Invalid DPC trigger %#010x\n", status);
+		goto send_ost;
+	}
+
+	dpc_process_error(edev);
+
+	/* Clear AER registers */
+	pci_aer_raw_clear_status(edev);
+
+	/*
+	 * Irrespective of whether the DPC event is triggered by
+	 * ERR_FATAL or ERR_NONFATAL, since the link is already down,
+	 * use the FATAL error recovery path for both cases.
+	 */
+	estate = pcie_do_recovery(edev, pci_channel_io_frozen, dpc_reset_link);
+
+	pci_dbg(edev, "DPC port successfully recovered\n");
+send_ost:
+
+	/*
+	 * If recovery is successful, send _OST(0xF, BDF << 16 | 0x80)
+	 * to firmware. If not successful, send _OST(0xF, BDF << 16 | 0x81).
+	 */
+	if (estate == PCI_ERS_RESULT_RECOVERED)
+		acpi_send_edr_status(pdev, edev, EDR_OST_SUCCESS);
+	else
+		acpi_send_edr_status(pdev, edev, EDR_OST_FAILED);
+
+	pci_dev_put(edev);
+}
+
+void pci_acpi_add_edr_notifier(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status astatus;
+
+	if (!adev) {
+		pci_dbg(pdev, "No valid ACPI node, so skip EDR init\n");
+		return;
+	}
+
+	/*
+	 * Per the Downstream Port Containment Related Enhancements ECN to
+	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-6, EDR support
+	 * can only be enabled if DPC is controlled by firmware.
+	 *
+	 * TODO: Remove dependency on ACPI FIRMWARE_FIRST bit to
+	 * determine ownership of DPC between firmware or OS.
+	 * Per the Downstream Port Containment Related Enhancements
+	 * ECN to the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
+	 * OS can use bit 7 of _OSC control field to negotiate control
+	 * over DPC Capability.
+	 */
+	if (!pcie_aer_get_firmware_first(pdev) || pcie_ports_dpc_native) {
+		pci_dbg(pdev, "OS handles AER/DPC, so skip EDR init\n");
+		return;
+	}
+
+	astatus = acpi_install_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
+					      edr_handle_event, pdev);
+	if (ACPI_FAILURE(astatus)) {
+		pci_err(pdev, "Install ACPI_SYSTEM_NOTIFY handler failed\n");
+		return;
+	}
+
+	if (acpi_enable_dpc_port(pdev))
+		acpi_remove_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
+					   edr_handle_event);
+
+	pci_dbg(pdev, "EDR notifier is added successfully\n");
+
+	return;
+}
+
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+	if (!adev)
+		return;
+
+	pci_dbg(pdev, "EDR notifier is removed successfully\n");
+
+	acpi_remove_notify_handler(adev->handle, ACPI_SYSTEM_NOTIFY,
+				   edr_handle_event);
+}
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 62b7fdcc661c..2d155bfb8fbf 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -112,6 +112,14 @@  extern const guid_t pci_acpi_dsm_guid;
 #define RESET_DELAY_DSM			0x08
 #define FUNCTION_DELAY_DSM		0x09
 
+#ifdef CONFIG_PCIE_EDR
+void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
+#else
+static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
+static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
+#endif /* CONFIG_PCIE_EDR */
+
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }