Re: [PATCH v2 1/2] PCI: Add new method for registering PCI hosts

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Friday, July 1, 2016 3:46:48 PM CEST Liviu Dudau wrote:
> On Thu, Jun 30, 2016 at 05:19:30PM +0200, Thierry Reding wrote:
> > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > 
> > This patch makes the existing pci_host_bridge structure a proper device
> > that is usable by PCI host drivers in a more standard way. In addition
> > to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
> > and pci_create_root_bus interfaces, this unfortunately means having to
> > add yet another interface doing basically the same thing, and add some
> > extra code in the initial step.
> > 
> > However, this time it's more likely to be extensible enough that we
> > won't have to do another one again in the future, and we should be
> > able to reduce code much more as a result.
> 
> I am really disapointed by the direction in which the whole pci_host_bridge
> structure and associated functions is going. When I started to get involved
> in this mess that is the creation of a root PCI bus I was hoping that we
> are moving towards one or few functions that create the root bus by using
> the details provided in the host bridge without having so many variants for
> the functionality.

That is the plan, we just never made a lot of progress here.

> One reason for this holy mess is the duplication of information. Both root
> bus and the pci_host_bridge hold pointers to useful information (msi_controller,
> pci_ops) and coherency should be guaranteed when one or the other gets created.
> Hence the ridiculous dance being done to find if root bus hasn't already been
> created and if not reusing the scrap bus we have created at the top of
> pci_create_root_bus() to actually set the data, then create the pci_host_bridge,
> oops we need more information, and so on. (apologies for the incoherent style,
> I'm trying to duplicate the spirit of the probe.c code :) )

Agreed.

> I think this patchset has the right intention but it is not doing it in
> the right way. To me, the right way is to separate the whole allocation
> of the pci_host_bridge structure from the scanning or the root bus (keeping the
> INIT_LIST_HEAD(&bridge->windows) call inside), not add the sysdata pointer
> *at all* to pci_host_bridge, move the information from pci_bus into pci_host_bridge
> and make pci_scan_root_bus take a pci_host_bridge pointer once that has been
> done.

Again, this is what I'm doing here, unfortunately we cannot remove the
sysdata pointer altogether as we still have over a hundred existing PCI
host bridge implementations that all use sysdata.

> Think of the code structure as a reflection of how the system is structured: the
> PCI-to-host bridge is a structure that holds the information required to connect
> the functionality of the host code (msi_controller, host-to-bus resource windows)
> to the PCI(e) bus. The root pci_bus should only have PCI(e) bus information if
> possible.

Exactly.

> > 
> > The main idea is to pull the allocation of 'struct pci_host_bridge' out
> > of the registration, and let individual host drivers and architecture
> > code fill the members before calling the registration function.
> 
> That's how we got into the arch/arm mess that we currently have. The host driver
> should not drive the instantiation of the pci_host_bridge structure because it
> will prevent you from having two drivers running at the same time.

Can you clarify that? I don't see what prevents two drivers from each
creating a pci_host_bridge structure and passing it into
pci_host_bridge_register().

> > -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> > -{
> > -	struct pci_host_bridge *bridge;
> > -
> > -	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > -	if (!bridge)
> > -		return NULL;
> > -
> > -	INIT_LIST_HEAD(&bridge->windows);
> > -	bridge->bus = b;
> > -	return bridge;
> > -}
> > -
> 
> You might have strong arguments for removing this function but they are not properly
> explained here and I feel that they should. Specially as ....

In my initial version, it simply became unnecessary because all callers
of pci_host_bridge_register() would have to allocate it anyway, and with
the pci_bus assignment gone, it didn't do much at all.

> >  static const unsigned char pcix_bus_speed[] = {
> >  	PCI_SPEED_UNKNOWN,		/* 0 */
> >  	PCI_SPEED_66MHz_PCIX,		/* 1 */
> > @@ -2117,53 +2104,56 @@ 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)
> > +void pci_host_bridge_init(struct pci_host_bridge *bridge)
> > +{
> > +	INIT_LIST_HEAD(&bridge->windows);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_host_bridge_init);
> 
> I have no idea why it is useful to re-initialise the bridge->windows list at any
> time other than allocation time.

That's why I suggested doing this in the allocation function now.

> > +int pci_host_bridge_register(struct pci_host_bridge *bridge)
> >  {
> >  	int error;
> > -	struct pci_host_bridge *bridge;
> >  	struct pci_bus *b, *b2;
> >  	struct resource_entry *window, *n;
> > +	LIST_HEAD(resources);
> >  	struct resource *res;
> >  	resource_size_t offset;
> >  	char bus_addr[64];
> >  	char *fmt;
> > +	struct device *parent = bridge->dev.parent;
> >  
> >  	b = pci_alloc_bus(NULL);
> >  	if (!b)
> > -		return NULL;
> > +		return -ENOMEM;
> > +	bridge->bus = b;
> >  
> > -	b->sysdata = sysdata;
> > -	b->ops = ops;
> > -	b->number = b->busn_res.start = bus;
> > +	/* temporarily move resources off the list */
> > +	list_splice_init(&bridge->windows, &resources);
> 
> What are you trying to accomplish here with the moving around of the bridge->windows list
> elements? This also leaves the bridge->windows list empty afterwards, is that intended?

I was trying to preserve the existing behavior, this can probably
be simplified, but as my initial version was completely untested
I decided not to touch it.

Later in the function, the list is filled one entry at a time from
the local 'resources' list that we traditionally passed as a function
argument before.

Ideally we'd just walk the list instead of doing the
split/list_del/list_add dance, and this would be a logical
cleanup on top of this patch.

> > @@ -817,6 +821,8 @@ 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);
> > +void pci_host_bridge_init(struct pci_host_bridge *bridge);
> > +int pci_host_bridge_register(struct pci_host_bridge *bridge);
> >  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);
> 
> There is too much stuff moving around. I know it is needed and the temptation is to get it
> done as quickly as possible, but it makes the reviewing and the commenting on this series hard.

This is why I left the list handling you mentioned above unchanged
in my original patch instead of rewriting it.

> Can you split the patch into one that adds the new members
> into pci_host_bridge, then do the renaming/re-organisation in
> another patch?

That should be possible, I guess Thierry can deal with that when respinning
the series.

	Arnd