diff mbox

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

Message ID 1495525511-202465-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Lin May 23, 2017, 7:45 a.m. UTC
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>

---

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(-)

Comments

kernel test robot May 24, 2017, 4:32 a.m. UTC | #1
Hi Shawn,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Shawn-Lin/PCI-use-IDA-to-manage-domain-number-if-not-getting-it-from-DT/20170524-095618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/pci/pci.c: In function 'pci_put_old_domain_nr':
>> drivers/pci/pci.c:5357:38: error: 'struct pci_bus' has no member named 'domain_nr'
      ida_simple_remove(&__domain_nr, bus->domain_nr);
                                         ^~

vim +5357 drivers/pci/pci.c

  5351		return ida_simple_get(&__domain_nr, 0, sizeof(u64), GFP_KERNEL);
  5352	}
  5353	
  5354	void pci_put_old_domain_nr(struct pci_bus *bus)
  5355	{
  5356		if (ida_domain)
> 5357			ida_simple_remove(&__domain_nr, bus->domain_nr);
  5358	}
  5359	
  5360	#ifdef CONFIG_PCI_DOMAINS_GENERIC

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Bjorn Helgaas May 24, 2017, 10:30 p.m. UTC | #2
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@chromium.org>
> Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---
> 
> 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
> 
>
Shawn Lin May 25, 2017, 12:38 a.m. UTC | #3
Hi Bjorn,

On 2017/5/25 6:30, Bjorn Helgaas wrote:
> Hi Shawn,
>
> On Tue, May 23, 2017 at 03:45:11PM +0800, Shawn Lin wrote:

[...]

>> +		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?

yup, I think we could simplify the code.

>
> 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.

I plan to create a new function to allocate domain number using IDA as
then we don't touch the logic of of_pci_bus_find_domain_nr, and the
name of of_pci_bus_find_domain_nr explicitly say that the domain number
comes from DT.

Then I could put the condition check inside the pci_bus_find_domain_nr
as your suggested.


Does it looks ok to you?

>
>>  	} else {
>>  		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",

[...]

>>  		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.
>

woops, I forgot to check the ACPI cases.

>>  	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
>>
>>
>
>
>
diff mbox

Patch

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);
 
-int pci_get_new_domain_nr(void)
+/* get domain number from IDA */
+static bool ida_domain = true;
+
+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)
 {
-	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);
 	} 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);
 	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 */
 
 /*