Hi Guilherme,
Some comments below ...
On Fri, 2016-18-03 at 21:49:06 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 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.
>
> Reviewed-by: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@xxxxxxxxxxxxxxxxxx>
> ---
> arch/powerpc/kernel/pci-common.c | 40 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 0f7a60f..bc31ac1 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
Did we just make that up? It seems like a lot, but then we have some big
systems?
> +/* For dynamic PHB numbering: used/free PHBs tracking bitmap. */
Locking? It looks like it's protected by the hose_spinlock, but you should say
that here, and also in the comment for hose_spinlock.
> +static DECLARE_BITMAP(phb_bitmap, MAX_PHBS);
>
> /* ISA Memory physical address */
> resource_size_t isa_mem_base;
> @@ -64,6 +67,32 @@ struct dma_map_ops *get_pci_dma_ops(void)
> }
> EXPORT_SYMBOL(get_pci_dma_ops);
>
There should be a comment here saying what the locking requirements are for
this function.
> +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)) {
Firstly I don't see why this check needs to be conditional on pseries. Any
machine where the PHB has a 'reg' property should be able to use 'reg' for
numbering.
> + regs = of_get_property(dn, "reg", NULL);
> + if (regs)
> + return (int)(be32_to_cpu(regs[1]) & 0xFFFF);
This should use of_property_read_u32().
> + } else if (machine_is(powernv)) {
This shouldn't be a machine check, it should just look for 'ibm,opal-phbid'
first, before 'reg'.
> + prop64 = of_get_property(dn, "ibm,opal-phbid", NULL);
> + if (prop64)
> + return (int)(be64_to_cpup(prop64) & 0xFFFF);
of_property_read_u64().
> + }
And finally in either case above, where you get a number from the device tree,
you must check that it's not already allocated. Otherwise if you have a system
where some PHBs have a property but others don't, you may give out the same
number twice. Also you could have firmware give you the same number twice
(which would be a firmware bug, but those happen).
If the number is allocated you fall back to dynamic numbering.
If it's not allocated you must mark it as allocated in the bitmap.
> +
> + /* 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;
> +}
> +
> struct pci_controller *pcibios_alloc_controller(struct device_node *dev)
> {
> struct pci_controller *phb;
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
