[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

The current logic used for PCI domain assignment in arm64
pci_bus_assign_domain_nr() is flawed in that, depending on the host
controllers configuration for a platform and the respective initialization
sequence, core code may end up allocating PCI domain numbers from both DT and
the core code generic domain counter, which would result in PCI domain
allocation aliases/errors.

This patch fixes the logic behind the PCI domain number assignment and
moves the resulting code to generic PCI core code so that the same domain
allocation logic is used on all platforms selecting

CONFIG_PCI_DOMAINS_GENERIC

instead of resorting to an arch specific implementation that might end up
duplicating the PCI domain assignment logic wrongly.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
v1 => v2:

- Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
  adding an OF layer API
- Updated commit log and code comments

 arch/arm64/kernel/pci.c | 22 ----------------------
 drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..6f93c24 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
 
 	return 0;
 }
-
-
-#ifdef CONFIG_PCI_DOMAINS_GENERIC
-static bool dt_domain_found = false;
-
-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
-{
-	int domain = of_get_pci_domain_nr(parent->of_node);
-
-	if (domain >= 0) {
-		dt_domain_found = true;
-	} else if (dt_domain_found == true) {
-		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
-			parent->of_node->full_name);
-		return;
-	} else {
-		domain = pci_get_new_domain_nr();
-	}
-
-	bus->domain_nr = domain;
-}
-#endif
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 625a4ac..2279414 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -10,6 +10,8 @@
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
@@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
 {
 	return atomic_inc_return(&__domain_nr);
 }
+
+#ifdef CONFIG_PCI_DOMAINS_GENERIC
+
+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+{
+	static int use_dt_domains = -1;
+	int domain = of_get_pci_domain_nr(parent->of_node);
+
+	/*
+	 * Check DT domain and use_dt_domains values.
+	 *
+	 * 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.
+	 *
+	 * 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:
+	 *
+	 * 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).
+	 *
+	 * All other combinations imply we have a platform that is trying
+	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
+	 * which is a recipe for domain mishandling and it is prevented by
+	 * invalidating the domain value (domain = -1) and printing a
+	 * corresponding error.
+	 */
+	if (domain >= 0 && use_dt_domains) {
+		use_dt_domains = 1;
+	} else if (domain < 0 && use_dt_domains != 1) {
+		use_dt_domains = 0;
+		domain = pci_get_new_domain_nr();
+	} else {
+		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
+			parent->of_node->full_name);
+		domain = -1;
+	}
+
+	bus->domain_nr = domain;
+}
+#endif
 #endif
 
 /**
-- 
2.1.2

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

The current logic used for PCI domain assignment in arm64
pci_bus_assign_domain_nr() is flawed in that, depending on the host
controllers configuration for a platform and the respective initialization
sequence, core code may end up allocating PCI domain numbers from both DT and
the core code generic domain counter, which would result in PCI domain
allocation aliases/errors.

This patch fixes the logic behind the PCI domain number assignment and
moves the resulting code to generic PCI core code so that the same domain
allocation logic is used on all platforms selecting

CONFIG_PCI_DOMAINS_GENERIC

instead of resorting to an arch specific implementation that might end up
duplicating the PCI domain assignment logic wrongly.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
v1 => v2:

- Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
  adding an OF layer API
- Updated commit log and code comments

 arch/arm64/kernel/pci.c | 22 ----------------------
 drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..6f93c24 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
 
 	return 0;
 }
-
-
-#ifdef CONFIG_PCI_DOMAINS_GENERIC
-static bool dt_domain_found = false;
-
-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
-{
-	int domain = of_get_pci_domain_nr(parent->of_node);
-
-	if (domain >= 0) {
-		dt_domain_found = true;
-	} else if (dt_domain_found == true) {
-		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
-			parent->of_node->full_name);
-		return;
-	} else {
-		domain = pci_get_new_domain_nr();
-	}
-
-	bus->domain_nr = domain;
-}
-#endif
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 625a4ac..2279414 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -10,6 +10,8 @@
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
@@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
 {
 	return atomic_inc_return(&__domain_nr);
 }
+
+#ifdef CONFIG_PCI_DOMAINS_GENERIC
+
+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
+{
+	static int use_dt_domains = -1;
+	int domain = of_get_pci_domain_nr(parent->of_node);
+
+	/*
+	 * Check DT domain and use_dt_domains values.
+	 *
+	 * 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.
+	 *
+	 * 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:
+	 *
+	 * 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).
+	 *
+	 * All other combinations imply we have a platform that is trying
+	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
+	 * which is a recipe for domain mishandling and it is prevented by
+	 * invalidating the domain value (domain = -1) and printing a
+	 * corresponding error.
+	 */
+	if (domain >= 0 && use_dt_domains) {
+		use_dt_domains = 1;
+	} else if (domain < 0 && use_dt_domains != 1) {
+		use_dt_domains = 0;
+		domain = pci_get_new_domain_nr();
+	} else {
+		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
+			parent->of_node->full_name);
+		domain = -1;
+	}
+
+	bus->domain_nr = domain;
+}
+#endif
 #endif
 
 /**
-- 
2.1.2

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org>
> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments

Is this approach ok with everyone ? I would need to have this patch
queued so that I can rebase the ARM32 pcibios pci_sys_data domain removal
on top of it, if everyone agrees of course.

Thanks !
Lorenzo

> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> -- 
> 2.1.2
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments

Is this approach ok with everyone ? I would need to have this patch
queued so that I can rebase the ARM32 pcibios pci_sys_data domain removal
on top of it, if everyone agrees of course.

Thanks !
Lorenzo

> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> -- 
> 2.1.2
> 
> 

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments

Is this approach ok with everyone ? I would need to have this patch
queued so that I can rebase the ARM32 pcibios pci_sys_data domain removal
on top of it, if everyone agrees of course.

Thanks !
Lorenzo

> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> -- 
> 2.1.2
> 
> 

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Liviu Dudau]

On Wed, Nov 12, 2014 at 10:09:44AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> > v1 => v2:
> > 
> > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> >   adding an OF layer API
> > - Updated commit log and code comments
> 
> Is this approach ok with everyone ? I would need to have this patch
> queued so that I can rebase the ARM32 pcibios pci_sys_data domain removal
> on top of it, if everyone agrees of course.

Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

Best regards,
Liviu

> 
> Thanks !
> Lorenzo
> 
> > 
> >  arch/arm64/kernel/pci.c | 22 ----------------------
> >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..6f93c24 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  	return 0;
> >  }
> > -
> > -
> > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -static bool dt_domain_found = false;
> > -
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > -{
> > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > -
> > -	if (domain >= 0) {
> > -		dt_domain_found = true;
> > -	} else if (dt_domain_found == true) {
> > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > -			parent->of_node->full_name);
> > -		return;
> > -	} else {
> > -		domain = pci_get_new_domain_nr();
> > -	}
> > -
> > -	bus->domain_nr = domain;
> > -}
> > -#endif
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 625a4ac..2279414 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> >  {
> >  	return atomic_inc_return(&__domain_nr);
> >  }
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> >  /**
> > -- 
> > 2.1.2
> > 
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Liviu Dudau]

On Wed, Nov 12, 2014 at 10:09:44AM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> > v1 => v2:
> > 
> > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> >   adding an OF layer API
> > - Updated commit log and code comments
> 
> Is this approach ok with everyone ? I would need to have this patch
> queued so that I can rebase the ARM32 pcibios pci_sys_data domain removal
> on top of it, if everyone agrees of course.

Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

Best regards,
Liviu

> 
> Thanks !
> Lorenzo
> 
> > 
> >  arch/arm64/kernel/pci.c | 22 ----------------------
> >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..6f93c24 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  	return 0;
> >  }
> > -
> > -
> > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -static bool dt_domain_found = false;
> > -
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > -{
> > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > -
> > -	if (domain >= 0) {
> > -		dt_domain_found = true;
> > -	} else if (dt_domain_found == true) {
> > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > -			parent->of_node->full_name);
> > -		return;
> > -	} else {
> > -		domain = pci_get_new_domain_nr();
> > -	}
> > -
> > -	bus->domain_nr = domain;
> > -}
> > -#endif
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 625a4ac..2279414 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> >  {
> >  	return atomic_inc_return(&__domain_nr);
> >  }
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> >  /**
> > -- 
> > 2.1.2
> > 
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Arnd Bergmann]

On Monday 10 November 2014 16:41:46 Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

The general approach seems good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

I would suggest one simplification:

> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  

Since this is now in the file in which it gets called, you can mark the
function itself as 'static' and remove the extern declaration and inline
wrapper from the header file. You can also avoid the #ifdef by doing

void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
	static int use_dt_domains = -1;
	int domain = of_get_pci_domain_nr(parent->of_node);

	if (!IS_ENABLED(CONFIG_PCI_DOMAINS_GENERIC))
		return;

	...
}


	Arnd

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Arnd Bergmann]

On Monday 10 November 2014 16:41:46 Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

The general approach seems good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

I would suggest one simplification:

> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  

Since this is now in the file in which it gets called, you can mark the
function itself as 'static' and remove the extern declaration and inline
wrapper from the header file. You can also avoid the #ifdef by doing

void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
{
	static int use_dt_domains = -1;
	int domain = of_get_pci_domain_nr(parent->of_node);

	if (!IS_ENABLED(CONFIG_PCI_DOMAINS_GENERIC))
		return;

	...
}


	Arnd

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Wed, Nov 12, 2014 at 10:39:17AM +0000, Arnd Bergmann wrote:
> On Monday 10 November 2014 16:41:46 Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> The general approach seems good to me,
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> I would suggest one simplification:
> 
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> 
> Since this is now in the file in which it gets called, you can mark the
> function itself as 'static' and remove the extern declaration and inline
> wrapper from the header file. You can also avoid the #ifdef by doing

It is not, it is in driver/pci/pci.c, it is called in probe.c.

Maybe I can move the function to probe.c, but this would leave the
domain handling in two separate files.

I can't remove the #ifdeffery in that domain_nr in pci_bus is #ifdeffed
too, unless I remove that #ifdef and I compile it in all the time.

Thanks all for the review,
Lorenzo

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Wed, Nov 12, 2014 at 10:39:17AM +0000, Arnd Bergmann wrote:
> On Monday 10 November 2014 16:41:46 Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> The general approach seems good to me,
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> I would suggest one simplification:
> 
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> 
> Since this is now in the file in which it gets called, you can mark the
> function itself as 'static' and remove the extern declaration and inline
> wrapper from the header file. You can also avoid the #ifdef by doing

It is not, it is in driver/pci/pci.c, it is called in probe.c.

Maybe I can move the function to probe.c, but this would leave the
domain handling in two separate files.

I can't remove the #ifdeffery in that domain_nr in pci_bus is #ifdeffed
too, unless I remove that #ifdef and I compile it in all the time.

Thanks all for the review,
Lorenzo

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Arnd Bergmann]

On Wednesday 12 November 2014 14:14:29 Lorenzo Pieralisi wrote:
> > 
> > Since this is now in the file in which it gets called, you can mark the
> > function itself as 'static' and remove the extern declaration and inline
> > wrapper from the header file. You can also avoid the #ifdef by doing
> 
> It is not, it is in driver/pci/pci.c, it is called in probe.c.
> 
> Maybe I can move the function to probe.c, but this would leave the
> domain handling in two separate files.
> 
> I can't remove the #ifdeffery in that domain_nr in pci_bus is #ifdeffed
> too, unless I remove that #ifdef and I compile it in all the time.
> 

Right, I see. Unless Bjorn has some other preference, I'd just leave it
with your current version then.

	Arnd

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Arnd Bergmann]

On Wednesday 12 November 2014 14:14:29 Lorenzo Pieralisi wrote:
> > 
> > Since this is now in the file in which it gets called, you can mark the
> > function itself as 'static' and remove the extern declaration and inline
> > wrapper from the header file. You can also avoid the #ifdef by doing
> 
> It is not, it is in driver/pci/pci.c, it is called in probe.c.
> 
> Maybe I can move the function to probe.c, but this would leave the
> domain handling in two separate files.
> 
> I can't remove the #ifdeffery in that domain_nr in pci_bus is #ifdeffed
> too, unless I remove that #ifdef and I compile it in all the time.
> 

Right, I see. Unless Bjorn has some other preference, I'd just leave it
with your current version then.

	Arnd

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Yijing Wang]

Hi Lorenzo,
   You should send this to Bjorn instead of cc. Other, why put the OF related
function in PCI core. Why not move it in drivers/of/of_pci.c ?

On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments
> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> 


-- 
Thanks!
Yijing

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Yijing Wang]

Hi Lorenzo,
   You should send this to Bjorn instead of cc. Other, why put the OF related
function in PCI core. Why not move it in drivers/of/of_pci.c ?

On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments
> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> 


-- 
Thanks!
Yijing

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Yijing Wang]

Hi Lorenzo,
   You should send this to Bjorn instead of cc. Other, why put the OF related
function in PCI core. Why not move it in drivers/of/of_pci.c ?

On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments
> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> 


-- 
Thanks!
Yijing

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Wed, Nov 19, 2014 at 09:16:34AM +0000, Yijing Wang wrote:
> Hi Lorenzo,
>    You should send this to Bjorn instead of cc. Other, why put the OF related
> function in PCI core. Why not move it in drivers/of/of_pci.c ?

I did, you missed v1, and the problem is that with ACPI forthcoming we
do not want to have domain assignment scattered in different places,
but part of core code (ie a single function that prevents mixed
initialization from DT/counter and so on).

Bjorn, are you fine with this patch ? Do you want me to resend it as
part of the ARM 32 PCI domain refactoring [1] ? [1] depends on this
patch going in first.

Thanks,
Lorenzo

[1] http://www.spinics.net/lists/arm-kernel/msg375423.html

> On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> > v1 => v2:
> > 
> > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> >   adding an OF layer API
> > - Updated commit log and code comments
> > 
> >  arch/arm64/kernel/pci.c | 22 ----------------------
> >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..6f93c24 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  	return 0;
> >  }
> > -
> > -
> > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -static bool dt_domain_found = false;
> > -
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > -{
> > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > -
> > -	if (domain >= 0) {
> > -		dt_domain_found = true;
> > -	} else if (dt_domain_found == true) {
> > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > -			parent->of_node->full_name);
> > -		return;
> > -	} else {
> > -		domain = pci_get_new_domain_nr();
> > -	}
> > -
> > -	bus->domain_nr = domain;
> > -}
> > -#endif
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 625a4ac..2279414 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> >  {
> >  	return atomic_inc_return(&__domain_nr);
> >  }
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> >  /**
> > 
> 
> 
> -- 
> Thanks!
> Yijing
> 
> 

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

On Wed, Nov 19, 2014 at 09:16:34AM +0000, Yijing Wang wrote:
> Hi Lorenzo,
>    You should send this to Bjorn instead of cc. Other, why put the OF related
> function in PCI core. Why not move it in drivers/of/of_pci.c ?

I did, you missed v1, and the problem is that with ACPI forthcoming we
do not want to have domain assignment scattered in different places,
but part of core code (ie a single function that prevents mixed
initialization from DT/counter and so on).

Bjorn, are you fine with this patch ? Do you want me to resend it as
part of the ARM 32 PCI domain refactoring [1] ? [1] depends on this
patch going in first.

Thanks,
Lorenzo

[1] http://www.spinics.net/lists/arm-kernel/msg375423.html

> On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> > v1 => v2:
> > 
> > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> >   adding an OF layer API
> > - Updated commit log and code comments
> > 
> >  arch/arm64/kernel/pci.c | 22 ----------------------
> >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..6f93c24 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  	return 0;
> >  }
> > -
> > -
> > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -static bool dt_domain_found = false;
> > -
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > -{
> > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > -
> > -	if (domain >= 0) {
> > -		dt_domain_found = true;
> > -	} else if (dt_domain_found == true) {
> > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > -			parent->of_node->full_name);
> > -		return;
> > -	} else {
> > -		domain = pci_get_new_domain_nr();
> > -	}
> > -
> > -	bus->domain_nr = domain;
> > -}
> > -#endif
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 625a4ac..2279414 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> >  {
> >  	return atomic_inc_return(&__domain_nr);
> >  }
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> >  /**
> > 
> 
> 
> -- 
> Thanks!
> Yijing
> 
> 

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Bjorn Helgaas]

On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Applied with Liviu and Arnd's acks to pci/domain for v3.19, thanks.

It doesn't matter whether I'm in the to: or cc: list.  The important thing
is that it appears on the linux-pci list, because I use patchwork as my
to-do list, and patchwork only vacuums up stuff from linux-pci.

> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments
> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> -- 
> 2.1.2
> 

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Bjorn Helgaas]

On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> The current logic used for PCI domain assignment in arm64
> pci_bus_assign_domain_nr() is flawed in that, depending on the host
> controllers configuration for a platform and the respective initialization
> sequence, core code may end up allocating PCI domain numbers from both DT and
> the core code generic domain counter, which would result in PCI domain
> allocation aliases/errors.
> 
> This patch fixes the logic behind the PCI domain number assignment and
> moves the resulting code to generic PCI core code so that the same domain
> allocation logic is used on all platforms selecting
> 
> CONFIG_PCI_DOMAINS_GENERIC
> 
> instead of resorting to an arch specific implementation that might end up
> duplicating the PCI domain assignment logic wrongly.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Applied with Liviu and Arnd's acks to pci/domain for v3.19, thanks.

It doesn't matter whether I'm in the to: or cc: list.  The important thing
is that it appears on the linux-pci list, because I use patchwork as my
to-do list, and patchwork only vacuums up stuff from linux-pci.

> ---
> v1 => v2:
> 
> - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
>   adding an OF layer API
> - Updated commit log and code comments
> 
>  arch/arm64/kernel/pci.c | 22 ----------------------
>  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..6f93c24 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  	return 0;
>  }
> -
> -
> -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> -static bool dt_domain_found = false;
> -
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> -{
> -	int domain = of_get_pci_domain_nr(parent->of_node);
> -
> -	if (domain >= 0) {
> -		dt_domain_found = true;
> -	} else if (dt_domain_found == true) {
> -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> -			parent->of_node->full_name);
> -		return;
> -	} else {
> -		domain = pci_get_new_domain_nr();
> -	}
> -
> -	bus->domain_nr = domain;
> -}
> -#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 625a4ac..2279414 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -10,6 +10,8 @@
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
>  {
>  	return atomic_inc_return(&__domain_nr);
>  }
> +
> +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	static int use_dt_domains = -1;
> +	int domain = of_get_pci_domain_nr(parent->of_node);
> +
> +	/*
> +	 * Check DT domain and use_dt_domains values.
> +	 *
> +	 * 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.
> +	 *
> +	 * 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:
> +	 *
> +	 * 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).
> +	 *
> +	 * All other combinations imply we have a platform that is trying
> +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> +	 * which is a recipe for domain mishandling and it is prevented by
> +	 * invalidating the domain value (domain = -1) and printing a
> +	 * corresponding error.
> +	 */
> +	if (domain >= 0 && use_dt_domains) {
> +		use_dt_domains = 1;
> +	} else if (domain < 0 && use_dt_domains != 1) {
> +		use_dt_domains = 0;
> +		domain = pci_get_new_domain_nr();
> +	} else {
> +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> +			parent->of_node->full_name);
> +		domain = -1;
> +	}
> +
> +	bus->domain_nr = domain;
> +}
> +#endif
>  #endif
>  
>  /**
> -- 
> 2.1.2
> 

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Grant Likely]

On Wed, 19 Nov 2014 09:39:48 +0000
, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
 wrote:
> On Wed, Nov 19, 2014 at 09:16:34AM +0000, Yijing Wang wrote:
> > Hi Lorenzo,
> >    You should send this to Bjorn instead of cc. Other, why put the OF related
> > function in PCI core. Why not move it in drivers/of/of_pci.c ?
> 
> I did, you missed v1, and the problem is that with ACPI forthcoming we
> do not want to have domain assignment scattered in different places,
> but part of core code (ie a single function that prevents mixed
> initialization from DT/counter and so on).

Bus specific OF code has generally been moving into the individual
subsystems. SPI and I2C are the prime examples. It's easier to
coordinate with the generic subsystem code when it lives in the same
place. The PCI code still lives in drivers/of/ simply because nobody has
been motivated enough to refactor it.

> 
> Bjorn, are you fine with this patch ? Do you want me to resend it as
> part of the ARM 32 PCI domain refactoring [1] ? [1] depends on this
> patch going in first.
> 
> Thanks,
> Lorenzo
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg375423.html
> 
> > On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> > > The current logic used for PCI domain assignment in arm64
> > > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > > controllers configuration for a platform and the respective initialization
> > > sequence, core code may end up allocating PCI domain numbers from both DT and
> > > the core code generic domain counter, which would result in PCI domain
> > > allocation aliases/errors.
> > > 
> > > This patch fixes the logic behind the PCI domain number assignment and
> > > moves the resulting code to generic PCI core code so that the same domain
> > > allocation logic is used on all platforms selecting
> > > 
> > > CONFIG_PCI_DOMAINS_GENERIC
> > > 
> > > instead of resorting to an arch specific implementation that might end up
> > > duplicating the PCI domain assignment logic wrongly.
> > > 
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > > v1 => v2:
> > > 
> > > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> > >   adding an OF layer API
> > > - Updated commit log and code comments
> > > 
> > >  arch/arm64/kernel/pci.c | 22 ----------------------
> > >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 50 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > > index ce5836c..6f93c24 100644
> > > --- a/arch/arm64/kernel/pci.c
> > > +++ b/arch/arm64/kernel/pci.c
> > > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> > >  
> > >  	return 0;
> > >  }
> > > -
> > > -
> > > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > -static bool dt_domain_found = false;
> > > -
> > > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > -{
> > > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > > -
> > > -	if (domain >= 0) {
> > > -		dt_domain_found = true;
> > > -	} else if (dt_domain_found == true) {
> > > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > > -			parent->of_node->full_name);
> > > -		return;
> > > -	} else {
> > > -		domain = pci_get_new_domain_nr();
> > > -	}
> > > -
> > > -	bus->domain_nr = domain;
> > > -}
> > > -#endif
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 625a4ac..2279414 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -10,6 +10,8 @@
> > >  #include <linux/kernel.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/init.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_pci.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/pm.h>
> > >  #include <linux/slab.h>
> > > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> > >  {
> > >  	return atomic_inc_return(&__domain_nr);
> > >  }
> > > +
> > > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > +
> > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > +{
> > > +	static int use_dt_domains = -1;
> > > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > > +
> > > +	/*
> > > +	 * Check DT domain and use_dt_domains values.
> > > +	 *
> > > +	 * 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.
> > > +	 *
> > > +	 * 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:
> > > +	 *
> > > +	 * 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).
> > > +	 *
> > > +	 * All other combinations imply we have a platform that is trying
> > > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > > +	 * which is a recipe for domain mishandling and it is prevented by
> > > +	 * invalidating the domain value (domain = -1) and printing a
> > > +	 * corresponding error.
> > > +	 */
> > > +	if (domain >= 0 && use_dt_domains) {
> > > +		use_dt_domains = 1;
> > > +	} else if (domain < 0 && use_dt_domains != 1) {
> > > +		use_dt_domains = 0;
> > > +		domain = pci_get_new_domain_nr();
> > > +	} else {
> > > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > > +			parent->of_node->full_name);
> > > +		domain = -1;
> > > +	}
> > > +
> > > +	bus->domain_nr = domain;
> > > +}
> > > +#endif
> > >  #endif
> > >  
> > >  /**
> > > 
> > 
> > 
> > -- 
> > Thanks!
> > Yijing
> > 
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Grant Likely]

On Wed, 19 Nov 2014 09:39:48 +0000
, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
 wrote:
> On Wed, Nov 19, 2014 at 09:16:34AM +0000, Yijing Wang wrote:
> > Hi Lorenzo,
> >    You should send this to Bjorn instead of cc. Other, why put the OF related
> > function in PCI core. Why not move it in drivers/of/of_pci.c ?
> 
> I did, you missed v1, and the problem is that with ACPI forthcoming we
> do not want to have domain assignment scattered in different places,
> but part of core code (ie a single function that prevents mixed
> initialization from DT/counter and so on).

Bus specific OF code has generally been moving into the individual
subsystems. SPI and I2C are the prime examples. It's easier to
coordinate with the generic subsystem code when it lives in the same
place. The PCI code still lives in drivers/of/ simply because nobody has
been motivated enough to refactor it.

> 
> Bjorn, are you fine with this patch ? Do you want me to resend it as
> part of the ARM 32 PCI domain refactoring [1] ? [1] depends on this
> patch going in first.
> 
> Thanks,
> Lorenzo
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg375423.html
> 
> > On 2014/11/11 0:41, Lorenzo Pieralisi wrote:
> > > The current logic used for PCI domain assignment in arm64
> > > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > > controllers configuration for a platform and the respective initialization
> > > sequence, core code may end up allocating PCI domain numbers from both DT and
> > > the core code generic domain counter, which would result in PCI domain
> > > allocation aliases/errors.
> > > 
> > > This patch fixes the logic behind the PCI domain number assignment and
> > > moves the resulting code to generic PCI core code so that the same domain
> > > allocation logic is used on all platforms selecting
> > > 
> > > CONFIG_PCI_DOMAINS_GENERIC
> > > 
> > > instead of resorting to an arch specific implementation that might end up
> > > duplicating the PCI domain assignment logic wrongly.
> > > 
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > ---
> > > v1 => v2:
> > > 
> > > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> > >   adding an OF layer API
> > > - Updated commit log and code comments
> > > 
> > >  arch/arm64/kernel/pci.c | 22 ----------------------
> > >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 50 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > > index ce5836c..6f93c24 100644
> > > --- a/arch/arm64/kernel/pci.c
> > > +++ b/arch/arm64/kernel/pci.c
> > > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> > >  
> > >  	return 0;
> > >  }
> > > -
> > > -
> > > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > -static bool dt_domain_found = false;
> > > -
> > > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > -{
> > > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > > -
> > > -	if (domain >= 0) {
> > > -		dt_domain_found = true;
> > > -	} else if (dt_domain_found == true) {
> > > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > > -			parent->of_node->full_name);
> > > -		return;
> > > -	} else {
> > > -		domain = pci_get_new_domain_nr();
> > > -	}
> > > -
> > > -	bus->domain_nr = domain;
> > > -}
> > > -#endif
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 625a4ac..2279414 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -10,6 +10,8 @@
> > >  #include <linux/kernel.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/init.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_pci.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/pm.h>
> > >  #include <linux/slab.h>
> > > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> > >  {
> > >  	return atomic_inc_return(&__domain_nr);
> > >  }
> > > +
> > > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > +
> > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > +{
> > > +	static int use_dt_domains = -1;
> > > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > > +
> > > +	/*
> > > +	 * Check DT domain and use_dt_domains values.
> > > +	 *
> > > +	 * 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.
> > > +	 *
> > > +	 * 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:
> > > +	 *
> > > +	 * 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).
> > > +	 *
> > > +	 * All other combinations imply we have a platform that is trying
> > > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > > +	 * which is a recipe for domain mishandling and it is prevented by
> > > +	 * invalidating the domain value (domain = -1) and printing a
> > > +	 * corresponding error.
> > > +	 */
> > > +	if (domain >= 0 && use_dt_domains) {
> > > +		use_dt_domains = 1;
> > > +	} else if (domain < 0 && use_dt_domains != 1) {
> > > +		use_dt_domains = 0;
> > > +		domain = pci_get_new_domain_nr();
> > > +	} else {
> > > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > > +			parent->of_node->full_name);
> > > +		domain = -1;
> > > +	}
> > > +
> > > +	bus->domain_nr = domain;
> > > +}
> > > +#endif
> > >  #endif
> > >  
> > >  /**
> > > 
> > 
> > 
> > -- 
> > Thanks!
> > Yijing
> > 
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

Hi Bjorn,

On Thu, Nov 20, 2014 at 10:54:49PM +0000, Bjorn Helgaas wrote:
> On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Applied with Liviu and Arnd's acks to pci/domain for v3.19, thanks.
> 
> It doesn't matter whether I'm in the to: or cc: list.  The important thing
> is that it appears on the linux-pci list, because I use patchwork as my
> to-do list, and patchwork only vacuums up stuff from linux-pci.

This patch has not been merged last cycle and it is a prerequisite for
the ARM pcibios domain code removal patchset you merged in your pci/domain
branch. Please consider pulling it there so that the ARM pcibios domain
removal patchset can be based correctly on top of this patch, at the
moment it can't compile (see kbuild test report).

Thanks,
Lorenzo

> > ---
> > v1 => v2:
> > 
> > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> >   adding an OF layer API
> > - Updated commit log and code comments
> > 
> >  arch/arm64/kernel/pci.c | 22 ----------------------
> >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..6f93c24 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  	return 0;
> >  }
> > -
> > -
> > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -static bool dt_domain_found = false;
> > -
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > -{
> > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > -
> > -	if (domain >= 0) {
> > -		dt_domain_found = true;
> > -	} else if (dt_domain_found == true) {
> > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > -			parent->of_node->full_name);
> > -		return;
> > -	} else {
> > -		domain = pci_get_new_domain_nr();
> > -	}
> > -
> > -	bus->domain_nr = domain;
> > -}
> > -#endif
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 625a4ac..2279414 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> >  {
> >  	return atomic_inc_return(&__domain_nr);
> >  }
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> >  /**
> > -- 
> > 2.1.2
> > 
> 

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Lorenzo Pieralisi]

Hi Bjorn,

On Thu, Nov 20, 2014 at 10:54:49PM +0000, Bjorn Helgaas wrote:
> On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
> > The current logic used for PCI domain assignment in arm64
> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
> > controllers configuration for a platform and the respective initialization
> > sequence, core code may end up allocating PCI domain numbers from both DT and
> > the core code generic domain counter, which would result in PCI domain
> > allocation aliases/errors.
> > 
> > This patch fixes the logic behind the PCI domain number assignment and
> > moves the resulting code to generic PCI core code so that the same domain
> > allocation logic is used on all platforms selecting
> > 
> > CONFIG_PCI_DOMAINS_GENERIC
> > 
> > instead of resorting to an arch specific implementation that might end up
> > duplicating the PCI domain assignment logic wrongly.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Applied with Liviu and Arnd's acks to pci/domain for v3.19, thanks.
> 
> It doesn't matter whether I'm in the to: or cc: list.  The important thing
> is that it appears on the linux-pci list, because I use patchwork as my
> to-do list, and patchwork only vacuums up stuff from linux-pci.

This patch has not been merged last cycle and it is a prerequisite for
the ARM pcibios domain code removal patchset you merged in your pci/domain
branch. Please consider pulling it there so that the ARM pcibios domain
removal patchset can be based correctly on top of this patch, at the
moment it can't compile (see kbuild test report).

Thanks,
Lorenzo

> > ---
> > v1 => v2:
> > 
> > - Moved generic pci_bus_assign_domain_nr() code to PCI core instead of
> >   adding an OF layer API
> > - Updated commit log and code comments
> > 
> >  arch/arm64/kernel/pci.c | 22 ----------------------
> >  drivers/pci/pci.c       | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..6f93c24 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -46,25 +46,3 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  	return 0;
> >  }
> > -
> > -
> > -#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -static bool dt_domain_found = false;
> > -
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > -{
> > -	int domain = of_get_pci_domain_nr(parent->of_node);
> > -
> > -	if (domain >= 0) {
> > -		dt_domain_found = true;
> > -	} else if (dt_domain_found == true) {
> > -		dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n",
> > -			parent->of_node->full_name);
> > -		return;
> > -	} else {
> > -		domain = pci_get_new_domain_nr();
> > -	}
> > -
> > -	bus->domain_nr = domain;
> > -}
> > -#endif
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 625a4ac..2279414 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/of.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/pm.h>
> >  #include <linux/slab.h>
> > @@ -4447,6 +4449,54 @@ int pci_get_new_domain_nr(void)
> >  {
> >  	return atomic_inc_return(&__domain_nr);
> >  }
> > +
> > +#ifdef CONFIG_PCI_DOMAINS_GENERIC
> > +
> > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +{
> > +	static int use_dt_domains = -1;
> > +	int domain = of_get_pci_domain_nr(parent->of_node);
> > +
> > +	/*
> > +	 * Check DT domain and use_dt_domains values.
> > +	 *
> > +	 * 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.
> > +	 *
> > +	 * 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:
> > +	 *
> > +	 * 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).
> > +	 *
> > +	 * All other combinations imply we have a platform that is trying
> > +	 * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> > +	 * which is a recipe for domain mishandling and it is prevented by
> > +	 * invalidating the domain value (domain = -1) and printing a
> > +	 * corresponding error.
> > +	 */
> > +	if (domain >= 0 && use_dt_domains) {
> > +		use_dt_domains = 1;
> > +	} else if (domain < 0 && use_dt_domains != 1) {
> > +		use_dt_domains = 0;
> > +		domain = pci_get_new_domain_nr();
> > +	} else {
> > +		dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
> > +			parent->of_node->full_name);
> > +		domain = -1;
> > +	}
> > +
> > +	bus->domain_nr = domain;
> > +}
> > +#endif
> >  #endif
> >  
> >  /**
> > -- 
> > 2.1.2
> > 
> 

Re: [RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Bjorn Helgaas]

On Sat, Dec 27, 2014 at 3:36 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Hi Bjorn,
>
> On Thu, Nov 20, 2014 at 10:54:49PM +0000, Bjorn Helgaas wrote:
>> On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
>> > The current logic used for PCI domain assignment in arm64
>> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
>> > controllers configuration for a platform and the respective initialization
>> > sequence, core code may end up allocating PCI domain numbers from both DT and
>> > the core code generic domain counter, which would result in PCI domain
>> > allocation aliases/errors.
>> >
>> > This patch fixes the logic behind the PCI domain number assignment and
>> > moves the resulting code to generic PCI core code so that the same domain
>> > allocation logic is used on all platforms selecting
>> >
>> > CONFIG_PCI_DOMAINS_GENERIC
>> >
>> > instead of resorting to an arch specific implementation that might end up
>> > duplicating the PCI domain assignment logic wrongly.
>> >
>> > Cc: Arnd Bergmann <arnd@arndb.de>
>> > Cc: Liviu Dudau <liviu.dudau@arm.com>
>> > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > Cc: Rob Herring <robh+dt@kernel.org>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>
>> Applied with Liviu and Arnd's acks to pci/domain for v3.19, thanks.
>>
>> It doesn't matter whether I'm in the to: or cc: list.  The important thing
>> is that it appears on the linux-pci list, because I use patchwork as my
>> to-do list, and patchwork only vacuums up stuff from linux-pci.
>
> This patch has not been merged last cycle and it is a prerequisite for
> the ARM pcibios domain code removal patchset you merged in your pci/domain
> branch. Please consider pulling it there so that the ARM pcibios domain
> removal patchset can be based correctly on top of this patch, at the
> moment it can't compile (see kbuild test report).

Huh, I can't remember why I didn't merge that for v3.19.  Maybe I just
missed it by mistake.

Anyway, I picked that patch into pci/domain for another try.

Bjorn

[RFC PATCH v2] drivers: pci: move PCI domain assignment to generic PCI code

[Bjorn Helgaas]

On Sat, Dec 27, 2014 at 3:36 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> Hi Bjorn,
>
> On Thu, Nov 20, 2014 at 10:54:49PM +0000, Bjorn Helgaas wrote:
>> On Mon, Nov 10, 2014 at 04:41:46PM +0000, Lorenzo Pieralisi wrote:
>> > The current logic used for PCI domain assignment in arm64
>> > pci_bus_assign_domain_nr() is flawed in that, depending on the host
>> > controllers configuration for a platform and the respective initialization
>> > sequence, core code may end up allocating PCI domain numbers from both DT and
>> > the core code generic domain counter, which would result in PCI domain
>> > allocation aliases/errors.
>> >
>> > This patch fixes the logic behind the PCI domain number assignment and
>> > moves the resulting code to generic PCI core code so that the same domain
>> > allocation logic is used on all platforms selecting
>> >
>> > CONFIG_PCI_DOMAINS_GENERIC
>> >
>> > instead of resorting to an arch specific implementation that might end up
>> > duplicating the PCI domain assignment logic wrongly.
>> >
>> > Cc: Arnd Bergmann <arnd@arndb.de>
>> > Cc: Liviu Dudau <liviu.dudau@arm.com>
>> > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > Cc: Rob Herring <robh+dt@kernel.org>
>> > Cc: Catalin Marinas <catalin.marinas@arm.com>
>> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>
>> Applied with Liviu and Arnd's acks to pci/domain for v3.19, thanks.
>>
>> It doesn't matter whether I'm in the to: or cc: list.  The important thing
>> is that it appears on the linux-pci list, because I use patchwork as my
>> to-do list, and patchwork only vacuums up stuff from linux-pci.
>
> This patch has not been merged last cycle and it is a prerequisite for
> the ARM pcibios domain code removal patchset you merged in your pci/domain
> branch. Please consider pulling it there so that the ARM pcibios domain
> removal patchset can be based correctly on top of this patch, at the
> moment it can't compile (see kbuild test report).

Huh, I can't remember why I didn't merge that for v3.19.  Maybe I just
missed it by mistake.

Anyway, I picked that patch into pci/domain for another try.

Bjorn