Hi Shawn,
On Tue, May 23, 2017 at 03:45:11PM +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@xxxxxxxxxxxx>
> Cc: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
>
> ---
>
> Changes in v3:
> - make ida_domain a system-wide property and check it in the code to
> combine with use_dt_domains. Also update the comment there.
>
> 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 | 42 ++++++++++++++++++++++++++++--------------
> drivers/pci/remove.c | 2 ++
> include/linux/pci.h | 8 ++++++--
> 3 files changed, 36 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b01bd5b..e5f5db0 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,15 +5341,25 @@ static void pci_no_domains(void)
> }
>
> #ifdef CONFIG_PCI_DOMAINS
> -static atomic_t __domain_nr = ATOMIC_INIT(-1);
> +DEFINE_IDA(__domain_nr);
Should be static.
> -int pci_get_new_domain_nr(void)
> +/* get domain number from IDA */
> +static bool ida_domain = true;
Reorder so "ida_domain" precedes the data structure it controls.
> +int pci_get_new_domain_nr(struct pci_bus *bus)
> +{
> + return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
> +}
> +
> +void pci_put_old_domain_nr(struct pci_bus *bus)
s/pci_put_old_domain_nr/pci_put_domain_nr/
> {
> - return atomic_inc_return(&__domain_nr);
> + if (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;
> @@ -5361,19 +5372,21 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> * If DT domain property is valid (domain >= 0) and
> * use_dt_domains != 0, the DT assignment is valid since this means
> * we have not previously allocated a domain number by using
> - * pci_get_new_domain_nr(); we should also update use_dt_domains to
> - * 1, to indicate that we have just assigned a domain number from
> - * DT.
> + * pci_get_new_domain_nr(), so we should set ida_domain to false to
> + * indicate that we don't allocate domain from idr; we should also
> + * update use_dt_domains to 1, to indicate that we have just assigned
> + * a domain number from DT.
> *
> * If DT domain property value is not valid (ie domain < 0), and we
> * have not previously assigned a domain number from DT
> - * (use_dt_domains != 1) we should assign a domain number by
> - * using the:
> + * (use_dt_domains != 1 && ida_domain != false) we should assign a
> + * domain number by using the:
> *
> * pci_get_new_domain_nr()
> *
> - * API and update the use_dt_domains value to keep track of method we
> - * are using to assign domain numbers (use_dt_domains = 0).
> + * API and update the use_dt_domains and ida_domain value to keep track
> + * of method we are using to assign domain numbers (use_dt_domains = 0
> + * and ida_domain = true).
> *
> * All other combinations imply we have a platform that is trying
> * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> @@ -5383,9 +5396,10 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
> */
> if (domain >= 0 && use_dt_domains) {
> use_dt_domains = 1;
> - } else if (domain < 0 && use_dt_domains != 1) {
> + ida_domain = false;
> + } else if (domain < 0 && use_dt_domains != 1 && ida_domain != false) {
> use_dt_domains = 0;
> - domain = pci_get_new_domain_nr();
> + domain = pci_get_new_domain_nr(bus);
These comments and logic are too complicated for my puny brain. Much of
this was pre-existing, of course. Surely there's a simpler way?
1) If we're using ACPI, every host bridge must have a _SEG method, and it
supplies the domain. We ignore any bridge without _SEG.
2) If we're using DT, every host bridge must supply "linux,pci-domain", and
it supplies the domain. We ignore any bridge without "linux,pci-domain".
3) Otherwise, we always use IDA.
> } else {
> dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> parent->of_node->full_name);
> @@ -5397,7 +5411,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);
In your current patch, ida_domain defaults to true, and it is only set
to false in of_pci_bus_find_domain_nr(), which is only called when
ACPI is disabled. Therefore, ida_domain will be true when we're using
ACPI, and I think pci_put_old_domain_nr() will erroneously remove
domains from tha IDA.
> pci_remove_bus(bus);
> host_bridge->bus = NULL;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33c2b0b..9296e31 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1445,13 +1445,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
>
>
