Re: [PATCH v5] PCI: use IDA to manage domain number if not getting it from DT

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, Aug 15, 2017 at 03:01:48PM +0800, Shawn Lin wrote:
> Hi Bjorn,
> 
> On 2017/8/12 5:17, Bjorn Helgaas wrote:
> >[+cc Lorenzo, resending because I fat-fingered the cc line and subject]
> >
> >On Tue, Jun 27, 2017 at 08:31:13AM +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.
> >>
> >>Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >
> >The "use_dt_domains" logic in of_pci_bus_find_domain_nr() is fairly
> >obtuse.  I *think*, now that we have pci_scan_root_bus_bridge() due to
> >Lorenzo's excellent work, the time is ripe for moving the domain
> >number from arch-specific places into struct pci_host_bridge.
> >
> >I suspect that will end up simplifying the CONFIG_PCI_DOMAINS vs
> >CONFIG_PCI_DOMAINS_GENERIC situation, and I wonder whether it might
> >enable some simplification of of_pci_bus_find_domain_nr() as well,
> >which in turn, might make *this* patch simpler.
> >
> >This isn't that big a patch to begin with, so I could apply it as-is
> >and we could do more domain cleanup later.  It's just that it's
> >intertwined with the PCI_DOMAINS #ifdefs and maybe there's an
> >opportunity to make this story more readable if those are out of the
> >way.  Any thoughts?
> 
> That sounds good to me that aftering add IDA domain, we could start
> considering moving domain number from arch-specific places into the
> bridge code and may be could also finally remove the macro
> CONFIG_PCI_DOMAIN* both?

I need to see how this can be implemented (another hook in
pci_host_bridge ?) but I suspect we can't get away with arch
specific bits - or maybe you are referring to having one single
place where the domain is _assigned_ using an arch specific hook
(in pci_host_bridge) ? I have to have a look into this, certainly
this patch should be considered because that atomic counter deserved
more thought, yes.

Thanks,
Lorenzo

> >>Changes in v5:
> >>- fix Bjorn's comments for v3 as actually I didn't address his comments
> >>   for v3 when posted my v4.
> >>   v3: https://patchwork.kernel.org/patch/9742003/
> >>   For ACPI case, we never use DT or IDA to get domain numbers and
> >>   acpi_pci_bus_find_domain_nr would return the proper value we need
> >>   whether _SEG exist or not. For DT or IDA, we hope bridges use one of
> >>   them consistently, and this is actually what the pre-existing code
> >>   did. So I remove ida_domain now since it complicated the logic of the
> >>   code and we could just make use_dt_domains global instead to slightly
> >>   achieve our purpose.
> >>
> >>Changes in v4:
> >>- make domain_nr depends on CONFIG_PCI_DOMAINS instead of
> >>CONFIG_PCI_DOMAINS_GENERIC.
> >>
> >>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    | 13 ++++++++++---
> >>  drivers/pci/remove.c |  2 ++
> >>  include/linux/pci.h  |  7 +++++--
> >>  3 files changed, 17 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >>index b58a6b3..9953eaf 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>
> >>@@ -5305,17 +5306,23 @@ static void pci_no_domains(void)
> >>  }
> >>  #ifdef CONFIG_PCI_DOMAINS
> >>-static atomic_t __domain_nr = ATOMIC_INIT(-1);
> >>+static DEFINE_IDA(__domain_nr);
> >>  int pci_get_new_domain_nr(void)
> >>  {
> >>-	return atomic_inc_return(&__domain_nr);
> >>+	return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
> >>+}
> >>+
> >>+static int use_dt_domains = -1;
> >>+void pci_put_domain_nr(struct pci_bus *bus)
> >>+{
> >>+	if (acpi_disabled && use_dt_domains != 1)
> >>+		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 use_dt_domains = -1;
> >>  	int domain = -1;
> >>  	if (parent)
> >>diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> >>index 73a03d3..01dd1b4 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_domain_nr(bus);
> >>  	pci_remove_bus(bus);
> >>  	host_bridge->bus = NULL;
> >>diff --git a/include/linux/pci.h b/include/linux/pci.h
> >>index 18cc70a..d6be9596 100644
> >>--- a/include/linux/pci.h
> >>+++ b/include/linux/pci.h
> >>@@ -523,7 +523,7 @@ struct pci_bus {
> >>  	unsigned char	primary;	/* number of primary bridge */
> >>  	unsigned char	max_bus_speed;	/* enum pci_bus_speed */
> >>  	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
> >>-#ifdef CONFIG_PCI_DOMAINS_GENERIC
> >>+#ifdef CONFIG_PCI_DOMAINS
> >>  	int		domain_nr;
> >>  #endif
> >>@@ -1466,11 +1466,14 @@ static inline int pci_enable_ptm(struct pci_dev *dev, u8 *granularity)
> >>  #ifdef CONFIG_PCI_DOMAINS
> >>  extern int pci_domains_supported;
> >>  int pci_get_new_domain_nr(void);
> >>+void pci_put_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(void)
> >>+{ return -ENOSYS; }
> >>+static inline void pci_put_domain_nr(struct pci_bus *bus) { }
> >>  #endif /* CONFIG_PCI_DOMAINS */
> >>  /*
> >>-- 
> >>1.9.1
> >>
> >>
> >
> >
> >
>