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

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

On Tue, Mar 15, 2016 at 07:54:41PM -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 PCI device hotplug.
>
>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>
>---
> arch/powerpc/kernel/pci-common.c | 43 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 40 insertions(+), 3 deletions(-)
>
>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>index 0f7a60f..d4217f9 100644
>--- a/arch/powerpc/kernel/pci-common.c
>+++ b/arch/powerpc/kernel/pci-common.c
>@@ -44,8 +44,11 @@
> 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	8192
>+
>+/* For dynamic PHB numbering: used/free PHBs tracking bitmap. */
>+DECLARE_BITMAP(global_phb_bitmap, MAX_PHBS);
>

I guess it can be file-scoped variable.

> /* ISA Memory physical address */
> resource_size_t isa_mem_base;
>@@ -64,6 +67,39 @@ struct dma_map_ops *get_pci_dma_ops(void)
> }
> EXPORT_SYMBOL(get_pci_dma_ops);
>
>+static int get_phb_number(struct device_node *dn)
>+{
>+	const __be64 *prop64;
>+	const __be32 *regs;
>+	int phb_id = 0;
>+
>+	/* try fixed PHB numbering first */
>+	if (machine_is(pseries)) {
>+		regs = of_get_property(dn, "reg", NULL);
>+		if (regs)
>+			return (int)(be32_to_cpu(regs[1]) & 0xFFFF);
>+	}
>+
>+	else { /* maybe PowerNV? */
>+		prop64 = of_get_property(dn, "ibm,opal-phbid", NULL);
>+		if (prop64)
>+			return (int)(be64_to_cpup(prop64) & 0xFFFF);
>+	}
>+

We usually have something like below. Also, why not checking if it's
PowerNV platform by machine_is(powernv)? Besides, I guess the comments
here can be more indicative:

	if (...) {

	} else ( ...) {

	}

>+	/* dynamic PHB numbering mechanism */
>+	while ((phb_id < MAX_PHBS) && test_bit(phb_id, global_phb_bitmap))
>+		phb_id++;
>+
>+	BUG_ON(phb_id >= MAX_PHBS); /* reached maximum number of PHBs */
>+	set_bit(phb_id, global_phb_bitmap);
>+	return phb_id;

It could be simplified to:

        do {
		phb_id = find_next_zero_bit(global_phb_bitmap, MAX_PHBS, 0);
                BUG_ON(phb_id >= MAX_PHBS);
        } while(test_and_set_bit(phb_id, global_phb_bitmap));

	return phb_id;
>+}
>+
>+static void release_phb_number(int phb_id)
>+{
>+	clear_bit(phb_id, global_phb_bitmap);
>+}
>+

The logic of this function can be merged to pcibos_free_controller() as it's
just one statement and it's called by pcibios_free_controller() only. Also,
it will cause memory corruption if (phb_id >= MAX_PHBS) on pSeries/PowerNV
though it's rare.

> struct pci_controller *pcibios_alloc_controller(struct device_node *dev)
> {
> 	struct pci_controller *phb;
>@@ -72,7 +108,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 +130,7 @@ EXPORT_SYMBOL_GPL(pcibios_alloc_controller);
> void pcibios_free_controller(struct pci_controller *phb)
> {
> 	spin_lock(&hose_spinlock);
>+	release_phb_number(phb->global_number);
> 	list_del(&phb->list_node);
> 	spin_unlock(&hose_spinlock);
>

Thanks,
Gavin