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

[Guilherme G. Piccoli]

On 05/25/2016 03:26 AM, Michael Ellerman wrote:
> On Wed, 2016-18-05 at 01:48:00 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 at linux.vnet.ibm.com>
>> Reviewed-by: Gavin Shan <gwshan at linux.vnet.ibm.com>
>> Reviewed-by: Ian Munsie <imunsie at au1.ibm.com>
>> ---
>> v6:
>>      * Dropped the of_get_property() use to use instead
>> of_property_read_u64()/of_property_read_u32_array as per Michael's
>> suggestion. Since these 2 functions deal with endianness conversion,
>> no need to manually convert endianness anymore. Logic was simplified.
>
> Thanks.
>
>>      * Changed MAX_PHBS to 64K as per Gavin's suggestion. Changed 0xFFFF
>> to (MAX_PHBS - 1) then.
>
> Thanks.
>

Thanks very much for your review and comments Michael.


>>      * Changed test_bit() and set_bit() to test_and_set_bit() as per
>> Gavin's suggestion.
>>
>>      * Removed some obvious comments.
>>
>>      * 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.
>
> I'll drop it and test here on Cell & others.
>

OK, glad to hear this. I have no access to Cell and other platforms, 
thanks for taking time to test this :)


>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>> index 0f7a60f..4afc9c1 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,47 @@ 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;
>> +	int phb_id;
>> +
>> +	/*
>> +	 * 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_array(dn, "reg", (u32 *)&prop, 2);
>> +	if (ret)
>> +		goto dynamic_phb_numbering;
>> +
>> +	phb_id = (int)(prop & (MAX_PHBS - 1));
>> +
>> +	/* We need to be sure to not use the same PHB number twice. */
>> +	if (test_and_set_bit(phb_id, phb_bitmap))
>> +		goto dynamic_phb_numbering;
>> +
>> +	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.
>> +	 */
>> +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;
>> +}
>
> You really shouldn't need a goto in a function this simple.
>
> Either rearrange it so you can do the logic once, or just put the dynamic case
> into a helper function.

Sure, I'll rearrange the code to drop this goto, thanks for the suggestion.

Cheers,


Guilherme



>
> cheers
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>