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@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.
>
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@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
--
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
