[v6] powerpc/pci: Assign fixed PHB number based on device-tree properties

[Michael Ellerman]

On Wed, 2016-18-05 at 01:48:00 UTC, "Guilherme G. Piccoli" wrote:
> The domain/PHB field of PCI addresses has its value obtained from a
> global variable, incremented each time a new domain (represented by
> struct pci_controller) is added on the system. The domain addition
> process happens during boot or due to PHB hotplug add.
> 
> As recent kernels are using predictable naming for network interfaces,
> the network stack is more tied to PCI naming. This can be a problem in
> hotplug scenarios, because PCI addresses will change if devices are
> removed and then re-added. This situation seems unusual, but it can
> happen if a user wants to replace a NIC without rebooting the machine,
> for example.
> 
> This patch changes the way PCI domain values are generated: now, we use
> device-tree properties to assign fixed PHB numbers to PCI addresses
> when available (meaning pSeries and PowerNV cases). We also use a bitmap
> to allow dynamic PHB numbering when device-tree properties are not
> used. This bitmap keeps track of used PHB numbers and if a PHB is
> released (by hotplug operations for example), it allows the reuse of
> this PHB number, avoiding PCI address to change in case of device remove
> and re-add soon after. No functional changes were introduced.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
> Reviewed-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
> Reviewed-by: Ian Munsie <imunsie at au1.ibm.com>
> ---
> v6:
>     * Dropped the of_get_property() use to use instead
> of_property_read_u64()/of_property_read_u32_array as per Michael's
> suggestion. Since these 2 functions deal with endianness conversion,
> no need to manually convert endianness anymore. Logic was simplified.

Thanks.

>     * Changed MAX_PHBS to 64K as per Gavin's suggestion. Changed 0xFFFF
> to (MAX_PHBS - 1) then.

Thanks.

>     * Changed test_bit() and set_bit() to test_and_set_bit() as per
> Gavin's suggestion.
> 
>     * Removed some obvious comments.
> 
>     * Didn't remove machine check for pSeries on "reg" property lookup.
> It's worthy to keep it, since almost every platform (if not all of them)
> contain the "reg" property on PHB node in device-tree, but only in
> pSeries we're 100% sure it can be used as the PHB unique identifier.
> Since the patch has a dynamic PHB numbering mechanism, the other platforms
> won't have trouble with it.

I'll drop it and test here on Cell & others.

> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 0f7a60f..4afc9c1 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -41,11 +41,18 @@
>  #include <asm/ppc-pci.h>
>  #include <asm/eeh.h>
>  
> +/* hose_spinlock protects accesses to the the phb_bitmap. */
>  static DEFINE_SPINLOCK(hose_spinlock);
>  LIST_HEAD(hose_list);
>  
> -/* XXX kill that some day ... */
> -static int global_phb_number;		/* Global phb counter */
> +/* For dynamic PHB numbering on get_phb_number(): max number of PHBs. */
> +#define MAX_PHBS 0x10000
> +
> +/*
> + * For dynamic PHB numbering: used/free PHBs tracking bitmap.
> + * Accesses to this bitmap should be protected by hose_spinlock.
> + */
> +static DECLARE_BITMAP(phb_bitmap, MAX_PHBS);
>  
>  /* ISA Memory physical address */
>  resource_size_t isa_mem_base;
> @@ -64,6 +71,47 @@ struct dma_map_ops *get_pci_dma_ops(void)
>  }
>  EXPORT_SYMBOL(get_pci_dma_ops);
>  
> +/*
> + * This function should run under locking protection, specifically
> + * hose_spinlock.
> + */
> +static int get_phb_number(struct device_node *dn)
> +{
> +	u64 prop;
> +	int ret;
> +	int phb_id;
> +
> +	/*
> +	 * Try fixed PHB numbering first, by checking archs and reading
> +	 * the respective device-tree properties. Firstly, try PowerNV by
> +	 * reading "ibm,opal-phbid", only present in OPAL environment.
> +	 */
> +	ret = of_property_read_u64(dn, "ibm,opal-phbid", &prop);
> +	if (ret && machine_is(pseries))
> +		ret = of_property_read_u32_array(dn, "reg", (u32 *)&prop, 2);
> +	if (ret)
> +		goto dynamic_phb_numbering;
> +
> +	phb_id = (int)(prop & (MAX_PHBS - 1));
> +
> +	/* We need to be sure to not use the same PHB number twice. */
> +	if (test_and_set_bit(phb_id, phb_bitmap))
> +		goto dynamic_phb_numbering;
> +
> +	return phb_id;
> +
> +	/*
> +	 * If not pSeries nor PowerNV, or if fixed PHB numbering tried to add
> +	 * the same PHB number twice, then fallback to dynamic PHB numbering.
> +	 */
> +dynamic_phb_numbering:
> +	phb_id = find_first_zero_bit(phb_bitmap, MAX_PHBS);
> +	BUG_ON(phb_id >= MAX_PHBS);
> +	set_bit(phb_id, phb_bitmap);
> +
> +	return phb_id;
> +}

You really shouldn't need a goto in a function this simple.

Either rearrange it so you can do the logic once, or just put the dynamic case
into a helper function.

cheers