| Message ID | 1425868467-9667-11-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 09, 2015 at 10:34:07AM +0800, Yijing Wang wrote: > Introduce pci_host_bridge_list to manage pci host > bridges in system, so we could detect whether > the host in domain:bus is alreay registered. > Then we could remove bus alreay exist test in > __pci_create_root_bus(). It's a nice idea to move this test into the core. While you're at it, why don't you check for any overlap with the bus ranges of existing host bridges? For example, if we're trying to create a new host bridge to [bus 40-7f], it should conflict with existing bridges to [bus 00-7f] as well as to [bus 40-ff]. I think your current patch will detect the latter conflict but not the former. > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/host-bridge.c | 24 +++++++++++++++++++++++- > drivers/pci/probe.c | 8 +------- > include/linux/pci.h | 1 + > 3 files changed, 25 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > index 3bd45e7..0bb08ef 100644 > --- a/drivers/pci/host-bridge.c > +++ b/drivers/pci/host-bridge.c > @@ -8,6 +8,9 @@ > > #include "pci.h" > > +static LIST_HEAD(pci_host_bridge_list); > +static DEFINE_MUTEX(phb_mutex); We don't use the "phb" abbreviation in the PCI core. > + > static void pci_release_host_bridge_dev(struct device *dev) > { > struct pci_host_bridge *bridge = to_pci_host_bridge(dev); > @@ -25,7 +28,7 @@ struct pci_host_bridge *pci_create_host_bridge( > int error; > int bus = PCI_BUSNUM(db); > int domain = PCI_DOMAIN(db); > - struct pci_host_bridge *host; > + struct pci_host_bridge *host, *temp; > struct resource_entry *window, *n; > > host = kzalloc(sizeof(*host), GFP_KERNEL); > @@ -40,6 +43,18 @@ struct pci_host_bridge *pci_create_host_bridge( > */ > pci_host_assign_domain_nr(host); > > + mutex_lock(&phb_mutex); > + list_for_each_entry(temp, &pci_host_bridge_list, list) > + if (temp->domain == host->domain > + && temp->busnum == host->busnum) { > + dev_dbg(&host->dev, "pci host bridge pci%04x:%02x exist\n", > + host->domain, host->busnum); &host->dev hasn't been initialized yet! And the text of the original message was better. > + mutex_unlock(&phb_mutex); > + kfree(host); > + return NULL; > + } > + mutex_unlock(&phb_mutex); This is racy. Two CPUs adding a bridge to [bus 00-7f] at about the same time can both succeed. And it needs curly braces around the list_for_each_entry() body. > + > host->dev.parent = parent; > INIT_LIST_HEAD(&host->windows); > host->dev.release = pci_release_host_bridge_dev; > @@ -55,11 +70,18 @@ struct pci_host_bridge *pci_create_host_bridge( > resource_list_for_each_entry_safe(window, n, resources) > list_move_tail(&window->node, &host->windows); > > + mutex_lock(&phb_mutex); > + list_add_tail(&host->list, &pci_host_bridge_list); > + mutex_unlock(&phb_mutex); > return host; > } > > void pci_free_host_bridge(struct pci_host_bridge *host) > { > + mutex_lock(&phb_mutex); > + list_del(&host->list); > + mutex_unlock(&phb_mutex); > + > device_unregister(&host->dev); > } > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 27ec612..7238fa3 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1870,7 +1870,7 @@ static struct pci_bus *__pci_create_root_bus( > void *sysdata) > { > int error; > - struct pci_bus *b, *b2; > + struct pci_bus *b; > struct resource_entry *window; > struct device *parent; > struct resource *res; > @@ -1887,12 +1887,6 @@ static struct pci_bus *__pci_create_root_bus( > b->ops = ops; > b->number = b->busn_res.start = bridge->busnum; > pci_bus_assign_domain_nr(b, parent); > - b2 = pci_find_bus(pci_domain_nr(b), b->number); > - 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->bus = b; > b->bridge = get_device(&bridge->dev); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 463eaa3..b621f5b 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -406,6 +406,7 @@ struct pci_host_bridge { > struct device dev; > struct pci_bus *bus; /* root bus */ > struct list_head windows; /* resource_entry */ > + struct list_head list; > void (*release_fn)(struct pci_host_bridge *); > void *release_data; > }; > -- > 1.7.1 >
On 2015/3/12 10:55, Bjorn Helgaas wrote: > On Mon, Mar 09, 2015 at 10:34:07AM +0800, Yijing Wang wrote: >> Introduce pci_host_bridge_list to manage pci host >> bridges in system, so we could detect whether >> the host in domain:bus is alreay registered. >> Then we could remove bus alreay exist test in >> __pci_create_root_bus(). > > It's a nice idea to move this test into the core. While you're at it, why > don't you check for any overlap with the bus ranges of existing host > bridges? For example, if we're trying to create a new host bridge to > [bus 40-7f], it should conflict with existing bridges to [bus 00-7f] > as well as to [bus 40-ff]. I think your current patch will detect the > latter conflict but not the former. Now pci host bridge may only know its start bus number, like acpi _BBN provided, but does not limit the end bus number, Eg. two pci roots report _BBN 0x0 and 0x80, so we have two bus number resource (0, 0xff) and (0x80, 0xff), if we check it strictly, some pci scan would fail which currently scan success. > >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> --- >> drivers/pci/host-bridge.c | 24 +++++++++++++++++++++++- >> drivers/pci/probe.c | 8 +------- >> include/linux/pci.h | 1 + >> 3 files changed, 25 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c >> index 3bd45e7..0bb08ef 100644 >> --- a/drivers/pci/host-bridge.c >> +++ b/drivers/pci/host-bridge.c >> @@ -8,6 +8,9 @@ >> >> #include "pci.h" >> >> +static LIST_HEAD(pci_host_bridge_list); >> +static DEFINE_MUTEX(phb_mutex); > > We don't use the "phb" abbreviation in the PCI core. OK > >> + >> static void pci_release_host_bridge_dev(struct device *dev) >> { >> struct pci_host_bridge *bridge = to_pci_host_bridge(dev); >> @@ -25,7 +28,7 @@ struct pci_host_bridge *pci_create_host_bridge( >> int error; >> int bus = PCI_BUSNUM(db); >> int domain = PCI_DOMAIN(db); >> - struct pci_host_bridge *host; >> + struct pci_host_bridge *host, *temp; >> struct resource_entry *window, *n; >> >> host = kzalloc(sizeof(*host), GFP_KERNEL); >> @@ -40,6 +43,18 @@ struct pci_host_bridge *pci_create_host_bridge( >> */ >> pci_host_assign_domain_nr(host); >> >> + mutex_lock(&phb_mutex); >> + list_for_each_entry(temp, &pci_host_bridge_list, list) >> + if (temp->domain == host->domain >> + && temp->busnum == host->busnum) { >> + dev_dbg(&host->dev, "pci host bridge pci%04x:%02x exist\n", >> + host->domain, host->busnum); > > &host->dev hasn't been initialized yet! And the text of the original > message was better. Oh, my mistake, good catch. > >> + mutex_unlock(&phb_mutex); >> + kfree(host); >> + return NULL; >> + } >> + mutex_unlock(&phb_mutex); > > This is racy. Two CPUs adding a bridge to [bus 00-7f] at about the same > time can both succeed. > Hmm, I need to unlock after I add the new host bridge to the gloal host bridge list, thanks. > And it needs curly braces around the list_for_each_entry() body. OK > >> + >> host->dev.parent = parent; >> INIT_LIST_HEAD(&host->windows); >> host->dev.release = pci_release_host_bridge_dev; >> @@ -55,11 +70,18 @@ struct pci_host_bridge *pci_create_host_bridge( >> resource_list_for_each_entry_safe(window, n, resources) >> list_move_tail(&window->node, &host->windows); >> >> + mutex_lock(&phb_mutex); >> + list_add_tail(&host->list, &pci_host_bridge_list); >> + mutex_unlock(&phb_mutex); >> return host; >> } >> >> void pci_free_host_bridge(struct pci_host_bridge *host) >> { >> + mutex_lock(&phb_mutex); >> + list_del(&host->list); >> + mutex_unlock(&phb_mutex); >> + >> device_unregister(&host->dev); >> } >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 27ec612..7238fa3 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1870,7 +1870,7 @@ static struct pci_bus *__pci_create_root_bus( >> void *sysdata) >> { >> int error; >> - struct pci_bus *b, *b2; >> + struct pci_bus *b; >> struct resource_entry *window; >> struct device *parent; >> struct resource *res; >> @@ -1887,12 +1887,6 @@ static struct pci_bus *__pci_create_root_bus( >> b->ops = ops; >> b->number = b->busn_res.start = bridge->busnum; >> pci_bus_assign_domain_nr(b, parent); >> - b2 = pci_find_bus(pci_domain_nr(b), b->number); >> - 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->bus = b; >> b->bridge = get_device(&bridge->dev); >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 463eaa3..b621f5b 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -406,6 +406,7 @@ struct pci_host_bridge { >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> struct list_head windows; /* resource_entry */ >> + struct list_head list; >> void (*release_fn)(struct pci_host_bridge *); >> void *release_data; >> }; >> -- >> 1.7.1 >> > > . >
On Thu, Mar 12, 2015 at 09:03:12PM +0800, Yijing Wang wrote: > On 2015/3/12 10:55, Bjorn Helgaas wrote: > > On Mon, Mar 09, 2015 at 10:34:07AM +0800, Yijing Wang wrote: > >> Introduce pci_host_bridge_list to manage pci host > >> bridges in system, so we could detect whether > >> the host in domain:bus is alreay registered. > >> Then we could remove bus alreay exist test in > >> __pci_create_root_bus(). > > > > It's a nice idea to move this test into the core. While you're at it, why > > don't you check for any overlap with the bus ranges of existing host > > bridges? For example, if we're trying to create a new host bridge to > > [bus 40-7f], it should conflict with existing bridges to [bus 00-7f] > > as well as to [bus 40-ff]. I think your current patch will detect the > > latter conflict but not the former. > > Now pci host bridge may only know its start bus number, like acpi _BBN provided, > but does not limit the end bus number, Eg. two pci roots report _BBN 0x0 and 0x80, > so we have two bus number resource (0, 0xff) and (0x80, 0xff), if we check it strictly, > some pci scan would fail which currently scan success. _BBN is not the correct source for the bridge's bus number range. There's a comment in acpi_pci_root_add() that explains why: * We need both the start and end of the downstream bus range * to interpret _CBA (MMCONFIG base address), so it really is * supposed to be in _CRS. If we don't find it there, all we * can do is assume [_BBN-0xFF] or [0-0xFF]. A platform SHOULD know the start and and end bus number. If it doesn't I think it's the platform's responsibility to carve up the bus number range. Maybe this can be done by trimming the range of the [bus 00-ff] bridge when we discover another bridge that leads to bus 80.
>>> It's a nice idea to move this test into the core. While you're at it, why >>> don't you check for any overlap with the bus ranges of existing host >>> bridges? For example, if we're trying to create a new host bridge to >>> [bus 40-7f], it should conflict with existing bridges to [bus 00-7f] >>> as well as to [bus 40-ff]. I think your current patch will detect the >>> latter conflict but not the former. >> >> Now pci host bridge may only know its start bus number, like acpi _BBN provided, >> but does not limit the end bus number, Eg. two pci roots report _BBN 0x0 and 0x80, >> so we have two bus number resource (0, 0xff) and (0x80, 0xff), if we check it strictly, >> some pci scan would fail which currently scan success. > > _BBN is not the correct source for the bridge's bus number range. There's > a comment in acpi_pci_root_add() that explains why: > > * We need both the start and end of the downstream bus range > * to interpret _CBA (MMCONFIG base address), so it really is > * supposed to be in _CRS. If we don't find it there, all we > * can do is assume [_BBN-0xFF] or [0-0xFF]. > > A platform SHOULD know the start and and end bus number. If it doesn't I > think it's the platform's responsibility to carve up the bus number range. > Maybe this can be done by trimming the range of the [bus 00-ff] bridge when > we discover another bridge that leads to bus 80. Currently, if platform does not know the end bus number (not provide the bus resource), we will update the max bus number returned by pci_scan_child_bus() to the bus resource end, and I think this is reasonable. I consider to introduce a flag to identify the bus resource which end bus number is undefined, then we could force all pci_scan_root_bus() etc. callers to provide the bus resource, and we could process the bus resource end according the bus resource flag. Also then we could reduce the bus argument which is the same as busn_res->start. I would try to provide draft patch, then we could discuss it more clearly. Thanks! Yijing. > > . >
On Thu, Mar 12, 2015 at 10:28 PM, Yijing Wang <wangyijing@huawei.com> wrote: >>>> It's a nice idea to move this test into the core. While you're at it, why >>>> don't you check for any overlap with the bus ranges of existing host >>>> bridges? For example, if we're trying to create a new host bridge to >>>> [bus 40-7f], it should conflict with existing bridges to [bus 00-7f] >>>> as well as to [bus 40-ff]. I think your current patch will detect the >>>> latter conflict but not the former. >>> >>> Now pci host bridge may only know its start bus number, like acpi _BBN provided, >>> but does not limit the end bus number, Eg. two pci roots report _BBN 0x0 and 0x80, >>> so we have two bus number resource (0, 0xff) and (0x80, 0xff), if we check it strictly, >>> some pci scan would fail which currently scan success. >> >> _BBN is not the correct source for the bridge's bus number range. There's >> a comment in acpi_pci_root_add() that explains why: >> >> * We need both the start and end of the downstream bus range >> * to interpret _CBA (MMCONFIG base address), so it really is >> * supposed to be in _CRS. If we don't find it there, all we >> * can do is assume [_BBN-0xFF] or [0-0xFF]. >> >> A platform SHOULD know the start and and end bus number. If it doesn't I >> think it's the platform's responsibility to carve up the bus number range. >> Maybe this can be done by trimming the range of the [bus 00-ff] bridge when >> we discover another bridge that leads to bus 80. > > Currently, if platform does not know the end bus number (not provide the bus resource), > we will update the max bus number returned by pci_scan_child_bus() to the bus resource end, > and I think this is reasonable. I consider to introduce a flag to identify the bus resource > which end bus number is undefined, then we could force all pci_scan_root_bus() etc. callers > to provide the bus resource, and we could process the bus resource end according the bus resource flag. > Also then we could reduce the bus argument which is the same as busn_res->start. > I would try to provide draft patch, then we could discuss it more clearly. Without having seen a patch, my inclination is to avoid a flag because flags change the behavior of the code you call, which makes that code harder to follow. Maybe we could require these platforms to explicitly update the ending bus number after scanning the bus. Bjorn
>> Currently, if platform does not know the end bus number (not provide the bus resource), >> we will update the max bus number returned by pci_scan_child_bus() to the bus resource end, >> and I think this is reasonable. I consider to introduce a flag to identify the bus resource >> which end bus number is undefined, then we could force all pci_scan_root_bus() etc. callers >> to provide the bus resource, and we could process the bus resource end according the bus resource flag. >> Also then we could reduce the bus argument which is the same as busn_res->start. >> I would try to provide draft patch, then we could discuss it more clearly. > > Without having seen a patch, my inclination is to avoid a flag because > flags change the behavior of the code you call, which makes that code > harder to follow. Maybe we could require these platforms to > explicitly update the ending bus number after scanning the bus. OK, agree, thanks for your suggestion :) > > Bjorn > > . >
diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c index 3bd45e7..0bb08ef 100644 --- a/drivers/pci/host-bridge.c +++ b/drivers/pci/host-bridge.c @@ -8,6 +8,9 @@ #include "pci.h" +static LIST_HEAD(pci_host_bridge_list); +static DEFINE_MUTEX(phb_mutex); + static void pci_release_host_bridge_dev(struct device *dev) { struct pci_host_bridge *bridge = to_pci_host_bridge(dev); @@ -25,7 +28,7 @@ struct pci_host_bridge *pci_create_host_bridge( int error; int bus = PCI_BUSNUM(db); int domain = PCI_DOMAIN(db); - struct pci_host_bridge *host; + struct pci_host_bridge *host, *temp; struct resource_entry *window, *n; host = kzalloc(sizeof(*host), GFP_KERNEL); @@ -40,6 +43,18 @@ struct pci_host_bridge *pci_create_host_bridge( */ pci_host_assign_domain_nr(host); + mutex_lock(&phb_mutex); + list_for_each_entry(temp, &pci_host_bridge_list, list) + if (temp->domain == host->domain + && temp->busnum == host->busnum) { + dev_dbg(&host->dev, "pci host bridge pci%04x:%02x exist\n", + host->domain, host->busnum); + mutex_unlock(&phb_mutex); + kfree(host); + return NULL; + } + mutex_unlock(&phb_mutex); + host->dev.parent = parent; INIT_LIST_HEAD(&host->windows); host->dev.release = pci_release_host_bridge_dev; @@ -55,11 +70,18 @@ struct pci_host_bridge *pci_create_host_bridge( resource_list_for_each_entry_safe(window, n, resources) list_move_tail(&window->node, &host->windows); + mutex_lock(&phb_mutex); + list_add_tail(&host->list, &pci_host_bridge_list); + mutex_unlock(&phb_mutex); return host; } void pci_free_host_bridge(struct pci_host_bridge *host) { + mutex_lock(&phb_mutex); + list_del(&host->list); + mutex_unlock(&phb_mutex); + device_unregister(&host->dev); } diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 27ec612..7238fa3 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1870,7 +1870,7 @@ static struct pci_bus *__pci_create_root_bus( void *sysdata) { int error; - struct pci_bus *b, *b2; + struct pci_bus *b; struct resource_entry *window; struct device *parent; struct resource *res; @@ -1887,12 +1887,6 @@ static struct pci_bus *__pci_create_root_bus( b->ops = ops; b->number = b->busn_res.start = bridge->busnum; pci_bus_assign_domain_nr(b, parent); - b2 = pci_find_bus(pci_domain_nr(b), b->number); - 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->bus = b; b->bridge = get_device(&bridge->dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index 463eaa3..b621f5b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -406,6 +406,7 @@ struct pci_host_bridge { struct device dev; struct pci_bus *bus; /* root bus */ struct list_head windows; /* resource_entry */ + struct list_head list; void (*release_fn)(struct pci_host_bridge *); void *release_data; };