diff mbox series

powerpc/powernv: Disable native PCIe port management

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

Commit Message

Oliver O'Halloran Nov. 13, 2019, 9:40 a.m. UTC
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(+)

Comments

Bjorn Helgaas Nov. 13, 2019, 2:31 p.m. UTC | #1
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
>
Tyrel Datwyler Nov. 13, 2019, 8:39 p.m. UTC | #2
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;
>
Oliver O'Halloran Nov. 14, 2019, 1:34 p.m. UTC | #3
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.
Oliver O'Halloran Nov. 14, 2019, 1:37 p.m. UTC | #4
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.
Bjorn Helgaas Nov. 14, 2019, 1:54 p.m. UTC | #5
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
Michael Ellerman Nov. 15, 2019, 4:28 a.m. UTC | #6
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 mbox series

Patch

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;