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@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Ian Munsie <imunsie@xxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
