diff mbox

[RFC] drivers: of: move PCI domain assignment to generic OF code

Message ID 1415622368-14612-1-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Lorenzo Pieralisi Nov. 10, 2014, 12:26 p.m. UTC
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
move the resulting code to the generic OF layer so that other archs can
reuse the same domain allocation logic instead of resorting to an arch
specific implementation that might end up duplicating the PCI domain
assignment logic wrongly.

If OF is not configured for a platform, the code falls back to the
generic domain numbering implementation through the pci_get_new_domain_nr()
API.

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>
---
 arch/arm64/kernel/pci.c | 14 +++--------
 drivers/of/of_pci.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/of_pci.h  |  7 +++---
 3 files changed, 71 insertions(+), 16 deletions(-)

Comments

Arnd Bergmann Nov. 10, 2014, 12:51 p.m. UTC | #1
On Monday 10 November 2014 12:26:08 Lorenzo Pieralisi wrote:
> ---
>  arch/arm64/kernel/pci.c | 14 +++--------
>  drivers/of/of_pci.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/of_pci.h  |  7 +++---
>  3 files changed, 71 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index ce5836c..5e21c1c 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -49,20 +49,14 @@ int pcibios_add_device(struct pci_dev *dev)
>  
>  
>  #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);
> +	int domain = of_assign_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();
> +	if (domain < 0) {
> +		dev_err(parent, "PCI domain assignment failed\n");
> +		domain = -1;
>  	}
>  
>  	bus->domain_nr = domain;

Is there a need to still keep this in architecture specific code? Why
not move it to drivers/pci and let other firmware infrastructure
hook in there directly.

>  {
> @@ -45,10 +45,9 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>  	return -EINVAL;
>  }
>  
> -static inline int
> -of_get_pci_domain_nr(struct device_node *node)
> +static inline int of_assign_pci_domain_nr(struct device_node *node)
>  {
> -	return -1;
> +	return pci_get_new_domain_nr();
>  }
>  #endif

This gets a bit tricky otherwise once we add ACPI in the mix, with all
combinations of OF and ACPI at compile time and at runtime.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Nov. 10, 2014, 1:52 p.m. UTC | #2
On Mon, Nov 10, 2014 at 12:51:19PM +0000, Arnd Bergmann wrote:
> On Monday 10 November 2014 12:26:08 Lorenzo Pieralisi wrote:
> > ---
> >  arch/arm64/kernel/pci.c | 14 +++--------
> >  drivers/of/of_pci.c     | 66 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  include/linux/of_pci.h  |  7 +++---
> >  3 files changed, 71 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index ce5836c..5e21c1c 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -49,20 +49,14 @@ int pcibios_add_device(struct pci_dev *dev)
> >  
> >  
> >  #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);
> > +	int domain = of_assign_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();
> > +	if (domain < 0) {
> > +		dev_err(parent, "PCI domain assignment failed\n");
> > +		domain = -1;
> >  	}
> >  
> >  	bus->domain_nr = domain;
> 
> Is there a need to still keep this in architecture specific code? Why
> not move it to drivers/pci and let other firmware infrastructure
> hook in there directly.

Ok, I can move the function to drivers/pci/pci.c, I did not want to add
OF code in there but if it is ok with Bjorn I will move the generic

pci_bus_assign_domain_nr()

there and be done with this.

> >  {
> > @@ -45,10 +45,9 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> >  	return -EINVAL;
> >  }
> >  
> > -static inline int
> > -of_get_pci_domain_nr(struct device_node *node)
> > +static inline int of_assign_pci_domain_nr(struct device_node *node)
> >  {
> > -	return -1;
> > +	return pci_get_new_domain_nr();
> >  }
> >  #endif
> 
> This gets a bit tricky otherwise once we add ACPI in the mix, with all
> combinations of OF and ACPI at compile time and at runtime.

I definitely agree, that's why it is an RFC, basically wanted to
understand what we want to do with the generic implementation and where
to move it.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index ce5836c..5e21c1c 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -49,20 +49,14 @@  int pcibios_add_device(struct pci_dev *dev)
 
 
 #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);
+	int domain = of_assign_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();
+	if (domain < 0) {
+		dev_err(parent, "PCI domain assignment failed\n");
+		domain = -1;
 	}
 
 	bus->domain_nr = domain;
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 8882b46..43bf591 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -100,7 +100,7 @@  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
  * Returns the associated domain number from DT in the range [0-0xffff], or
  * a negative value if the required property is not found.
  */
-int of_get_pci_domain_nr(struct device_node *node)
+static int of_get_pci_domain_nr(struct device_node *node)
 {
 	const __be32 *value;
 	int len;
@@ -114,7 +114,69 @@  int of_get_pci_domain_nr(struct device_node *node)
 
 	return domain;
 }
-EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
+
+/**
+ * of_assign_pci_domain_nr() - Assign a PCI domain number
+ *
+ * @node: device tree node with the domain information
+ *
+ * This function assigns and returns the host bridge domain number by
+ * first parsing the device tree node to find the linux,pci-domain
+ * property and falling back to the generic domain number API if DT
+ * property is not present/valid. The function implements logic that prevents
+ * code from mixing domain numbers assignments from both DT and the
+ * generic domain number implementation and returns an error if such
+ * cases are detected at run-time.
+ *
+ * Returns the assigned domain number, or a negative value to signal an error
+ */
+int of_assign_pci_domain_nr(struct device_node *node)
+{
+	static int use_dt_domains = -1;
+	int domain = of_get_pci_domain_nr(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().
+	 *
+	 * If use_dt_domains value is == 0 and the DT property provides a
+	 * valid domain number (>=0), this means that we are assigning a
+	 * domain number from DT but we have previously allocated a domain
+	 * number by using pci_get_new_domain_nr() so we should bail out with
+	 * an error.
+	 *
+	 * If DT domain property value is not valid, 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 DT and generic domain number assignments, which is a recipe
+	 * for domain mishandling and it is prevented by returning an error
+	 * value to the caller.
+	 */
+	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 {
+		pr_err("Node %s has inconsistent \"linux,pci-domain\" property in DT\n",
+			node->full_name);
+		return -EINVAL;
+	}
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(of_assign_pci_domain_nr);
 
 #if defined(CONFIG_OF_ADDRESS)
 /**
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 1fd207e..fbba966 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -15,7 +15,7 @@  struct device_node *of_pci_find_child_device(struct device_node *parent,
 int of_pci_get_devfn(struct device_node *np);
 int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
-int of_get_pci_domain_nr(struct device_node *node);
+int of_assign_pci_domain_nr(struct device_node *node);
 #else
 static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
 {
@@ -45,10 +45,9 @@  of_pci_parse_bus_range(struct device_node *node, struct resource *res)
 	return -EINVAL;
 }
 
-static inline int
-of_get_pci_domain_nr(struct device_node *node)
+static inline int of_assign_pci_domain_nr(struct device_node *node)
 {
-	return -1;
+	return pci_get_new_domain_nr();
 }
 #endif