Re: [PATCH v7 4/6] pci: Introduce a domain number for pci_host_bridge.

[Liviu Dudau <Liviu.Dudau@arm.com>]

On Sat, Apr 05, 2014 at 01:00:07AM +0100, Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 03:34:30PM +0000, Liviu Dudau wrote:
> > Make it easier to discover the domain number of a bus by storing
> > the number in pci_host_bridge for the root bus. Several architectures
> > have their own way of storing this information, so it makes sense
> > to try to unify the code. 
> 
> I like the idea of unifying the way we handle the domain number.  But 
> I'd like to see more of the strategy before committing to this approach.

*My* strategy is to get rid of pci_domain_nr(). I don't see why we need
to have arch specific way of providing the number, specially after looking
at the existing implementations that return a value from a variable that
is never touched or incremented. My guess is that pci_domain_nr() was
created to work around the fact that there was no domain_nr maintainance in
the generic code.

> 
> This patch adds struct pci_host_bridge.domain_nr, but of course
> pci_domain_nr() doesn't use it.  It can't today, because
> pci_create_root_bus() relies on pci_domain_nr() to fill in
> pci_host_bridge.domain_nr.
> 
> But I suppose the intent is that someday we can make pci_domain_nr()
> arch-independent somehow.  I'd just like to see more of the path to
> that happening.

The path would be to send a patch that removes all existing pci_domain_nr()
macros/inline functions and rely on the generic function.

> 
> > While at this, add a new function that
> > creates a root bus in a given domain and make pci_create_root_bus()
> > a wrapper around this function.
> 
> I'm a little concerned about adding a new "create root bus" interface,
> partly because we have quite a few already, and I'd like to reduce the
> number of them instead of adding more.  And I think there might be other
> similar opportunities for unification, so I could easily see us adding new
> functions in the future to unify NUMA node info, ECAM info, etc.

The reason for creating the wrapper function was to allow for explicit passing
of domain_nr. If we find architectures where generic allocation of domain_nr
doesn't work for them, we can make them use this wrapper to pass the domain_nr
into the host bridge when creating the root bus.

> 
> I wonder if we need some sort of descriptor structure that the arch could
> fill in and pass to the PCI core.  Then we could add new members without
> having to create new interfaces each time.

I'm trying to reduce the number of variables being passed between architectures
and generic code. host_bridge (with the associated root bus), domain_nr those
are needed. Is there anything else that you have in your mind that needs to
be shared?

My approach would be in sharing of the data: PCI is a standard, and the core
framework implements it. What is so broken in your architecture that you need
to work around the core code? And I'm not talking about drivers and quirks,
but architectural level support.

Best regards,
Liviu

> 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++--------
> >  include/linux/pci.h |  4 ++++
> >  2 files changed, 37 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index fd11c12..172c615 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1714,8 +1714,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
> >  {
> >  }
> >  
> > -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> > +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
> > +		int domain, int bus, struct pci_ops *ops, void *sysdata,
> > +		struct list_head *resources)
> >  {
> >  	int error;
> >  	struct pci_host_bridge *bridge;
> > @@ -1728,30 +1729,34 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >  
> >  	bridge = pci_alloc_host_bridge();
> >  	if (!bridge)
> > -		return NULL;
> > +		return ERR_PTR(-ENOMEM);
> >  
> >  	bridge->dev.parent = parent;
> >  	bridge->dev.release = pci_release_host_bridge_dev;
> > +	bridge->domain_nr = domain;
> >  	error = pcibios_root_bridge_prepare(bridge);
> >  	if (error)
> >  		goto err_out;
> >  
> >  	b = pci_alloc_bus();
> > -	if (!b)
> > +	if (!b) {
> > +		error = -ENOMEM;
> >  		goto err_out;
> > +	}
> >  
> >  	b->sysdata = sysdata;
> >  	b->ops = ops;
> >  	b->number = b->busn_res.start = bus;
> > -	b2 = pci_find_bus(pci_domain_nr(b), bus);
> > +	b2 = pci_find_bus(bridge->domain_nr, bus);
> >  	if (b2) {
> >  		/* If we already got to this bus through a different bridge, ignore it */
> >  		dev_dbg(&b2->dev, "bus already known\n");
> > +		error = -EEXIST;
> >  		goto err_bus_out;
> >  	}
> >  
> >  	bridge->bus = b;
> > -	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> > +	dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus);
> >  	error = device_register(&bridge->dev);
> >  	if (error) {
> >  		put_device(&bridge->dev);
> > @@ -1766,7 +1771,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >  
> >  	b->dev.class = &pcibus_class;
> >  	b->dev.parent = b->bridge;
> > -	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> > +	dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus);
> >  	error = device_register(&b->dev);
> >  	if (error)
> >  		goto class_dev_reg_err;
> > @@ -1816,7 +1821,27 @@ err_bus_out:
> >  	kfree(b);
> >  err_out:
> >  	kfree(bridge);
> > -	return NULL;
> > +	return ERR_PTR(error);
> > +}
> > +
> > +struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> > +{
> > +	int domain_nr;
> > +	struct pci_bus *b = pci_alloc_bus();
> > +	if (!b)
> > +		return NULL;
> > +
> > +	b->sysdata = sysdata;
> > +	domain_nr = pci_domain_nr(b);
> > +	kfree(b);
> > +
> > +	b = pci_create_root_bus_in_domain(parent, domain_nr, bus,
> > +				ops, sysdata, resources);
> > +	if (IS_ERR(b))
> > +		return NULL;
> > +
> > +	return b;
> >  }
> >  
> >  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 33aa2ca..1eed009 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -394,6 +394,7 @@ struct pci_host_bridge_window {
> >  struct pci_host_bridge {
> >  	struct device dev;
> >  	struct pci_bus *bus;		/* root bus */
> > +	int domain_nr;
> >  	struct list_head windows;	/* pci_host_bridge_windows */
> >  	void (*release_fn)(struct pci_host_bridge *);
> >  	void *release_data;
> > @@ -747,6 +748,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
> >  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >  				    struct pci_ops *ops, void *sysdata,
> >  				    struct list_head *resources);
> > +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
> > +			int domain, int bus, struct pci_ops *ops,
> > +			void *sysdata, struct list_head *resources);
> >  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
> >  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
> >  void pci_bus_release_busn_res(struct pci_bus *b);
> > -- 
> > 1.9.0
> > 
> 

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