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:

quoted

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