PCI: use IDA to manage domain number if not getting it from DT

Message ID 1490175744-22727-1-git-send-email-shawn.lin@rock-chips.com
State Changes Requested
Headers show

Commit Message

Shawn Lin March 22, 2017, 9:42 a.m.
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@chromium.org>
Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/pci.c    | 11 +++++++----
 drivers/pci/remove.c |  5 +++++
 include/linux/pci.h  |  2 ++
 3 files changed, 14 insertions(+), 4 deletions(-)

Comments

Shawn Lin May 2, 2017, 6:55 a.m. | #1
Hi Bjorn,

Any thought on this patch? :)

On 2017/3/22 17:42, 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@chromium.org>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
>  drivers/pci/pci.c    | 11 +++++++----
>  drivers/pci/remove.c |  5 +++++
>  include/linux/pci.h  |  2 ++
>  3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a881c0d..1752ccc 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>
> @@ -5172,15 +5173,16 @@ 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)
>  {
> -	return atomic_inc_return(&__domain_nr);
> +	return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
>  }
>
>  #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;
> @@ -5224,12 +5226,13 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
>  		domain = -1;
>  	}
>
> +	bus->use_dt_domains = use_dt_domains;
>  	return domain;
>  }
>
>  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..ad93378 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -157,6 +157,11 @@ 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);
> +	#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +	if (!bus->use_dt_domains)
> +		ida_simple_remove(&__domain_nr, bus->domain_nr);
> +	#endif
> +
>  	pci_remove_bus(bus);
>  	host_bridge->bus = NULL;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6732d32..235b336 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -505,6 +505,7 @@ struct pci_bus {
>  	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	int		domain_nr;
> +	int		use_dt_domains;
>  #endif
>
>  	char		name[48];
> @@ -1449,6 +1450,7 @@ 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);
>  #else
>
Bjorn Helgaas May 17, 2017, 7:02 p.m. | #2
Hi Shawn,

On Wed, Mar 22, 2017 at 05:42:24PM +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.

I like the idea of this patch, but I have some comments/suggestions
about the implementation.

> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/pci/pci.c    | 11 +++++++----
>  drivers/pci/remove.c |  5 +++++
>  include/linux/pci.h  |  2 ++
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a881c0d..1752ccc 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>
> @@ -5172,15 +5173,16 @@ 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)
>  {
> -	return atomic_inc_return(&__domain_nr);
> +	return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
>  }
>  
>  #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;
> @@ -5224,12 +5226,13 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
>  		domain = -1;
>  	}
>  
> +	bus->use_dt_domains = use_dt_domains;
>  	return domain;
>  }
>  
>  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..ad93378 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -157,6 +157,11 @@ 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);
> +	#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +	if (!bus->use_dt_domains)
> +		ida_simple_remove(&__domain_nr, bus->domain_nr);
> +	#endif

1) I think there should be a wrapper alongside pci_get_new_domain_nr()
that does the remove.  That would remove the #ifdef here and help make
the remove symmetric with the get.

2) The "use_dt_domains" name is too specific and makes too many
assumptions about CONFIG_PCI_DOMAINS_GENERIC, e.g., it assumes that we
don't use CONFIG_PCI_DOMAINS_GENERIC with ACPI.

At a high level, if DT or ACPI supplies a domain number, we use that.
We only use the IDA if neither supplies a domain number.  I think it
would be clearer to set a bit in pci_get_new_domain_nr() that means
"we got this domain from IDA", and then test that bit in the
corresponding remove wrapper.

>  	pci_remove_bus(bus);
>  	host_bridge->bus = NULL;
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6732d32..235b336 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -505,6 +505,7 @@ struct pci_bus {
>  	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
>  	int		domain_nr;
> +	int		use_dt_domains;
>  #endif
>  
>  	char		name[48];
> @@ -1449,6 +1450,7 @@ 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);
>  #else
> -- 
> 1.9.1
> 
>

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a881c0d..1752ccc 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>
@@ -5172,15 +5173,16 @@  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)
 {
-	return atomic_inc_return(&__domain_nr);
+	return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
 }
 
 #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;
@@ -5224,12 +5226,13 @@  static int of_pci_bus_find_domain_nr(struct device *parent)
 		domain = -1;
 	}
 
+	bus->use_dt_domains = use_dt_domains;
 	return domain;
 }
 
 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..ad93378 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -157,6 +157,11 @@  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);
+	#ifdef CONFIG_PCI_DOMAINS_GENERIC
+	if (!bus->use_dt_domains)
+		ida_simple_remove(&__domain_nr, bus->domain_nr);
+	#endif
+
 	pci_remove_bus(bus);
 	host_bridge->bus = NULL;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6732d32..235b336 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -505,6 +505,7 @@  struct pci_bus {
 	unsigned char	cur_bus_speed;	/* enum pci_bus_speed */
 #ifdef CONFIG_PCI_DOMAINS_GENERIC
 	int		domain_nr;
+	int		use_dt_domains;
 #endif
 
 	char		name[48];
@@ -1449,6 +1450,7 @@  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);
 #else