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

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

On Fri, Mar 18, 2016 at 09:37:57AM +1100, Gavin Shan wrote:
>On Thu, Mar 17, 2016 at 05:03:46PM -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>
>
>Apart from below minor issues to be fixed, it looks good to me.
>
>Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>>---
>> arch/powerpc/kernel/pci-common.c | 41 +++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 38 insertions(+), 3 deletions(-)
>>
>>diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>>index 0f7a60f..8777614 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. */
>>+static DECLARE_BITMAP(phb_bitmap, MAX_PHBS);
>>
>> /* ISA Memory physical address */
>> resource_size_t isa_mem_base;
>>@@ -64,6 +67,33 @@ 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, by checking archs and reading
>>+	 * the respective device-tree property. */
>>+	if (machine_is(pseries)) {
>>+		regs = of_get_property(dn, "reg", NULL);
>>+		if (regs)
>>+			return (int)(be32_to_cpu(regs[1]) & 0xFFFF);
>>+	} else {
>>+		if (machine_is(powernv)) {
>>+			prop64 = of_get_property(dn, "ibm,opal-phbid", NULL);
>>+			if (prop64)
>>+				return (int)(be64_to_cpup(prop64) & 0xFFFF);
>>+		}
>>+	}
>>+
>
>The nested statements can be merged to one with "else if (machine_is(powernv))".
>
>>+	/* if not pSeries nor PowerNV, fallback to dynamic PHB numbering */
>>+	phb_id = find_first_zero_bit(phb_bitmap, MAX_PHBS);
>>+	BUG_ON(phb_id >= MAX_PHBS); /* reached maximum number of PHBs */
>>+	set_bit(phb_id, phb_bitmap);
>>+	return phb_id;
>
>It's possible another CPU sets this bit before current CPU updates it, which
>will cause same domain number used by two PHBs though it's very rare. I guess
>it might be worthwhile to check if the bit is reserved by somebody else to
>avoid the issue.
>

Please ignore this comment: there is a lock (hose_spinlock) avoiding the issue.

>>+}
>>+
>> struct pci_controller *pcibios_alloc_controller(struct device_node *dev)
>> {
>> 	struct pci_controller *phb;
>>@@ -72,7 +102,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 +124,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);
>>
>
>Thanks,
>Gavin