Re: [PATCH RFC v4 20/21] PCI: pciehp: Add support for the movable BARs feature

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Mar 11, 2019 at 04:31:21PM +0300, Sergey Miroshnichenko wrote:
> With movable BARs, adding a hotplugged device may affect all the PCIe
> domain starting from the root, so use a pci_rescan_bus() function which
> handles the rearrangement of existing BARs and bridge windows.
> 
> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com>
> ---
>  drivers/pci/hotplug/pciehp_pci.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index b9c1396db6fe..7c0871db5bae 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -56,12 +56,16 @@ int pciehp_configure_device(struct controller *ctrl)
>  		goto out;
>  	}
>  
> -	for_each_pci_bridge(dev, parent)
> -		pci_hp_add_bridge(dev);
> +	if (pci_movable_bars_enabled()) {
> +		pci_rescan_bus(parent);
> +	} else {
> +		for_each_pci_bridge(dev, parent)
> +			pci_hp_add_bridge(dev);
>  
> -	pci_assign_unassigned_bridge_resources(bridge);
> -	pcie_bus_configure_settings(parent);
> -	pci_bus_add_devices(parent);
> +		pci_assign_unassigned_bridge_resources(bridge);
> +		pcie_bus_configure_settings(parent);
> +		pci_bus_add_devices(parent);
> +	}

The addition of a second path at this level, i.e., different paths
depending on whether movable BARs are enabled, seems a little
problematic because it's hard to determine whether they're equivalent
except for the movable BAR aspect.  For example, you don't call
pci_hp_add_bridge() when movable BARs are enabled, and I can't tell
whether that's intentional or whether it's a problem.

This looks like the sort of change that should be made in other
hotplug paths, e.g., enable_slot() for acpiphp,
pcibios_finish_adding_to_bus() for powerpc (maybe? I can't really
tell), cpci_configure_slot() shpchp_configure_device()?

If we have or could invent some top-level interface that all these
places could use, and somewhere inside that we could do the movable
BAR magic, I think that would make it more maintainable.

>   out:
>  	pci_unlock_rescan_remove();
> -- 
> 2.20.1
>