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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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