| Message ID | 1467224062-9173-1-git-send-email-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 >
On Wed, 2016-29-06 at 18:14:22 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@linux.vnet.ibm.com> > Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > Reviewed-by: Ian Munsie <imunsie@au1.ibm.com> > Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/63a72284b159c569ec52f380c9 cheers
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);