| Message ID | 20240222221521.32159-3-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/DPC: Clean up DPC vs AER/EDR ownership and Kconfig | expand |
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);
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.
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 --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);