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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Wed, Jun 29, 2016 at 03:14:22PM -0300, 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@linux.vnet.ibm.com>
>Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Reviewed-by: Ian Munsie <imunsie@au1.ibm.com>

Guilherme, thanks for keeping improving it. If Michael needs:

Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

>---
>v7:
>    * Removed the goto as per Michael's suggestion;
>
>    * Changed of_property_read_u32_array() to of_property_read_u32_index(),
>as per Gavin's suggestion. This way, we end up using buid_low as the index
>of PHB in pSeries, which is expected but was not being achieved in v6,
>as per my mistake.
>
>    * 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.
>
> arch/powerpc/kernel/pci-common.c | 53 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 3 deletions(-)
>
>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>index 0f7a60f..c87545b 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,41 @@ 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, phb_id = -1;
>+
>+	/*
>+	 * 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_index(dn, "reg", 1, (u32 *)&prop);
>+	if (!ret)
>+		phb_id = (int)(prop & (MAX_PHBS - 1));
>+
>+	/* We need to be sure to not use the same PHB number twice. */
>+	if ((phb_id >= 0) && !test_and_set_bit(phb_id, phb_bitmap))
>+		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.
>+	 */
>+	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;
>+}
>+
> struct pci_controller *pcibios_alloc_controller(struct device_node *dev)
> {
> 	struct pci_controller *phb;
>@@ -72,7 +114,7 @@ struct pci_controller *pcibios_alloc_controller(struct device_node *dev)
> 	if (phb == NULL)
> 		return NULL;
> 	spin_lock(&hose_spinlock);
>-	phb->global_number = global_phb_number++;
>+	phb->global_number = get_phb_number(dev);
> 	list_add_tail(&phb->list_node, &hose_list);
> 	spin_unlock(&hose_spinlock);
> 	phb->dn = dev;
>@@ -94,6 +136,11 @@ EXPORT_SYMBOL_GPL(pcibios_alloc_controller);
> void pcibios_free_controller(struct pci_controller *phb)
> {
> 	spin_lock(&hose_spinlock);
>+
>+	/* Clear bit of phb_bitmap to allow reuse of this PHB number. */
>+	if (phb->global_number < MAX_PHBS)
>+		clear_bit(phb->global_number, phb_bitmap);
>+
> 	list_del(&phb->list_node);
> 	spin_unlock(&hose_spinlock);
>
>-- 
>2.1.0
>