Re: [PATCH v2 3/6] PCI/EDR: Remove CONFIG_PCIE_EDR

[Kuppuswamy Sathyanarayanan]

Hi,

On 11/3/2026 12:46 PM, Yazen Ghannam wrote:
> On Tue, Oct 20, 2026 at 11:19:20AM -0700, Kuppuswamy Sathyanarayanan wrote:
>> Error Disconnect Recover (EDR) is the ACPI half of the hybrid DPC model,
>> where firmware owns DPC and notifies the OS of containment events.
>>
>> CONFIG_PCIE_EDR made EDR optional on top of CONFIG_PCIE_DPC.  This adds
>> another knob to the decision to request DPC control in _OSC.  The OS
>> requests DPC control only if it also supports EDR, so with
>> CONFIG_PCIE_EDR=n it never requests DPC control.  There is little reason
>> to build DPC without EDR on ACPI systems.
>>
>> Remove the option and build edr.c whenever CONFIG_PCIE_DPC and
>> CONFIG_ACPI are both enabled.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/acpi/pci_root.c   | 6 +++---
>>  drivers/pci/pcie/Kconfig  | 9 ---------
>>  drivers/pci/pcie/Makefile | 5 ++++-
>>  drivers/pci/pcie/dpc.c    | 5 +++--
>>  include/linux/pci-acpi.h  | 4 ++--
>>  5 files changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index a1a9c06012f8..b0e0b33e3254 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -438,7 +438,7 @@ static u32 calculate_support(void)
>>  		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
>>  	if (pci_msi_enabled())
>>  		support |= OSC_PCI_MSI_SUPPORT;
>> -	if (IS_ENABLED(CONFIG_PCIE_EDR))
>> +	if (IS_ENABLED(CONFIG_PCIE_DPC))
>>  		support |= OSC_PCI_EDR_SUPPORT;
>>  
>>  	return support;
>> @@ -515,9 +515,9 @@ static u32 calculate_control(void)
>>  	 * 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.
>> +	 * and EDR.  EDR is always built with DPC on ACPI systems.
>>  	 */
>> -	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
>> +	if (IS_ENABLED(CONFIG_PCIE_DPC))
>>  		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
>>  
>>  	return control;
>> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
>> index 207c2deae35f..de4ea33b4524 100644
>> --- a/drivers/pci/pcie/Kconfig
>> +++ b/drivers/pci/pcie/Kconfig
>> @@ -137,12 +137,3 @@ config PCIE_PTM
>>  	  This is only useful if you have devices that support PTM, but it
>>  	  is safe to enable even if you don't.
>>  
>> -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.
> 
> What about keeping 'PCIE_EDR' and using it like 'PCIE_PME'?
> 
> IOW, remove the user-visible parts have it default to (PCIE_DPC &&
> ACPI)?
> 
> I understand EDR as its own feature on top of DPC. So I think there's
> value in having the EDR bits clearly distinct from the DPC bits.
> 
> Also, it would reduce the changes in this patch.

Good idea.  I'll make PCIE_EDR a def_bool that depends on PCIE_DPC &&
ACPI in v3.  That still removes the user-visible knob and keeps the
rest of the code as is.

> 
>> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
>> index b0b43a18c304..46b24311f659 100644
>> --- a/drivers/pci/pcie/Makefile
>> +++ b/drivers/pci/pcie/Makefile
>> @@ -13,4 +13,7 @@ obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
>>  obj-$(CONFIG_PCIE_PME)		+= pme.o
>>  obj-$(CONFIG_PCIE_DPC)		+= dpc.o
>>  obj-$(CONFIG_PCIE_PTM)		+= ptm.o
>> -obj-$(CONFIG_PCIE_EDR)		+= edr.o
>> +
>> +ifdef CONFIG_ACPI
>> +obj-$(CONFIG_PCIE_DPC)		+= edr.o
>> +endif
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index 104ff2b91f1f..71adc0e94bca 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -109,10 +109,11 @@ bool pci_dpc_recovered(struct pci_dev *pdev)
>>  
>>  	/*
>>  	 * Synchronization between hotplug and DPC is not supported
>> -	 * if DPC is owned by firmware and EDR is not enabled.
>> +	 * if DPC is owned by firmware and there is no EDR, i.e., on
>> +	 * non-ACPI systems.
>>  	 */
>>  	host = pci_find_host_bridge(pdev->bus);
>> -	if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
>> +	if (!host->native_dpc && !IS_ENABLED(CONFIG_ACPI))
>>  		return false;
>>  
> 
> This seems like dead code, and it was deleted in an older version.
> 
> The conditions seem to be mutually exclusive.
> 
> Maybe it should just be removed in a pre-patch?

Agreed that it is dead, but only after this patch.  Before it, the
check is still reachable with CONFIG_ACPI=y, CONFIG_PCIE_EDR=n and
firmware-owned DPC.  Once EDR is always built with DPC on ACPI,
native_dpc can only be cleared on ACPI systems, where EDR is present.
So I'll remove the check in this patch.

Thanks for the review.

> 
> Thanks,
> Yazen

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer