Hi Bjorn,
? 2017/5/23 5:18, Bjorn Helgaas ??:
> On Thu, May 18, 2017 at 09:47:17AM +0800, Shawn Lin wrote:
>> If not getting domain number from DT, the domain number will
>> keep increasing once doing unbind/bind RC drivers. This could
>> introduce pointless tree view of lspci as shows below:
>>
>> -+-[0001:00]---00.0-[01]----00.0
>> \-[0000:00]-
>>
>> The more test we do, the lengthier it would be. The more serious
>> issue is that if attaching two hierarchies for two different domains
>> belonging to two root bridges, so when doing unbind/bind test for one
>> of them and keep the other, then the domain number would finally
>> overflow and make the two hierarchies of devices share the some domain
>> number but actually they shouldn't. So it looks like we need to invent
>> a new indexing ID mechanism to manage domain number. This patch
>> introduces idr to achieve our purpose.
>>
>> Cc: Brian Norris <briannorris at chromium.org>
>> Cc: Jeffy Chen <jeffy.chen at rock-chips.com>
>> Signed-off-by: Shawn Lin <shawn.lin at rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - add a remove wrapper
>> - rename use_dt_domains to ida_domain and set this bit
>> in pci_get_new_domain_nr and test it in the remove wrapper.
>>
>> drivers/pci/pci.c | 23 +++++++++++++++++------
>> drivers/pci/remove.c | 2 ++
>> include/linux/pci.h | 9 +++++++--
>> 3 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b01bd5b..8affda5 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -11,6 +11,7 @@
>> #include <linux/kernel.h>
>> #include <linux/delay.h>
>> #include <linux/dmi.h>
>> +#include <linux/idr.h>
>> #include <linux/init.h>
>> #include <linux/of.h>
>> #include <linux/of_pci.h>
>> @@ -5340,19 +5341,29 @@ static void pci_no_domains(void)
>> }
>>
>> #ifdef CONFIG_PCI_DOMAINS
>> -static atomic_t __domain_nr = ATOMIC_INIT(-1);
>> +DEFINE_IDA(__domain_nr);
>>
>> -int pci_get_new_domain_nr(void)
>> +int pci_get_new_domain_nr(struct pci_bus *bus)
>> {
>> - return atomic_inc_return(&__domain_nr);
>> + bus->ida_domain = true;
>> + return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
>> +}
>> +
>> +void pci_put_old_domain_nr(struct pci_bus *bus)
>> +{
>> + if (bus->ida_domain)
>> + ida_simple_remove(&__domain_nr, bus->domain_nr);
>> }
>>
>> #ifdef CONFIG_PCI_DOMAINS_GENERIC
>> -static int of_pci_bus_find_domain_nr(struct device *parent)
>> +static int of_pci_bus_find_domain_nr(struct pci_bus *bus,
>> + struct device *parent)
>> {
>> static int use_dt_domains = -1;
>> int domain = -1;
>>
>> + bus->ida_domain = false;
>> +
>> if (parent)
>> domain = of_get_pci_domain_nr(parent->of_node);
>> /*
>> @@ -5385,7 +5396,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
>> use_dt_domains = 1;
>> } else if (domain < 0 && use_dt_domains != 1) {
>> use_dt_domains = 0;
>> - domain = pci_get_new_domain_nr();
>> + domain = pci_get_new_domain_nr(bus);
>> } else {
>> dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
>> parent->of_node->full_name);
>> @@ -5397,7 +5408,7 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
>>
>> int pci_bus_find_domain_nr(struct pci_bus *bus, struct device *parent)
>> {
>> - return acpi_disabled ? of_pci_bus_find_domain_nr(parent) :
>> + return acpi_disabled ? of_pci_bus_find_domain_nr(bus, parent) :
>> acpi_pci_bus_find_domain_nr(bus);
>> }
>> #endif
>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
>> index 73a03d3..1bbbb37 100644
>> --- a/drivers/pci/remove.c
>> +++ b/drivers/pci/remove.c
>> @@ -157,6 +157,8 @@ void pci_remove_root_bus(struct pci_bus *bus)
>> list_for_each_entry_safe(child, tmp,
>> &bus->devices, bus_list)
>> pci_remove_bus_device(child);
>> +
>> + pci_put_old_domain_nr(bus);
>> pci_remove_bus(bus);
>> host_bridge->bus = NULL;
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 33c2b0b..d4f6fb8 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -513,6 +513,7 @@ struct pci_bus {
>> unsigned char cur_bus_speed; /* enum pci_bus_speed */
>> #ifdef CONFIG_PCI_DOMAINS_GENERIC
>> int domain_nr;
>> + bool ida_domain; /* get domain number from IDA */
>
> The fact that ida_domain is a member of struct pci_bus suggests that
> it can be a per-bus property. However, I think it is actually a
> system-wide property. If we get the domain from DT for some buses but
> via IDA for other buses, there's no mechanism to prevent collisions,
> is there?
yes, you are right.
>
> If it's a system-wide property, I'd rather have a single global
> variable for it because that makes it more obvious to the reader that
> we don't have to worry about collisions between a DT domain number and
> one from IDA.
ok, will respin v3 to address this.
Thanks.
>
>> #endif
>>
>> char name[48];
>> @@ -1445,13 +1446,17 @@ static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
>> * configuration space.
>> */
>> #ifdef CONFIG_PCI_DOMAINS
>> +extern struct ida __domain_nr;
>> extern int pci_domains_supported;
>> -int pci_get_new_domain_nr(void);
>> +int pci_get_new_domain_nr(struct pci_bus *bus);
>> +void pci_put_old_domain_nr(struct pci_bus *bus);
>> #else
>> enum { pci_domains_supported = 0 };
>> static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
>> static inline int pci_proc_domain(struct pci_bus *bus) { return 0; }
>> -static inline int pci_get_new_domain_nr(void) { return -ENOSYS; }
>> +static inline int pci_get_new_domain_nr(struct pci_bus *bus)
>> +{ return -ENOSYS; }
>> +static inline void pci_put_old_domain_nr(struct pci_bus *bus) { }
>> #endif /* CONFIG_PCI_DOMAINS */
>>
>> /*
>> --
>> 1.9.1
>>
>>
>
>
>
