| Message ID | 20191113094035.22394-1-oohall@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | powerpc/powernv: Disable native PCIe port management | expand |
On Wed, Nov 13, 2019 at 08:40:35PM +1100, Oliver O'Halloran wrote: > On PowerNV the PCIe topology is (currently) managed the powernv platform > code in cooperation with firmware. The PCIe-native service drivers bypass > both and this can cause problems. > > Historically this hasn't been a big deal since the only port service > driver that saw much use was the AER driver. The AER driver relies > a kernel service to report when errors occur rather than acting autonmously > so it's fairly easy to ignore. On PowerNV (and pseries) AER events are > handled through EEH, which ignores the AER service, so it's never been > an issue. > > Unfortunately, the hotplug port service driver (pciehp) does act > autonomously and conflicts with the platform specific hotplug > driver (pnv_php). The main issue is that pciehp claims the interrupt > associated with the PCIe capability which in turn prevents pnv_php from > claiming it. > > This results in hotplug events being handled by pciehp which does not > notify firmware when the PCIe topology changes, and does not setup/teardown > the arch specific PCI device structures (pci_dn) when the topology changes. > The end result is that hot-added devices cannot be enabled and hot-removed > devices may not be fully torn-down on removal. > > We can fix these problems by setting the "pcie_ports_disabled" flag during > platform initialisation. The flag indicates the platform owns the PCIe > ports which stops the portbus driver being registered. > > Cc: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> > Fixes: 66725152fb9f ("PCI/hotplug: PowerPC PowerNV PCI hotplug driver") > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > Sergey, just FYI. I'll try sort out the rest of the hotplug > trainwreck in 5.6. > > The Fixes: here is for the patch that added pnv_php in 4.8. It's been > a problem since then, but wasn't noticed until people started testing > it after the EEH fixes in commit 799abe283e51 ("powerpc/eeh: Clean up > EEH PEs after recovery finishes") went in earlier in the 5.4 cycle. > --- > arch/powerpc/platforms/powernv/pci.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c > index 2825d00..ae62583 100644 > --- a/arch/powerpc/platforms/powernv/pci.c > +++ b/arch/powerpc/platforms/powernv/pci.c > @@ -941,6 +941,23 @@ void __init pnv_pci_init(void) > > pci_add_flags(PCI_CAN_SKIP_ISA_ALIGN); > > +#ifdef CONFIG_PCIEPORTBUS > + /* > + * On PowerNV PCIe devices are (currently) managed in cooperation > + * with firmware. This isn't *strictly* required, but there's enough > + * assumptions baked into both firmware and the platform code that > + * it's unwise to allow the portbus services to be used. > + * > + * We need to fix this eventually, but for now set this flag to disable > + * the portbus driver. The AER service isn't required since that AER > + * events are handled via EEH. The pciehp hotplug driver can't work > + * without kernel changes (and portbus binding breaks pnv_php). The > + * other services also require some thinking about how we're going > + * to integrate them. > + */ > + pcie_ports_disabled = true; > +#endif This is fine, but it feels like sort of a blunt instrument. Is there any practical way to clear pci_host_bridge.native_pcie_hotplug (and native_aer if appropriate) for the PHBs in question? That would also prevent pciehp from binding. We might someday pull portdrv into the PCI core directly instead of as a separate driver, and I'm thinking that might be easier if we have more specific indications of what the core shouldn't use. > /* If we don't have OPAL, eg. in sim, just skip PCI probe */ > if (!firmware_has_feature(FW_FEATURE_OPAL)) > return; > -- > 2.9.5 >
Nothing but pedantic spelling and grammar nits of the commit log follow. -Tyrel On 11/13/19 1:40 AM, Oliver O'Halloran wrote: > On PowerNV the PCIe topology is (currently) managed the powernv platform s/the powernv/by the powernv > code in cooperation with firmware. The PCIe-native service drivers bypass > both and this can cause problems. > > Historically this hasn't been a big deal since the only port service > driver that saw much use was the AER driver. The AER driver relies > a kernel service to report when errors occur rather than acting autonmously s/a kernel/on a kernel autonomously > so it's fairly easy to ignore. On PowerNV (and pseries) AER events are > handled through EEH, which ignores the AER service, so it's never been > an issue. > > Unfortunately, the hotplug port service driver (pciehp) does act > autonomously and conflicts with the platform specific hotplug > driver (pnv_php). The main issue is that pciehp claims the interrupt > associated with the PCIe capability which in turn prevents pnv_php from > claiming it. > > This results in hotplug events being handled by pciehp which does not > notify firmware when the PCIe topology changes, and does not setup/teardown > the arch specific PCI device structures (pci_dn) when the topology changes. > The end result is that hot-added devices cannot be enabled and hot-removed > devices may not be fully torn-down on removal. > > We can fix these problems by setting the "pcie_ports_disabled" flag during > platform initialisation. The flag indicates the platform owns the PCIe > ports which stops the portbus driver being registered. s/being/from being > > Cc: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> > Fixes: 66725152fb9f ("PCI/hotplug: PowerPC PowerNV PCI hotplug driver") > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > Sergey, just FYI. I'll try sort out the rest of the hotplug > trainwreck in 5.6. > > The Fixes: here is for the patch that added pnv_php in 4.8. It's been > a problem since then, but wasn't noticed until people started testing > it after the EEH fixes in commit 799abe283e51 ("powerpc/eeh: Clean up > EEH PEs after recovery finishes") went in earlier in the 5.4 cycle. > --- > arch/powerpc/platforms/powernv/pci.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c > index 2825d00..ae62583 100644 > --- a/arch/powerpc/platforms/powernv/pci.c > +++ b/arch/powerpc/platforms/powernv/pci.c > @@ -941,6 +941,23 @@ void __init pnv_pci_init(void) > > pci_add_flags(PCI_CAN_SKIP_ISA_ALIGN); > > +#ifdef CONFIG_PCIEPORTBUS > + /* > + * On PowerNV PCIe devices are (currently) managed in cooperation > + * with firmware. This isn't *strictly* required, but there's enough > + * assumptions baked into both firmware and the platform code that > + * it's unwise to allow the portbus services to be used. > + * > + * We need to fix this eventually, but for now set this flag to disable > + * the portbus driver. The AER service isn't required since that AER > + * events are handled via EEH. The pciehp hotplug driver can't work > + * without kernel changes (and portbus binding breaks pnv_php). The > + * other services also require some thinking about how we're going > + * to integrate them. > + */ > + pcie_ports_disabled = true; > +#endif > + > /* If we don't have OPAL, eg. in sim, just skip PCI probe */ > if (!firmware_has_feature(FW_FEATURE_OPAL)) > return; >
On Thu, Nov 14, 2019 at 1:31 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > This is fine, but it feels like sort of a blunt instrument. Is there > any practical way to clear pci_host_bridge.native_pcie_hotplug (and > native_aer if appropriate) for the PHBs in question? That would also > prevent pciehp from binding. It is a large hammer, but I don't see a better way to handle it for the moment. I had another look and my initial assessment was wrong in that it's the portbus driver which claims the MSI rather than pciehp itself. The MSI in the PCIe capability is shared between hotplug events, PMEs, and BW notifications so to make the portbus concept work the portbus driver needs to own the interrupt. Basicly, pnv_php and portbus are fundamentally at odds with each other and can't be used concurrently. I also think there's some latent issues with the interaction of DPC and EEH since they operate off the same set of error messages. We haven't run into any problems yet, but I think that's largely because we haven't shipped any systems with DPC enabled. In any case, I'd prefer we disabled portbus until we've fully unpacked what's going on there. > We might someday pull portdrv into the PCI core directly instead of as > a separate driver, and I'm thinking that might be easier if we have > more specific indications of what the core shouldn't use. It's not intended to be a permanent change. In the long term I want to move everything except the initialisation and reset of the PHB out of firmware and into the kernel so we can use more of the native PCIe management features.
On Thu, Nov 14, 2019 at 7:39 AM Tyrel Datwyler <tyreld@linux.ibm.com> wrote: > > Nothing but pedantic spelling and grammar nits of the commit log follow. > > -Tyrel Thanks. My speeling is bad even on a good day and it was not a good day.
On Fri, Nov 15, 2019 at 12:34:50AM +1100, Oliver O'Halloran wrote: > On Thu, Nov 14, 2019 at 1:31 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > This is fine, but it feels like sort of a blunt instrument. Is there > > any practical way to clear pci_host_bridge.native_pcie_hotplug (and > > native_aer if appropriate) for the PHBs in question? That would also > > prevent pciehp from binding. > > It is a large hammer, but I don't see a better way to handle it for > the moment. I had another look and my initial assessment was wrong in > that it's the portbus driver which claims the MSI rather than pciehp > itself. The MSI in the PCIe capability is shared between hotplug > events, PMEs, and BW notifications so to make the portbus concept work > the portbus driver needs to own the interrupt. Basicly, pnv_php and > portbus are fundamentally at odds with each other and can't be used > concurrently. Yeah, that makes sense. Is there a Kconfig symbol for pnv_php? If so, you could make CONFIG_PCIEPORTBUS unselectable in the first place. But I'm guessing there isn't such a symbol because you probably want to be able to build generic kernels that run on machines that *can* use portdrv as well as on PowerNV. So I'm find with the patch as posted. Bjorn
Tyrel Datwyler <tyreld@linux.ibm.com> writes: > Nothing but pedantic spelling and grammar nits of the commit log follow. Thanks, they were annoying me :) cheers > On 11/13/19 1:40 AM, Oliver O'Halloran wrote: >> On PowerNV the PCIe topology is (currently) managed the powernv platform > > s/the powernv/by the powernv > >> code in cooperation with firmware. The PCIe-native service drivers bypass >> both and this can cause problems. >> >> Historically this hasn't been a big deal since the only port service >> driver that saw much use was the AER driver. The AER driver relies >> a kernel service to report when errors occur rather than acting autonmously > > s/a kernel/on a kernel > autonomously > >> so it's fairly easy to ignore. On PowerNV (and pseries) AER events are >> handled through EEH, which ignores the AER service, so it's never been >> an issue. >> >> Unfortunately, the hotplug port service driver (pciehp) does act >> autonomously and conflicts with the platform specific hotplug >> driver (pnv_php). The main issue is that pciehp claims the interrupt >> associated with the PCIe capability which in turn prevents pnv_php from >> claiming it. >> >> This results in hotplug events being handled by pciehp which does not >> notify firmware when the PCIe topology changes, and does not setup/teardown >> the arch specific PCI device structures (pci_dn) when the topology changes. >> The end result is that hot-added devices cannot be enabled and hot-removed >> devices may not be fully torn-down on removal. >> >> We can fix these problems by setting the "pcie_ports_disabled" flag during >> platform initialisation. The flag indicates the platform owns the PCIe >> ports which stops the portbus driver being registered. > > s/being/from being > >> >> Cc: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> >> Fixes: 66725152fb9f ("PCI/hotplug: PowerPC PowerNV PCI hotplug driver") >> Signed-off-by: Oliver O'Halloran <oohall@gmail.com> >> --- >> Sergey, just FYI. I'll try sort out the rest of the hotplug >> trainwreck in 5.6. >> >> The Fixes: here is for the patch that added pnv_php in 4.8. It's been >> a problem since then, but wasn't noticed until people started testing >> it after the EEH fixes in commit 799abe283e51 ("powerpc/eeh: Clean up >> EEH PEs after recovery finishes") went in earlier in the 5.4 cycle. >> --- >> arch/powerpc/platforms/powernv/pci.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c >> index 2825d00..ae62583 100644 >> --- a/arch/powerpc/platforms/powernv/pci.c >> +++ b/arch/powerpc/platforms/powernv/pci.c >> @@ -941,6 +941,23 @@ void __init pnv_pci_init(void) >> >> pci_add_flags(PCI_CAN_SKIP_ISA_ALIGN); >> >> +#ifdef CONFIG_PCIEPORTBUS >> + /* >> + * On PowerNV PCIe devices are (currently) managed in cooperation >> + * with firmware. This isn't *strictly* required, but there's enough >> + * assumptions baked into both firmware and the platform code that >> + * it's unwise to allow the portbus services to be used. >> + * >> + * We need to fix this eventually, but for now set this flag to disable >> + * the portbus driver. The AER service isn't required since that AER >> + * events are handled via EEH. The pciehp hotplug driver can't work >> + * without kernel changes (and portbus binding breaks pnv_php). The >> + * other services also require some thinking about how we're going >> + * to integrate them. >> + */ >> + pcie_ports_disabled = true; >> +#endif >> + >> /* If we don't have OPAL, eg. in sim, just skip PCI probe */ >> if (!firmware_has_feature(FW_FEATURE_OPAL)) >> return; >>
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c index 2825d00..ae62583 100644 --- a/arch/powerpc/platforms/powernv/pci.c +++ b/arch/powerpc/platforms/powernv/pci.c @@ -941,6 +941,23 @@ void __init pnv_pci_init(void) pci_add_flags(PCI_CAN_SKIP_ISA_ALIGN); +#ifdef CONFIG_PCIEPORTBUS + /* + * On PowerNV PCIe devices are (currently) managed in cooperation + * with firmware. This isn't *strictly* required, but there's enough + * assumptions baked into both firmware and the platform code that + * it's unwise to allow the portbus services to be used. + * + * We need to fix this eventually, but for now set this flag to disable + * the portbus driver. The AER service isn't required since that AER + * events are handled via EEH. The pciehp hotplug driver can't work + * without kernel changes (and portbus binding breaks pnv_php). The + * other services also require some thinking about how we're going + * to integrate them. + */ + pcie_ports_disabled = true; +#endif + /* If we don't have OPAL, eg. in sim, just skip PCI probe */ if (!firmware_has_feature(FW_FEATURE_OPAL)) return;
On PowerNV the PCIe topology is (currently) managed the powernv platform code in cooperation with firmware. The PCIe-native service drivers bypass both and this can cause problems. Historically this hasn't been a big deal since the only port service driver that saw much use was the AER driver. The AER driver relies a kernel service to report when errors occur rather than acting autonmously so it's fairly easy to ignore. On PowerNV (and pseries) AER events are handled through EEH, which ignores the AER service, so it's never been an issue. Unfortunately, the hotplug port service driver (pciehp) does act autonomously and conflicts with the platform specific hotplug driver (pnv_php). The main issue is that pciehp claims the interrupt associated with the PCIe capability which in turn prevents pnv_php from claiming it. This results in hotplug events being handled by pciehp which does not notify firmware when the PCIe topology changes, and does not setup/teardown the arch specific PCI device structures (pci_dn) when the topology changes. The end result is that hot-added devices cannot be enabled and hot-removed devices may not be fully torn-down on removal. We can fix these problems by setting the "pcie_ports_disabled" flag during platform initialisation. The flag indicates the platform owns the PCIe ports which stops the portbus driver being registered. Cc: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> Fixes: 66725152fb9f ("PCI/hotplug: PowerPC PowerNV PCI hotplug driver") Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- Sergey, just FYI. I'll try sort out the rest of the hotplug trainwreck in 5.6. The Fixes: here is for the patch that added pnv_php in 4.8. It's been a problem since then, but wasn't noticed until people started testing it after the EEH fixes in commit 799abe283e51 ("powerpc/eeh: Clean up EEH PEs after recovery finishes") went in earlier in the 5.4 cycle. --- arch/powerpc/platforms/powernv/pci.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)