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@xxxxxxxx>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
