Re: [PATCH] powerpc/powernv: Disable native PCIe port management

[Bjorn Helgaas <helgaas@kernel.org>]

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
>