diff mbox series

[v2,2/3] PCI/DPC: Remove CONFIG_PCIE_EDR

Message ID 20240222221521.32159-3-helgaas@kernel.org
State New
Headers
Series PCI/DPC: Clean up DPC vs AER/EDR ownership and Kconfig |

Commit Message

Bjorn Helgaas Feb. 22, 2024, 10:15 p.m. UTC 
  From: Bjorn Helgaas <bhelgaas@google.com>

Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC
without CONFIG_PCIE_EDR.  The PCI Firmware Spec, r3.3, sec 4.5.1,
table 4-5, says an ACPI OS that requests control of DPC must also
advertise support for EDR.

Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that
enabling DPC also enables EDR for ACPI systems.  Since EDR is an ACPI
feature, build edr.o only when CONFIG_ACPI is enabled.  Stubs cover the
case when DPC is enabled without ACPI.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/pci_root.c   |  2 +-
 drivers/pci/pcie/Kconfig  | 14 ++++----------
 drivers/pci/pcie/Makefile |  5 ++++-
 drivers/pci/pcie/dpc.c    | 10 ----------
 include/linux/pci-acpi.h  |  4 ++--
 5 files changed, 11 insertions(+), 24 deletions(-)

Comments

Kuppuswamy Sathyanarayanan Feb. 25, 2024, 8:05 p.m. UTC | #1 
Hi Bjorn,

On 2/22/24 2:15 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC
> without CONFIG_PCIE_EDR.  The PCI Firmware Spec, r3.3, sec 4.5.1,
> table 4-5, says an ACPI OS that requests control of DPC must also
> advertise support for EDR.
>
> Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that
> enabling DPC also enables EDR for ACPI systems.  Since EDR is an ACPI
> feature, build edr.o only when CONFIG_ACPI is enabled.  Stubs cover the
> case when DPC is enabled without ACPI.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/acpi/pci_root.c   |  2 +-
>  drivers/pci/pcie/Kconfig  | 14 ++++----------
>  drivers/pci/pcie/Makefile |  5 ++++-
>  drivers/pci/pcie/dpc.c    | 10 ----------
>  include/linux/pci-acpi.h  |  4 ++--
>  5 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index efc292b6214e..bcaf3d3a5e05 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -448,7 +448,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))	/* DPC => EDR support */
>  		support |= OSC_PCI_EDR_SUPPORT;

Since EDR internally touches AER registers, I still think we should make sure
OS enables AER support before advertising the EDR support.

>  
>  	return support;
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index 8999fcebde6a..21e98289fbe9 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -137,6 +137,10 @@ config PCIE_DPC
>  	  have this capability or you do not want to use this feature,
>  	  it is safe to answer N.
>  
> +	  On ACPI systems, this includes Error Disconnect Recover support,
> +	  the hybrid model that uses both firmware and OS to implement DPC,
> +	  as specified in the PCI Firmware Specification r3.3.

Nit: Include some section reference?

> +
>  config PCIE_PTM
>  	bool "PCI Express Precision Time Measurement support"
>  	help
> @@ -145,13 +149,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.
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 8de4ed5f98f1..72657f780c33 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -12,4 +12,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 94111e438241..0aa79581250b 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -102,19 +102,9 @@ static bool dpc_completed(struct pci_dev *pdev)
>   */
>  bool pci_dpc_recovered(struct pci_dev *pdev)
>  {
> -	struct pci_host_bridge *host;
> -
>  	if (!pdev->dpc_cap)
>  		return false;
>  
> -	/*
> -	 * Synchronization between hotplug and DPC is not supported
> -	 * if DPC is owned by firmware and EDR is not enabled.
> -	 */
> -	host = pci_find_host_bridge(pdev->bus);
> -	if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
> -		return false;
> -
>  	/*
>  	 * Need a timeout in case DPC never completes due to failure of
>  	 * dpc_wait_rp_inactive().  The spec doesn't mandate a time limit,
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 078225b514d4..92e196ba0249 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -122,13 +122,13 @@ extern const guid_t pci_acpi_dsm_guid;
>  #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
>  #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
>  
> -#ifdef CONFIG_PCIE_EDR
> +#ifdef CONFIG_PCIE_DPC
>  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 */
> +#endif /* CONFIG_PCIE_DPC */
>  
>  int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
>  void pci_acpi_clear_companion_lookup_hook(void);
Bjorn Helgaas March 1, 2024, 11:06 p.m. UTC | #2 
On Sun, Feb 25, 2024 at 12:05:12PM -0800, Kuppuswamy Sathyanarayanan wrote:
> On 2/22/24 2:15 PM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC
> > without CONFIG_PCIE_EDR.  The PCI Firmware Spec, r3.3, sec 4.5.1,
> > table 4-5, says an ACPI OS that requests control of DPC must also
> > advertise support for EDR.
> >
> > Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that
> > enabling DPC also enables EDR for ACPI systems.  Since EDR is an ACPI
> > feature, build edr.o only when CONFIG_ACPI is enabled.  Stubs cover the
> > case when DPC is enabled without ACPI.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/acpi/pci_root.c   |  2 +-
> >  drivers/pci/pcie/Kconfig  | 14 ++++----------
> >  drivers/pci/pcie/Makefile |  5 ++++-
> >  drivers/pci/pcie/dpc.c    | 10 ----------
> >  include/linux/pci-acpi.h  |  4 ++--
> >  5 files changed, 11 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index efc292b6214e..bcaf3d3a5e05 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -448,7 +448,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))	/* DPC => EDR support */
> >  		support |= OSC_PCI_EDR_SUPPORT;
> 
> Since EDR internally touches AER registers, I still think we should
> make sure OS enables AER support before advertising the EDR support.

I guess you're suggesting that we should make it look like this?

  if (host_bridge->native_aer && IS_ENABLED(CONFIG_PCIE_DPC))

That doesn't seem right to me because the implementation note in PCI
Firmware r3.3, sec 4.6.12, shows the EDR flow when firmware maintains
control of AER and DPC, i.e., "host_bridge->native_aer" would be
false.

> >  	return support;
> > diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> > index 8999fcebde6a..21e98289fbe9 100644
> > --- a/drivers/pci/pcie/Kconfig
> > +++ b/drivers/pci/pcie/Kconfig
> > @@ -137,6 +137,10 @@ config PCIE_DPC
> >  	  have this capability or you do not want to use this feature,
> >  	  it is safe to answer N.
> >  
> > +	  On ACPI systems, this includes Error Disconnect Recover support,
> > +	  the hybrid model that uses both firmware and OS to implement DPC,
> > +	  as specified in the PCI Firmware Specification r3.3.
> 
> Nit: Include some section reference?

I basically copied this from the PCIE_EDR help and updated the
revision number.  But I don't think the firmware spec is a very good
reference because EDR is defined by ACPI.  There's very little text in
the ACPI spec about EDR, but the firmware spec does assume you know
what *is* there.  And the ACPI spec is available to anybody, unlike
the PCI firmware spec.

+         On ACPI systems, this includes Error Disconnect Recover support,
+         the hybrid model that uses both firmware and OS to implement DPC,
+         as specified in ACPI r6.5, sec 5.6.6.

> >  config PCIE_PTM
> >  	bool "PCI Express Precision Time Measurement support"
> >  	help
> > @@ -145,13 +149,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.
Kuppuswamy Sathyanarayanan March 2, 2024, 6:42 a.m. UTC | #3 
On 3/1/24 3:06 PM, Bjorn Helgaas wrote:
> On Sun, Feb 25, 2024 at 12:05:12PM -0800, Kuppuswamy Sathyanarayanan wrote:
>> On 2/22/24 2:15 PM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC
>>> without CONFIG_PCIE_EDR.  The PCI Firmware Spec, r3.3, sec 4.5.1,
>>> table 4-5, says an ACPI OS that requests control of DPC must also
>>> advertise support for EDR.
>>>
>>> Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that
>>> enabling DPC also enables EDR for ACPI systems.  Since EDR is an ACPI
>>> feature, build edr.o only when CONFIG_ACPI is enabled.  Stubs cover the
>>> case when DPC is enabled without ACPI.
>>>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>> ---
>>>  drivers/acpi/pci_root.c   |  2 +-
>>>  drivers/pci/pcie/Kconfig  | 14 ++++----------
>>>  drivers/pci/pcie/Makefile |  5 ++++-
>>>  drivers/pci/pcie/dpc.c    | 10 ----------
>>>  include/linux/pci-acpi.h  |  4 ++--
>>>  5 files changed, 11 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>>> index efc292b6214e..bcaf3d3a5e05 100644
>>> --- a/drivers/acpi/pci_root.c
>>> +++ b/drivers/acpi/pci_root.c
>>> @@ -448,7 +448,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))	/* DPC => EDR support */
>>>  		support |= OSC_PCI_EDR_SUPPORT;
>> Since EDR internally touches AER registers, I still think we should
>> make sure OS enables AER support before advertising the EDR support.
> I guess you're suggesting that we should make it look like this?
>
>   if (host_bridge->native_aer && IS_ENABLED(CONFIG_PCIE_DPC))
>
> That doesn't seem right to me because the implementation note in PCI
> Firmware r3.3, sec 4.6.12, shows the EDR flow when firmware maintains
> control of AER and DPC, i.e., "host_bridge->native_aer" would be
> false.

No, my idea is to check for something like below:

if (IS_ENABLED(CONFIG_PCIEAER) && IS_ENABLED(CONFIG_PCIE_DPC)) or if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_DPC) to ensure AER is not disabled by noaer command line option. Since EDR handler depends on AER routines (like pci_aer_raw_clear_status()) to |clear AER registers, we need to ensure AER is enabled in kernel before advertising suppor for EDR.


>
>>>  	return support;
>>> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
>>> index 8999fcebde6a..21e98289fbe9 100644
>>> --- a/drivers/pci/pcie/Kconfig
>>> +++ b/drivers/pci/pcie/Kconfig
>>> @@ -137,6 +137,10 @@ config PCIE_DPC
>>>  	  have this capability or you do not want to use this feature,
>>>  	  it is safe to answer N.
>>>  
>>> +	  On ACPI systems, this includes Error Disconnect Recover support,
>>> +	  the hybrid model that uses both firmware and OS to implement DPC,
>>> +	  as specified in the PCI Firmware Specification r3.3.
>> Nit: Include some section reference?
> I basically copied this from the PCIE_EDR help and updated the
> revision number.  But I don't think the firmware spec is a very good
> reference because EDR is defined by ACPI.  There's very little text in
> the ACPI spec about EDR, but the firmware spec does assume you know
> what *is* there.  And the ACPI spec is available to anybody, unlike
> the PCI firmware spec.
>
> +         On ACPI systems, this includes Error Disconnect Recover support,
> +         the hybrid model that uses both firmware and OS to implement DPC,
> +         as specified in ACPI r6.5, sec 5.6.6.
>
>>>  config PCIE_PTM
>>>  	bool "PCI Express Precision Time Measurement support"
>>>  	help
>>> @@ -145,13 +149,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.
diff mbox series

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index efc292b6214e..bcaf3d3a5e05 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -448,7 +448,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))	/* DPC => EDR support */
 		support |= OSC_PCI_EDR_SUPPORT;
 
 	return support;
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 8999fcebde6a..21e98289fbe9 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -137,6 +137,10 @@  config PCIE_DPC
 	  have this capability or you do not want to use this feature,
 	  it is safe to answer N.
 
+	  On ACPI systems, this includes Error Disconnect Recover support,
+	  the hybrid model that uses both firmware and OS to implement DPC,
+	  as specified in the PCI Firmware Specification r3.3.
+
 config PCIE_PTM
 	bool "PCI Express Precision Time Measurement support"
 	help
@@ -145,13 +149,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.
diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 8de4ed5f98f1..72657f780c33 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -12,4 +12,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 94111e438241..0aa79581250b 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -102,19 +102,9 @@  static bool dpc_completed(struct pci_dev *pdev)
  */
 bool pci_dpc_recovered(struct pci_dev *pdev)
 {
-	struct pci_host_bridge *host;
-
 	if (!pdev->dpc_cap)
 		return false;
 
-	/*
-	 * Synchronization between hotplug and DPC is not supported
-	 * if DPC is owned by firmware and EDR is not enabled.
-	 */
-	host = pci_find_host_bridge(pdev->bus);
-	if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
-		return false;
-
 	/*
 	 * Need a timeout in case DPC never completes due to failure of
 	 * dpc_wait_rp_inactive().  The spec doesn't mandate a time limit,
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 078225b514d4..92e196ba0249 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -122,13 +122,13 @@  extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 
-#ifdef CONFIG_PCIE_EDR
+#ifdef CONFIG_PCIE_DPC
 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 */
+#endif /* CONFIG_PCIE_DPC */
 
 int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
 void pci_acpi_clear_companion_lookup_hook(void);