| Message ID | 1948942.cIbLaKDEOa@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Feb 19, 2014 at 11:24:30AM +0100, Arnd Bergmann wrote: > On Wednesday 19 February 2014 02:44:27 Liviu Dudau wrote: > > On Tue, Feb 18, 2014 at 10:41:25AM -0700, Jason Gunthorpe wrote: > > > > > So the Liviu, I would say the API should be similar to what we see in > > > other OF enabled driver bases subsystems, call the core code with a > > > platform_device pointer and function_ops pointer and have the core > > > code create a domain, figure out the domain # from the DT (via > > > aliases?), and so on. > > > > I wish things were easier! > > > > Lets look at the 'int pci_domain_nr(struct pci_bus *bus);' function. It is > > used to obtain the domain number of the bus passed as an argument. > > > > - include/linux/pci.h defines it as an inline function returning zero if > > !CONFIG_PCI || !CONFIG_PCI_DOMAINS. Otherwise it is silent on what the > > function might look like. > > I think we should come up with a default implementation that works for > any architecture using DT based probing, and requires PCI_DOMAINS to > be enabled. Agree. What I was trying to say was that the function is not *defined* if you have CONFIG_PCI && !CONFIG_PCI_DOMAINS, but it has a dummy implementation for !CONFIG_PCI or CONFIG_PCI && CONFIG_PCI_DOMAINS. > > > - alpha, ia64, microblaze, mips, powerpc and tile all define it as a cast > > of bus->sysdata to "struct pci_controller *" and then access a data > > member from there to get the domain number. But ... the pci_controller > > structure is different for each architecture, with few more members in > > addition that might be actually shared with generic framework code. > > We do have the generic 'struct pci_host_bridge'. It would be trivial to > add a 'domain' field in there and use it in the generic implementation. That's what my v1 patch was trying to do, but as Tanmay reported, it is not working, see below for the explanation why (short summary, we ask for domain number of a bus before the associated pci_host_bridge is alive). > > > - arm, s390, sparc and x86 have all different names for their sysdata, > > but they all contain inside a member that is used for giving a domain > > back. sparc gets an honorary mention here for getting the API wrong > > and returning -ENXIO in certain conditions (not like the generic code > > cares). > > I would hope that for the generic case, we can actually remove the > use of 'sysdata' and replace it with common code that just looks > at pci_host_bridge. Some architectures (s390 in particular) will probably > need their own data to be added in sysdata, but overall the architectures > are really trying to do the same thing here. With the work that Bjorn > and others have done over the past few years, you already need much less > architecture specific code. I think it's time to get it to a point where > most architectures can do without any at all. We are on the same page. I was just flagging the amount of work we need to do here to bring everyone towards common code. > > > That takes care of the implementation. But what about usage? > > > > - drivers/pci/probe.c: pci_create_root_bus allocates a new bus structure, > > sets up the sysdata and ops member pointers and then goes straight > > into trying to find if the newly created bus doesn't already exist by > > using the bus number given as parameter and pci_domain_nr() with the > > new bus structure as argument. I'm trying really hard to figure out > > what was the intention here, but from the point of view of someone > > trying to implement the pci_domain_nr() function I have no idea what > > to return for a virgin bus structure that is not yet attached to any > > parent. > > Right, this needs to be changed when moving the domain into pci_host_bridge. Yes, if only I understood what the intent was here. Unfortunately, the history of the file stops (for my git tree) at Linus' first commit from v2.6.0, so I need to dig deeper. What I am looking to understand is the answer to this question: "When you want to create a new bus and you want to make sure it is not duplicated, do you go across all domains or pick the current domain and allow for same bus number in different domains?" To give some context: I'm looking at drivers/pci/probe.c, pci_create_root_bus(). Specifically, this code: b = pci_alloc_bus(); if (!b) return NULL; b->sysdata = sysdata; b->ops = ops; b->number = b->busn_res.start = bus; b2 = pci_find_bus(pci_domain_nr(b), bus); if (b2) { /* If we already got to this bus through a different bridge, ignore it */ dev_dbg(&b2->dev, "bus already known\n"); goto err_out; } bridge = pci_alloc_host_bridge(b); if (!bridge) goto err_out; If you look at the comment after pci_find_bus, you can see that the intent was to try to find duplicate busses covered by different bridges (but in the same domain? maybe, because there used to be only one domain at the beginning, but then why use pci_domain_nr() here?). Problems I see here: - code is trying to get the domain number of a bus that it just created but not plugged yet in the hierarchy - pci_find_bus tries to find a bus in the global list of busses that has the same domain number and bus number as the parameters of the function. But the domain number was plucked out of thin air and all architectures seem to actually give you the current active domain number, not the one that actually covers the bus (and host_bridge) that you are trying to create. I will try to restructure the code here, but I think the check is superfluous and can be done better by a function that tries to find duplicates when you add the bus, rather than when creating the scaffolding. > > > So I can see the intent of what Jason is proposing and I'm heading that > > way myself, but I think I need to cleanup pci_create_root_bus first > > (change the creation order between bridge and bus). And if someone has > > a good idea on how to determine the domain # from DT we can pluck it > > into the pcibios_root_bridge_prepare() function (either the generic > > version or the arch specific one). > > How about the change below, to introduce a new pci_scan_domain() function > as a variant of pci_scan_root_bus()? > > Arnd > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 5094943..9f2ec2f 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1697,8 +1697,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) > +static struct pci_bus *__pci_create_root_bus(struct device *parent, > + int domain, int bus, struct pci_ops *ops, void *sysdata, > + struct list_head *resources) > { > int error; > struct pci_host_bridge *bridge; > @@ -1716,7 +1717,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > 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(domain, bus); > if (b2) { > /* If we already got to this bus through a different bridge, ignore it */ > dev_dbg(&b2->dev, "bus already known\n"); > @@ -1727,6 +1728,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > if (!bridge) > goto err_out; > > + bridge->domain = domain; > bridge->dev.parent = parent; > bridge->dev.release = pci_release_host_bridge_dev; > dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); > @@ -1801,6 +1803,13 @@ err_out: > return NULL; > } > > +struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > + struct pci_ops *ops, void *sysdata, struct list_head *resources) > +{ > + return __pci_create_root_bus(parent, pci_domain_nr(bus), bus, > + ops, sysdata, resources); > +} > + > int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > { > struct resource *res = &b->busn_res; > @@ -1899,6 +1908,42 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, > } > EXPORT_SYMBOL(pci_scan_root_bus); > > +struct pci_bus *pci_scan_domain(struct device *parent, int domain, > + struct pci_ops *ops, void *sysdata, struct list_head *resources) > +{ > + struct pci_host_bridge_window *window; > + bool found = false; > + struct pci_bus *b; > + int max; > + > + list_for_each_entry(window, resources, list) > + if (window->res->flags & IORESOURCE_BUS) { > + found = true; > + break; > + } > + > + if (!found) { > + dev_info(&b->dev, > + "No busn resource found for domain %d\n", domain); > + return NULL; > + } > + > + b = __pci_create_root_bus(parent, domain, window->res->start, > + ops, sysdata, resources); > + if (!b) > + return NULL; > + > + dev_info(&b->dev, > + "No busn resource found for root bus, will use [bus %02x-ff]\n", > + bus); > + pci_bus_insert_busn_res(b, bus, 255); > + } > + > + max = pci_scan_child_bus(b); > + > + pci_bus_add_devices(b); > + return b; > +} > /* Deprecated; use pci_scan_root_bus() instead */ > struct pci_bus *pci_scan_bus_parented(struct device *parent, > int bus, struct pci_ops *ops, void *sysdata) > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 1e26fc6..734c016 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -395,6 +395,7 @@ struct pci_host_bridge_window { > > struct pci_host_bridge { > struct device dev; > + int domain; > struct pci_bus *bus; /* root bus */ > struct list_head windows; /* pci_host_bridge_windows */ > void (*release_fn)(struct pci_host_bridge *); > Yes, I think this will work, as long as we can figure out if the intent of the code is corect. Thanks, Liviu -- 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
On Wednesday 19 February 2014 11:37:55 Liviu Dudau wrote: > > > Right, this needs to be changed when moving the domain into pci_host_bridge. > > Yes, if only I understood what the intent was here. Unfortunately, the history > of the file stops (for my git tree) at Linus' first commit from v2.6.0, so > I need to dig deeper. What I am looking to understand is the answer to this > question: "When you want to create a new bus and you want to make sure it > is not duplicated, do you go across all domains or pick the current domain > and allow for same bus number in different domains?" I suspect this is for some large IA64 machines where you can see the same domain at different addresses, depending on which CPU you are on, or something similarly fancy. Another explanation would be that you can have different ways to describe the same hardware: On a PC you can assume that the PCI bus is discoverable on I/O port 0x0cf8. At the same time, you may see it in ACPI with an mmconfig method. In any case, you definitely need to allow the same bus numbers on each domain, since the most important use case of domains is to get around the 256 bus limit. > To give some context: I'm looking at drivers/pci/probe.c, pci_create_root_bus(). > Specifically, this code: > > b = pci_alloc_bus(); > if (!b) > return NULL; > > b->sysdata = sysdata; > b->ops = ops; > b->number = b->busn_res.start = bus; > b2 = pci_find_bus(pci_domain_nr(b), bus); > if (b2) { > /* If we already got to this bus through a different bridge, ignore it */ > dev_dbg(&b2->dev, "bus already known\n"); > goto err_out; > } > > bridge = pci_alloc_host_bridge(b); > if (!bridge) > goto err_out; > > > If you look at the comment after pci_find_bus, you can see that the intent was > to try to find duplicate busses covered by different bridges (but in the same > domain? maybe, because there used to be only one domain at the beginning, but > then why use pci_domain_nr() here?). Problems I see here: Definitely duplicate buses in the same domain, as mentioned above. > - code is trying to get the domain number of a bus that it just created but not > plugged yet in the hierarchy The sysdata is already assigned, and all architectures either assume there is only one domain, or they get the domain number out of the sysdata that is filled out prior to calling pci_create_root_bus. > - pci_find_bus tries to find a bus in the global list of busses that has the > same domain number and bus number as the parameters of the function. But > the domain number was plucked out of thin air and all architectures seem to > actually give you the current active domain number, not the one that actually > covers the bus (and host_bridge) that you are trying to create. Not sure what you mean with 'current active domain number': They clearly look at b->sysdata here, which is a local structure describing what we are currently probing. 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
On Wed, Feb 19, 2014 at 02:26:10PM +0100, Arnd Bergmann wrote: > On Wednesday 19 February 2014 11:37:55 Liviu Dudau wrote: > > > > > Right, this needs to be changed when moving the domain into pci_host_bridge. > > > > Yes, if only I understood what the intent was here. Unfortunately, the history > > of the file stops (for my git tree) at Linus' first commit from v2.6.0, so > > I need to dig deeper. What I am looking to understand is the answer to this > > question: "When you want to create a new bus and you want to make sure it > > is not duplicated, do you go across all domains or pick the current domain > > and allow for same bus number in different domains?" > > I suspect this is for some large IA64 machines where you can see the same > domain at different addresses, depending on which CPU you are on, or > something similarly fancy. > > Another explanation would be that you can have different ways to describe > the same hardware: On a PC you can assume that the PCI bus is discoverable > on I/O port 0x0cf8. At the same time, you may see it in ACPI with an > mmconfig method. > > In any case, you definitely need to allow the same bus numbers on each domain, > since the most important use case of domains is to get around the 256 bus > limit. > > > To give some context: I'm looking at drivers/pci/probe.c, pci_create_root_bus(). > > Specifically, this code: > > > > b = pci_alloc_bus(); > > if (!b) > > return NULL; > > > > b->sysdata = sysdata; > > b->ops = ops; > > b->number = b->busn_res.start = bus; > > b2 = pci_find_bus(pci_domain_nr(b), bus); > > if (b2) { > > /* If we already got to this bus through a different bridge, ignore it */ > > dev_dbg(&b2->dev, "bus already known\n"); > > goto err_out; > > } > > > > bridge = pci_alloc_host_bridge(b); > > if (!bridge) > > goto err_out; > > > > > > If you look at the comment after pci_find_bus, you can see that the intent was > > to try to find duplicate busses covered by different bridges (but in the same > > domain? maybe, because there used to be only one domain at the beginning, but > > then why use pci_domain_nr() here?). Problems I see here: > > Definitely duplicate buses in the same domain, as mentioned above. > > > - code is trying to get the domain number of a bus that it just created but not > > plugged yet in the hierarchy > > The sysdata is already assigned, and all architectures either assume there is > only one domain, or they get the domain number out of the sysdata that is > filled out prior to calling pci_create_root_bus. Arnd, the question is not how it does it, the question is what is the correct answer? It's a new bus, does that mean that all new busses get created in the current domain? If that is the case, then fine, things start to make more sense, with the caveat that the domain # depends heavily on what the architecture uses for counting. > > > - pci_find_bus tries to find a bus in the global list of busses that has the > > same domain number and bus number as the parameters of the function. But > > the domain number was plucked out of thin air and all architectures seem to > > actually give you the current active domain number, not the one that actually > > covers the bus (and host_bridge) that you are trying to create. > > Not sure what you mean with 'current active domain number': They clearly look at > b->sysdata here, which is a local structure describing what we are currently > probing. Yes, but that definition of locality is not common to all architectures. arm for example stores it in the domain member of pci_sys_data, but that gets initialised every time someone calls pcibios_init_hw (or pci_common_init_dev) with the domain value held in struct hw_pci. But the host controllers don't talk to each other, so if you want to instantiate two different drivers that use hw_pci they likely use an individual count that starts at zero. Anyway, I don't think that using your suggestion is going to be a problem, so once I get out of this god forsaken task of bringing up a board I will post an updated patch. Best regards, Liviu > > Arnd >
On Wednesday 19 February 2014 15:30:44 Liviu Dudau wrote: > On Wed, Feb 19, 2014 at 02:26:10PM +0100, Arnd Bergmann wrote: > > On Wednesday 19 February 2014 11:37:55 Liviu Dudau wrote: > > > - code is trying to get the domain number of a bus that it just created but not > > > plugged yet in the hierarchy > > > > The sysdata is already assigned, and all architectures either assume there is > > only one domain, or they get the domain number out of the sysdata that is > > filled out prior to calling pci_create_root_bus. > > Arnd, the question is not how it does it, the question is what is the correct answer? > It's a new bus, does that mean that all new busses get created in the current domain? > If that is the case, then fine, things start to make more sense, with the caveat > that the domain # depends heavily on what the architecture uses for counting. Here is what each architecture does: Alpha: managed by host bridge driver, counting the hosts or fixed in hardware ARM: managed in host bridge driver, counting the hosts or using just one domain IA64: taken from firmware (either ACPI or SAL) microblaze: managed globally, counting the hosts powerpc: managed globally, counting the hosts s390: each PCI function has its own domain (!) sh: managed in host bridge driver, counting the hosts sparc: managed by host bridge driger, counting the hosts tile: managed globally, counting the hosts x86: managed by ACPI firmware In each case, we either know the domain from hardware or firmware when creating the host bridge, or we pick the next free number. > > > - pci_find_bus tries to find a bus in the global list of busses that has the > > > same domain number and bus number as the parameters of the function. But > > > the domain number was plucked out of thin air and all architectures seem to > > > actually give you the current active domain number, not the one that actually > > > covers the bus (and host_bridge) that you are trying to create. > > > > Not sure what you mean with 'current active domain number': They clearly look at > > b->sysdata here, which is a local structure describing what we are currently > > probing. > > Yes, but that definition of locality is not common to all architectures. arm for > example stores it in the domain member of pci_sys_data, but that gets initialised > every time someone calls pcibios_init_hw (or pci_common_init_dev) with the domain > value held in struct hw_pci. But the host controllers don't talk to each other, > so if you want to instantiate two different drivers that use hw_pci they likely > use an individual count that starts at zero. The assumption is that whoever calls pci_common_init{_dev} knows about all the PCI hosts in the system. As Jason Gunthorpe said, it would be desirable to have stable domain numbers, so it's probably best to take the number from DT (e.g. the aliases node). It's probably safe to assume that any machine using ATAGS for booting falls into the category where it's safe to keep the policy local in the host bridge driver. 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
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 5094943..9f2ec2f 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1697,8 +1697,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) +static struct pci_bus *__pci_create_root_bus(struct device *parent, + int domain, int bus, struct pci_ops *ops, void *sysdata, + struct list_head *resources) { int error; struct pci_host_bridge *bridge; @@ -1716,7 +1717,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, 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(domain, bus); if (b2) { /* If we already got to this bus through a different bridge, ignore it */ dev_dbg(&b2->dev, "bus already known\n"); @@ -1727,6 +1728,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, if (!bridge) goto err_out; + bridge->domain = domain; bridge->dev.parent = parent; bridge->dev.release = pci_release_host_bridge_dev; dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); @@ -1801,6 +1803,13 @@ err_out: return NULL; } +struct pci_bus *pci_create_root_bus(struct device *parent, int bus, + struct pci_ops *ops, void *sysdata, struct list_head *resources) +{ + return __pci_create_root_bus(parent, pci_domain_nr(bus), bus, + ops, sysdata, resources); +} + int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) { struct resource *res = &b->busn_res; @@ -1899,6 +1908,42 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus, } EXPORT_SYMBOL(pci_scan_root_bus); +struct pci_bus *pci_scan_domain(struct device *parent, int domain, + struct pci_ops *ops, void *sysdata, struct list_head *resources) +{ + struct pci_host_bridge_window *window; + bool found = false; + struct pci_bus *b; + int max; + + list_for_each_entry(window, resources, list) + if (window->res->flags & IORESOURCE_BUS) { + found = true; + break; + } + + if (!found) { + dev_info(&b->dev, + "No busn resource found for domain %d\n", domain); + return NULL; + } + + b = __pci_create_root_bus(parent, domain, window->res->start, + ops, sysdata, resources); + if (!b) + return NULL; + + dev_info(&b->dev, + "No busn resource found for root bus, will use [bus %02x-ff]\n", + bus); + pci_bus_insert_busn_res(b, bus, 255); + } + + max = pci_scan_child_bus(b); + + pci_bus_add_devices(b); + return b; +} /* Deprecated; use pci_scan_root_bus() instead */ struct pci_bus *pci_scan_bus_parented(struct device *parent, int bus, struct pci_ops *ops, void *sysdata) diff --git a/include/linux/pci.h b/include/linux/pci.h index 1e26fc6..734c016 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -395,6 +395,7 @@ struct pci_host_bridge_window { struct pci_host_bridge { struct device dev; + int domain; struct pci_bus *bus; /* root bus */ struct list_head windows; /* pci_host_bridge_windows */ void (*release_fn)(struct pci_host_bridge *);