On Mon, Sep 15, 2014 at 11:53 AM, Wei Yang <weiyang@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>>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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
