| Message ID | 1410732627-25445-1-git-send-email-andreas.noever@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Mon, Sep 15, 2014 at 12:10:27AM +0200, Andreas Noever wrote: >[+cc Thomas, your regression has probably the same cause] > >This looks a like it is going to be a little more complicated than anticipated. > >pci_scan_bus has two main branches. One for "broken" hardware and cardbus and >another one for bridges whose configuration looks sane. > >David's working 3.2 Kernel does the following: >pci 0000:00:1e.0: PCI bridge to [bus 01-01] (subtractive decode) >pci 0000:01:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring > >The 00:1e bridge is deemed sane and the 01:01 (cardbus) bridge below it is >deemed broken. pci_scan_bridge than assigns bus [2-5] to 01:01 without touching >1e (so we just got 4 imaginary busses). We just print a warning: >pci_bus 0000:02: [bus 02-05] partially hidden behind transparent bridge 0000:01 [bus 01-01] > >After returning to 00:1e (in the sane branch) we also do not update our >subordinate and just return. Later yenta_socket sees that this is nuts and >carefully increases the subordinate of 00:1e. > >My patch series for 3.15 was trying to make sure that pci_scan_bus does not >overstep its resources. So now we now refuse to create bus 2 under [01-01] >(fc1b2531 PCI: Don't scan random busses in pci_scan_bridge()) and a little >later we would forbid the cardbus code in pci_scan_bridge from reserving 4 more >busses (1820ffdc PCI: Make sure bus number resources stay within their parents >bounds). At least these two would need to be reverted. > >As an alternative the following patch tries grow the bus window, if necessary >by growing its parents bus window (recursively). This should make the yenta >fixup unnecessary. I have tested the patch with normal pci bridges + setpci, >since I don't have access to a cardbus system. > >So if you have time please test this and/or try to revert the two mentioned >commits. > >Thanks, >Andreas >--- > drivers/pci/probe.c | 45 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 36 insertions(+), 9 deletions(-) > >diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >index e3cf8a2..81dd668 100644 >--- a/drivers/pci/probe.c >+++ b/drivers/pci/probe.c >@@ -740,6 +740,30 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, > } > EXPORT_SYMBOL(pci_add_new_bus); > >+int pci_grow_bus(struct pci_bus *bus, int bus_max) >+{ >+ struct resource *res = &bus->busn_res; >+ struct resource old_res = *res; >+ int ret; >+ if (res->end >= bus_max) >+ return 0; >+ if (!bus->self || pci_is_root_bus(bus)) { >+ dev_printk(KERN_DEBUG, &bus->dev, >+ "root busn_res %pR cannot grow\n", &res); >+ return -EBUSY; >+ } >+ ret = pci_grow_bus(bus->parent, bus_max); >+ if (ret) >+ return ret; >+ ret = adjust_resource(res, res->start, bus_max - res->start + 1); >+ dev_printk(KERN_DEBUG, &bus->dev, >+ "busn_res: %pR end %s grown to %02x\n", >+ &old_res, ret ? "cannot be" : "has", bus_max); >+ If adjust_resource fails, I think we can't write the bus_max into the bridge register. >+ pci_write_config_byte(bus->self, PCI_SUBORDINATE_BUS, bus_max); >+ return ret; >+} >+ > /* > * If it's a bridge, configure it and scan the bus behind it. > * For CardBus bridges, we don't scan behind as the devices will >@@ -814,12 +838,11 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > } > > cmax = pci_scan_child_bus(child); >- if (cmax > subordinate) >- dev_warn(&dev->dev, "bridge has subordinate %02x but max busn %02x\n", >- subordinate, cmax); >- /* subordinate should equal child->busn_res.end */ >- if (subordinate > max) >- max = subordinate; >+ if (cmax > child->busn_res.end) >+ dev_warn(&dev->dev, "bridge has bus resource %pR but max busn %02x\n", >+ &child->busn_res, cmax); >+ if (child->busn_res.end > max) >+ max = child->busn_res.end; By a quick look, I don't see some place change the busn_res.end. And in the pci_bus_insert_busn_res(), the child->busn_res.end is set to subordinate. So I don't see the reason you change it. Do I miss something? > } else { > /* > * We need to assign a number to this bus which we always >@@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > if (max >= bus->busn_res.end) { > dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", >- max, &bus->busn_res); >- goto out; >+ max + 1, &bus->busn_res); >+ /* Try to resize bus */ >+ if (pci_grow_bus(bus, max + 1)) >+ goto out; On some platforms, like powerpc, we have some limitations of the bus number a bridge could have. Sometimes, we need the start bus number to be power 2 aligned. Hmm... hope I don't make a mistake, add Gavin in CC list. Gavin, is my understanding correct? > } > > /* Clear errors */ >@@ -908,6 +933,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > break; > } > } >+ dev_info(&dev->dev, "adding %02x bus numbers for cardbus\n", i); > max += i; > } > /* >@@ -916,7 +942,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > if (max > bus->busn_res.end) { > dev_warn(&dev->dev, "max busn %02x is outside %pR\n", > max, &bus->busn_res); >- max = bus->busn_res.end; >+ if (pci_grow_bus(bus, max)) >+ max = bus->busn_res.end; > } > pci_bus_update_busn_res_end(child, max); > pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max); >-- >2.1.0 > >-- >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 Mon, Sep 15, 2014 at 11:53 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: > On Mon, Sep 15, 2014 at 12:10:27AM +0200, Andreas Noever wrote: >>[+cc Thomas, your regression has probably the same cause] >> >>This looks a like it is going to be a little more complicated than anticipated. >> >>pci_scan_bus has two main branches. One for "broken" hardware and cardbus and >>another one for bridges whose configuration looks sane. >> >>David's working 3.2 Kernel does the following: >>pci 0000:00:1e.0: PCI bridge to [bus 01-01] (subtractive decode) >>pci 0000:01:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring >> >>The 00:1e bridge is deemed sane and the 01:01 (cardbus) bridge below it is >>deemed broken. pci_scan_bridge than assigns bus [2-5] to 01:01 without touching >>1e (so we just got 4 imaginary busses). We just print a warning: >>pci_bus 0000:02: [bus 02-05] partially hidden behind transparent bridge 0000:01 [bus 01-01] >> >>After returning to 00:1e (in the sane branch) we also do not update our >>subordinate and just return. Later yenta_socket sees that this is nuts and >>carefully increases the subordinate of 00:1e. >> >>My patch series for 3.15 was trying to make sure that pci_scan_bus does not >>overstep its resources. So now we now refuse to create bus 2 under [01-01] >>(fc1b2531 PCI: Don't scan random busses in pci_scan_bridge()) and a little >>later we would forbid the cardbus code in pci_scan_bridge from reserving 4 more >>busses (1820ffdc PCI: Make sure bus number resources stay within their parents >>bounds). At least these two would need to be reverted. >> >>As an alternative the following patch tries grow the bus window, if necessary >>by growing its parents bus window (recursively). This should make the yenta >>fixup unnecessary. I have tested the patch with normal pci bridges + setpci, >>since I don't have access to a cardbus system. >> >>So if you have time please test this and/or try to revert the two mentioned >>commits. >> >>Thanks, >>Andreas >>--- >> drivers/pci/probe.c | 45 ++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 36 insertions(+), 9 deletions(-) >> >>diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>index e3cf8a2..81dd668 100644 >>--- a/drivers/pci/probe.c >>+++ b/drivers/pci/probe.c >>@@ -740,6 +740,30 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >> } >> EXPORT_SYMBOL(pci_add_new_bus); >> >>+int pci_grow_bus(struct pci_bus *bus, int bus_max) >>+{ >>+ struct resource *res = &bus->busn_res; >>+ struct resource old_res = *res; >>+ int ret; >>+ if (res->end >= bus_max) >>+ return 0; >>+ if (!bus->self || pci_is_root_bus(bus)) { >>+ dev_printk(KERN_DEBUG, &bus->dev, >>+ "root busn_res %pR cannot grow\n", &res); >>+ return -EBUSY; >>+ } >>+ ret = pci_grow_bus(bus->parent, bus_max); >>+ if (ret) >>+ return ret; >>+ ret = adjust_resource(res, res->start, bus_max - res->start + 1); >>+ dev_printk(KERN_DEBUG, &bus->dev, >>+ "busn_res: %pR end %s grown to %02x\n", >>+ &old_res, ret ? "cannot be" : "has", bus_max); >>+ > > If adjust_resource fails, I think we can't write the bus_max into the bridge > register. Right, there is an if statement missing. Thanks. >>+ pci_write_config_byte(bus->self, PCI_SUBORDINATE_BUS, bus_max); >>+ return ret; >>+} >>+ >> /* >> * If it's a bridge, configure it and scan the bus behind it. >> * For CardBus bridges, we don't scan behind as the devices will >>@@ -814,12 +838,11 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >> } >> >> cmax = pci_scan_child_bus(child); >>- if (cmax > subordinate) >>- dev_warn(&dev->dev, "bridge has subordinate %02x but max busn %02x\n", >>- subordinate, cmax); >>- /* subordinate should equal child->busn_res.end */ >>- if (subordinate > max) >>- max = subordinate; >>+ if (cmax > child->busn_res.end) >>+ dev_warn(&dev->dev, "bridge has bus resource %pR but max busn %02x\n", >>+ &child->busn_res, cmax); >>+ if (child->busn_res.end > max) >>+ max = child->busn_res.end; > > By a quick look, I don't see some place change the busn_res.end. And in the > pci_bus_insert_busn_res(), the child->busn_res.end is set to subordinate. So I > don't see the reason you change it. adjust_resource in pci_grow_bus changes busn_res.end. The value in 'subordinate' is then stale at this point. > > Do I miss something? > >> } else { >> /* >> * We need to assign a number to this bus which we always >>@@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >> >> if (max >= bus->busn_res.end) { >> dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", >>- max, &bus->busn_res); >>- goto out; >>+ max + 1, &bus->busn_res); >>+ /* Try to resize bus */ >>+ if (pci_grow_bus(bus, max + 1)) >>+ goto out; > > On some platforms, like powerpc, we have some limitations of the bus number a > bridge could have. Sometimes, we need the start bus number to be power 2 > aligned. > > Hmm... hope I don't make a mistake, add Gavin in CC list. Gavin, is my > understanding correct? Hm I have not seen any code that would enforce this. Is it possible to do pci=assign-busses on PowerPC? >> } >> >> /* Clear errors */ >>@@ -908,6 +933,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >> break; >> } >> } >>+ dev_info(&dev->dev, "adding %02x bus numbers for cardbus\n", i); >> max += i; >> } >> /* >>@@ -916,7 +942,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >> if (max > bus->busn_res.end) { >> dev_warn(&dev->dev, "max busn %02x is outside %pR\n", >> max, &bus->busn_res); >>- max = bus->busn_res.end; >>+ if (pci_grow_bus(bus, max)) >>+ max = bus->busn_res.end; >> } >> pci_bus_update_busn_res_end(child, max); >> pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max); >>-- >>2.1.0 >> >>-- >>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 > > -- > Richard Yang > Help you, Help me > -- 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 Mon, Sep 15, 2014 at 05:53:05PM +0800, Wei Yang wrote: > On Mon, Sep 15, 2014 at 12:10:27AM +0200, Andreas Noever wrote: > ... > >@@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > > > if (max >= bus->busn_res.end) { > > dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", > >- max, &bus->busn_res); > >- goto out; > >+ max + 1, &bus->busn_res); > >+ /* Try to resize bus */ > >+ if (pci_grow_bus(bus, max + 1)) > >+ goto out; > > On some platforms, like powerpc, we have some limitations of the bus number a > bridge could have. Sometimes, we need the start bus number to be power 2 > aligned. Huh. I have to admit that I'm getting tired of all the powerpc-specific PCI hacks. It's hard enough to get this stuff working on hardware that conforms to the spec, and scattering pcibios_*() hooks around makes the code even harder to follow. What would happen if powerpc used PCI_PROBE_ONLY? Do you really depend on the PCI core to configure anything for you, or does your firmware set everything up the way it needs to be? Changing bridge configuration seems like something we should avoid under PCI_PROBE_ONLY (I haven't read Andreas' patch in detail, so I don't know if that's how it works). If PCI_PROBE_ONLY would work for powerpc, then we wouldn't have an issue here. Bjorn -- 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 Mon, Sep 15, 2014 at 12:04:22PM +0200, Andreas Noever wrote: >On Mon, Sep 15, 2014 at 11:53 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: >> On Mon, Sep 15, 2014 at 12:10:27AM +0200, Andreas Noever wrote: >>>[+cc Thomas, your regression has probably the same cause] >>> >>>This looks a like it is going to be a little more complicated than anticipated. >>> >>>pci_scan_bus has two main branches. One for "broken" hardware and cardbus and >>>another one for bridges whose configuration looks sane. >>> >>>David's working 3.2 Kernel does the following: >>>pci 0000:00:1e.0: PCI bridge to [bus 01-01] (subtractive decode) >>>pci 0000:01:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring >>> >>>The 00:1e bridge is deemed sane and the 01:01 (cardbus) bridge below it is >>>deemed broken. pci_scan_bridge than assigns bus [2-5] to 01:01 without touching >>>1e (so we just got 4 imaginary busses). We just print a warning: >>>pci_bus 0000:02: [bus 02-05] partially hidden behind transparent bridge 0000:01 [bus 01-01] >>> >>>After returning to 00:1e (in the sane branch) we also do not update our >>>subordinate and just return. Later yenta_socket sees that this is nuts and >>>carefully increases the subordinate of 00:1e. >>> >>>My patch series for 3.15 was trying to make sure that pci_scan_bus does not >>>overstep its resources. So now we now refuse to create bus 2 under [01-01] >>>(fc1b2531 PCI: Don't scan random busses in pci_scan_bridge()) and a little >>>later we would forbid the cardbus code in pci_scan_bridge from reserving 4 more >>>busses (1820ffdc PCI: Make sure bus number resources stay within their parents >>>bounds). At least these two would need to be reverted. >>> >>>As an alternative the following patch tries grow the bus window, if necessary >>>by growing its parents bus window (recursively). This should make the yenta >>>fixup unnecessary. I have tested the patch with normal pci bridges + setpci, >>>since I don't have access to a cardbus system. >>> >>>So if you have time please test this and/or try to revert the two mentioned >>>commits. >>> >>>Thanks, >>>Andreas >>>--- >>> drivers/pci/probe.c | 45 ++++++++++++++++++++++++++++++++++++--------- >>> 1 file changed, 36 insertions(+), 9 deletions(-) >>> >>>diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>index e3cf8a2..81dd668 100644 >>>--- a/drivers/pci/probe.c >>>+++ b/drivers/pci/probe.c >>>@@ -740,6 +740,30 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >>> } >>> EXPORT_SYMBOL(pci_add_new_bus); >>> >>>+int pci_grow_bus(struct pci_bus *bus, int bus_max) >>>+{ >>>+ struct resource *res = &bus->busn_res; >>>+ struct resource old_res = *res; >>>+ int ret; >>>+ if (res->end >= bus_max) >>>+ return 0; >>>+ if (!bus->self || pci_is_root_bus(bus)) { >>>+ dev_printk(KERN_DEBUG, &bus->dev, >>>+ "root busn_res %pR cannot grow\n", &res); >>>+ return -EBUSY; >>>+ } >>>+ ret = pci_grow_bus(bus->parent, bus_max); >>>+ if (ret) >>>+ return ret; >>>+ ret = adjust_resource(res, res->start, bus_max - res->start + 1); >>>+ dev_printk(KERN_DEBUG, &bus->dev, >>>+ "busn_res: %pR end %s grown to %02x\n", >>>+ &old_res, ret ? "cannot be" : "has", bus_max); >>>+ >> >> If adjust_resource fails, I think we can't write the bus_max into the bridge >> register. >Right, there is an if statement missing. Thanks. > >>>+ pci_write_config_byte(bus->self, PCI_SUBORDINATE_BUS, bus_max); >>>+ return ret; >>>+} >>>+ >>> /* >>> * If it's a bridge, configure it and scan the bus behind it. >>> * For CardBus bridges, we don't scan behind as the devices will >>>@@ -814,12 +838,11 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>> } >>> >>> cmax = pci_scan_child_bus(child); >>>- if (cmax > subordinate) >>>- dev_warn(&dev->dev, "bridge has subordinate %02x but max busn %02x\n", >>>- subordinate, cmax); >>>- /* subordinate should equal child->busn_res.end */ >>>- if (subordinate > max) >>>- max = subordinate; >>>+ if (cmax > child->busn_res.end) >>>+ dev_warn(&dev->dev, "bridge has bus resource %pR but max busn %02x\n", >>>+ &child->busn_res, cmax); >>>+ if (child->busn_res.end > max) >>>+ max = child->busn_res.end; >> >> By a quick look, I don't see some place change the busn_res.end. And in the >> pci_bus_insert_busn_res(), the child->busn_res.end is set to subordinate. So I >> don't see the reason you change it. >adjust_resource in pci_grow_bus changes busn_res.end. The value in >'subordinate' is then stale at this point. > Yep, I see it. Thanks >> >> Do I miss something? >> >>> } else { >>> /* >>> * We need to assign a number to this bus which we always >>>@@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>> >>> if (max >= bus->busn_res.end) { >>> dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", >>>- max, &bus->busn_res); >>>- goto out; >>>+ max + 1, &bus->busn_res); >>>+ /* Try to resize bus */ >>>+ if (pci_grow_bus(bus, max + 1)) >>>+ goto out; >> >> On some platforms, like powerpc, we have some limitations of the bus number a >> bridge could have. Sometimes, we need the start bus number to be power 2 >> aligned. >> >> Hmm... hope I don't make a mistake, add Gavin in CC list. Gavin, is my >> understanding correct? >Hm I have not seen any code that would enforce this. Is it possible to >do pci=assign-busses on PowerPC? > No code in kernel, while we assign the bus number in firmware. Usually we don't re-assigned the bus numbers at kernel. >>> } >>> >>> /* Clear errors */ >>>@@ -908,6 +933,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>> break; >>> } >>> } >>>+ dev_info(&dev->dev, "adding %02x bus numbers for cardbus\n", i); >>> max += i; >>> } >>> /* >>>@@ -916,7 +942,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>> if (max > bus->busn_res.end) { >>> dev_warn(&dev->dev, "max busn %02x is outside %pR\n", >>> max, &bus->busn_res); >>>- max = bus->busn_res.end; >>>+ if (pci_grow_bus(bus, max)) >>>+ max = bus->busn_res.end; >>> } >>> pci_bus_update_busn_res_end(child, max); >>> pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max); >>>-- >>>2.1.0 >>> >>>-- >>>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 >> >> -- >> Richard Yang >> Help you, Help me >> >-- >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 Tue, Sep 16, 2014 at 09:37:04AM +0800, Wei Yang wrote: >On Mon, Sep 15, 2014 at 12:04:22PM +0200, Andreas Noever wrote: >>On Mon, Sep 15, 2014 at 11:53 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote: >>> On Mon, Sep 15, 2014 at 12:10:27AM +0200, Andreas Noever wrote: >>>>[+cc Thomas, your regression has probably the same cause] >>>> >>>>This looks a like it is going to be a little more complicated than anticipated. >>>> >>>>pci_scan_bus has two main branches. One for "broken" hardware and cardbus and >>>>another one for bridges whose configuration looks sane. >>>> >>>>David's working 3.2 Kernel does the following: >>>>pci 0000:00:1e.0: PCI bridge to [bus 01-01] (subtractive decode) >>>>pci 0000:01:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring >>>> >>>>The 00:1e bridge is deemed sane and the 01:01 (cardbus) bridge below it is >>>>deemed broken. pci_scan_bridge than assigns bus [2-5] to 01:01 without touching >>>>1e (so we just got 4 imaginary busses). We just print a warning: >>>>pci_bus 0000:02: [bus 02-05] partially hidden behind transparent bridge 0000:01 [bus 01-01] >>>> >>>>After returning to 00:1e (in the sane branch) we also do not update our >>>>subordinate and just return. Later yenta_socket sees that this is nuts and >>>>carefully increases the subordinate of 00:1e. >>>> >>>>My patch series for 3.15 was trying to make sure that pci_scan_bus does not >>>>overstep its resources. So now we now refuse to create bus 2 under [01-01] >>>>(fc1b2531 PCI: Don't scan random busses in pci_scan_bridge()) and a little >>>>later we would forbid the cardbus code in pci_scan_bridge from reserving 4 more >>>>busses (1820ffdc PCI: Make sure bus number resources stay within their parents >>>>bounds). At least these two would need to be reverted. >>>> >>>>As an alternative the following patch tries grow the bus window, if necessary >>>>by growing its parents bus window (recursively). This should make the yenta >>>>fixup unnecessary. I have tested the patch with normal pci bridges + setpci, >>>>since I don't have access to a cardbus system. >>>> >>>>So if you have time please test this and/or try to revert the two mentioned >>>>commits. >>>> >>>>Thanks, >>>>Andreas >>>>--- >>>> drivers/pci/probe.c | 45 ++++++++++++++++++++++++++++++++++++--------- >>>> 1 file changed, 36 insertions(+), 9 deletions(-) >>>> >>>>diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>>index e3cf8a2..81dd668 100644 >>>>--- a/drivers/pci/probe.c >>>>+++ b/drivers/pci/probe.c >>>>@@ -740,6 +740,30 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, >>>> } >>>> EXPORT_SYMBOL(pci_add_new_bus); >>>> >>>>+int pci_grow_bus(struct pci_bus *bus, int bus_max) >>>>+{ >>>>+ struct resource *res = &bus->busn_res; >>>>+ struct resource old_res = *res; >>>>+ int ret; >>>>+ if (res->end >= bus_max) >>>>+ return 0; >>>>+ if (!bus->self || pci_is_root_bus(bus)) { >>>>+ dev_printk(KERN_DEBUG, &bus->dev, >>>>+ "root busn_res %pR cannot grow\n", &res); >>>>+ return -EBUSY; >>>>+ } >>>>+ ret = pci_grow_bus(bus->parent, bus_max); >>>>+ if (ret) >>>>+ return ret; >>>>+ ret = adjust_resource(res, res->start, bus_max - res->start + 1); >>>>+ dev_printk(KERN_DEBUG, &bus->dev, >>>>+ "busn_res: %pR end %s grown to %02x\n", >>>>+ &old_res, ret ? "cannot be" : "has", bus_max); >>>>+ >>> >>> If adjust_resource fails, I think we can't write the bus_max into the bridge >>> register. >>Right, there is an if statement missing. Thanks. >> >>>>+ pci_write_config_byte(bus->self, PCI_SUBORDINATE_BUS, bus_max); >>>>+ return ret; >>>>+} >>>>+ >>>> /* >>>> * If it's a bridge, configure it and scan the bus behind it. >>>> * For CardBus bridges, we don't scan behind as the devices will >>>>@@ -814,12 +838,11 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>>> } >>>> >>>> cmax = pci_scan_child_bus(child); >>>>- if (cmax > subordinate) >>>>- dev_warn(&dev->dev, "bridge has subordinate %02x but max busn %02x\n", >>>>- subordinate, cmax); >>>>- /* subordinate should equal child->busn_res.end */ >>>>- if (subordinate > max) >>>>- max = subordinate; >>>>+ if (cmax > child->busn_res.end) >>>>+ dev_warn(&dev->dev, "bridge has bus resource %pR but max busn %02x\n", >>>>+ &child->busn_res, cmax); >>>>+ if (child->busn_res.end > max) >>>>+ max = child->busn_res.end; >>> >>> By a quick look, I don't see some place change the busn_res.end. And in the >>> pci_bus_insert_busn_res(), the child->busn_res.end is set to subordinate. So I >>> don't see the reason you change it. >>adjust_resource in pci_grow_bus changes busn_res.end. The value in >>'subordinate' is then stale at this point. >> > >Yep, I see it. Thanks > >>> >>> Do I miss something? >>> >>>> } else { >>>> /* >>>> * We need to assign a number to this bus which we always >>>>@@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>>> >>>> if (max >= bus->busn_res.end) { >>>> dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", >>>>- max, &bus->busn_res); >>>>- goto out; >>>>+ max + 1, &bus->busn_res); >>>>+ /* Try to resize bus */ >>>>+ if (pci_grow_bus(bus, max + 1)) >>>>+ goto out; >>> >>> On some platforms, like powerpc, we have some limitations of the bus number a >>> bridge could have. Sometimes, we need the start bus number to be power 2 >>> aligned. >>> >>> Hmm... hope I don't make a mistake, add Gavin in CC list. Gavin, is my >>> understanding correct? >>Hm I have not seen any code that would enforce this. Is it possible to >>do pci=assign-busses on PowerPC? >> > >No code in kernel, while we assign the bus number in firmware. > >Usually we don't re-assigned the bus numbers at kernel. > Yes, you're correct. But we don't have to have the limitation on P8: Each PCI bridge has downstream 2^N spanning buses. That means PCI bridge on P8 can take any bus numbers assigned by firmware/kernel if I'm correct. Actually, it's P7 box specific problem when we have a PCIe-to-PCI bridge and have to put everything behind the bridge into one PE. If we don't have PCI domain behind PCIe-to-PCI bridge on P7 box, we needn't consider the limitation. Another reason why we don't expect kernel to reassign bus numbers is related to hotplug and EEH. Currently, we don't reconfigure things for (RID mapping, IO and MMIO mapping to PE#). So the PCI bus numbers assigned to one particular PCI bridge is expected to be fixed. I guess we can improve it when having a chance looking at current implementation so that things will be reconfigured during hotplug. >>>> } >>>> >>>> /* Clear errors */ >>>>@@ -908,6 +933,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>>> break; >>>> } >>>> } >>>>+ dev_info(&dev->dev, "adding %02x bus numbers for cardbus\n", i); >>>> max += i; >>>> } >>>> /* >>>>@@ -916,7 +942,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >>>> if (max > bus->busn_res.end) { >>>> dev_warn(&dev->dev, "max busn %02x is outside %pR\n", >>>> max, &bus->busn_res); >>>>- max = bus->busn_res.end; >>>>+ if (pci_grow_bus(bus, max)) >>>>+ max = bus->busn_res.end; >>>> } >>>> pci_bus_update_busn_res_end(child, max); >>>> pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max); Thanks, Gavin -- 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 Mon, Sep 15, 2014 at 01:03:19PM -0600, Bjorn Helgaas wrote: >On Mon, Sep 15, 2014 at 05:53:05PM +0800, Wei Yang wrote: >> On Mon, Sep 15, 2014 at 12:10:27AM +0200, Andreas Noever wrote: >> ... > >> >@@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) >> > >> > if (max >= bus->busn_res.end) { >> > dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", >> >- max, &bus->busn_res); >> >- goto out; >> >+ max + 1, &bus->busn_res); >> >+ /* Try to resize bus */ >> >+ if (pci_grow_bus(bus, max + 1)) >> >+ goto out; >> >> On some platforms, like powerpc, we have some limitations of the bus number a >> bridge could have. Sometimes, we need the start bus number to be power 2 >> aligned. > >Huh. I have to admit that I'm getting tired of all the powerpc-specific >PCI hacks. It's hard enough to get this stuff working on hardware that >conforms to the spec, and scattering pcibios_*() hooks around makes the >code even harder to follow. > Yep, those powerpc-specific things are not friendly :-( I feel very sad it brings a lot difficulties to maintain it in linux mainline. Sorry to bring so many trouble to you. >What would happen if powerpc used PCI_PROBE_ONLY? Do you really depend on >the PCI core to configure anything for you, or does your firmware set >everything up the way it needs to be? > Hmm... I had a try with PCI_PROBE_ONLY set, sounds can't bring up the machine. Currently, we rely on the kernel to assign devices' resources. When PCI_PROBE_ONLY is set, device resources will not be setup properly. >Changing bridge configuration seems like something we should avoid under >PCI_PROBE_ONLY (I haven't read Andreas' patch in detail, so I don't know if >that's how it works). If PCI_PROBE_ONLY would work for powerpc, then we >wouldn't have an issue here. > >Bjorn
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index e3cf8a2..81dd668 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -740,6 +740,30 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, } EXPORT_SYMBOL(pci_add_new_bus); +int pci_grow_bus(struct pci_bus *bus, int bus_max) +{ + struct resource *res = &bus->busn_res; + struct resource old_res = *res; + int ret; + if (res->end >= bus_max) + return 0; + if (!bus->self || pci_is_root_bus(bus)) { + dev_printk(KERN_DEBUG, &bus->dev, + "root busn_res %pR cannot grow\n", &res); + return -EBUSY; + } + ret = pci_grow_bus(bus->parent, bus_max); + if (ret) + return ret; + ret = adjust_resource(res, res->start, bus_max - res->start + 1); + dev_printk(KERN_DEBUG, &bus->dev, + "busn_res: %pR end %s grown to %02x\n", + &old_res, ret ? "cannot be" : "has", bus_max); + + pci_write_config_byte(bus->self, PCI_SUBORDINATE_BUS, bus_max); + return ret; +} + /* * If it's a bridge, configure it and scan the bus behind it. * For CardBus bridges, we don't scan behind as the devices will @@ -814,12 +838,11 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) } cmax = pci_scan_child_bus(child); - if (cmax > subordinate) - dev_warn(&dev->dev, "bridge has subordinate %02x but max busn %02x\n", - subordinate, cmax); - /* subordinate should equal child->busn_res.end */ - if (subordinate > max) - max = subordinate; + if (cmax > child->busn_res.end) + dev_warn(&dev->dev, "bridge has bus resource %pR but max busn %02x\n", + &child->busn_res, cmax); + if (child->busn_res.end > max) + max = child->busn_res.end; } else { /* * We need to assign a number to this bus which we always @@ -840,8 +863,10 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) if (max >= bus->busn_res.end) { dev_warn(&dev->dev, "can't allocate child bus %02x from %pR\n", - max, &bus->busn_res); - goto out; + max + 1, &bus->busn_res); + /* Try to resize bus */ + if (pci_grow_bus(bus, max + 1)) + goto out; } /* Clear errors */ @@ -908,6 +933,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) break; } } + dev_info(&dev->dev, "adding %02x bus numbers for cardbus\n", i); max += i; } /* @@ -916,7 +942,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) if (max > bus->busn_res.end) { dev_warn(&dev->dev, "max busn %02x is outside %pR\n", max, &bus->busn_res); - max = bus->busn_res.end; + if (pci_grow_bus(bus, max)) + max = bus->busn_res.end; } pci_bus_update_busn_res_end(child, max); pci_write_config_byte(dev, PCI_SUBORDINATE_BUS, max);