Alex Chiang wrote:
Hello Kenji-san,
* Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
Alex Chiang wrote:
I hadn't gotten around to verifying/fixing existing callers of
pci_do_scan_bus yet. I was focusing on the core first.
There aren't too many callers, but unfortunately, I don't have
any hardware that actually uses the existing drivers.
I seem to recall that your machines support shpchp. Would you
mind testing this patch and telling me if your machine still
behaves properly?
I have machines that support shpchp. But unfortunately I don't
have any adapter card that contains bridge, which is needed to
test your change.
You're right.
The more I think about it though, the more I think that even
without the below patch to clean up the callers of
pci_do_scan_bus, we should be ok, because:
- all the old code (which I removed below) existed
because the old PCI core would refuse to scan PCI buses
that had already been discovered
- that meant that it would never descend past a known
bridge to try and find new child bridges
- that meant that hotplug drivers had to manually
discover new bridges and add them, essentially
duplicating functionality in pci_scan_bridge
This patch series allows the PCI core to scan existing bridges
and descend down into the children every time, looking for new
bridges and devices, so all the code in shpchp, cpcihp, and other
callers of pci_do_scan_bus shouldn't be necessary anymore.
Also, if we do add new bridges once manually in shpchp, and then
call the new pci_do_scan_bus again, we will _not_ add devices
twice because the core should check each bridge and device for
struct pci_dev.is_added.
So anyway, I think that cleaning up the callers of
pci_do_scan_bus is a good idea, but multiple calls to the
interface definitely should not result in problems. If they do,
then that's a bug in my patch series.
I'm sorry, but I didn't have enough time to try your patch on
my environment. So I'm still just looking at the code.
I looked at shpchp_configure_device() from the view point of
bridge hot-add. I think it is broken regardless of your change
because it calls pci_bus_add_devices() (through pci_do_scan_bus)
before assigning resources. So I think it must be changed
regardless of your change. But it's a little difficult for me
because I don't have any test environment as I mentioned before.
But I'm still worrying about your change against pci_do_scan_bus().
Without your change, pci_do_scan_bus() scans child buses and add
devices without assigning resources. I guess that it means existing
callers of pci_do_scan_bus() have some mechanism to assign resource
by theirselves and they don't expect pci_do_scan_bus() assigns
resources.
By the way, I have one question about rescan. Please suppose that
we enable the bridge(B) and its children using rescan interface
in the picture below.
|
-------------------------------------- parent bus
| |
bridge(A) bridge(B)
(working) (Not working)
| |
------------- -------------
| | | |
dev dev dev dev
(working) (working) (Not working)
In this case, your rescan mechanism calls pci_do_scan_bus() for
parent bus, and pci_do_scan_bus() calls pci_bus_assign_resources()
for parent bus. My question is, does pci_bus_assign_resources() do
nothing against bridge(A) that is currently working? I guess
pci_bus_assign_resources() would update some registers of bridge(A)
and it would breaks currently working devices.
Thanks,
Kenji Kaneshige
I looked at shpchp_configure_device and I think that simply
scanning the device's parent bus should work.
Ok, I'll try it.
I set up a git tree to make it easier to test:
git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
The 'test-20090313' branch contains all the latest fixes to
enable this patch series. It does not contain the patch below, so
you will have to apply it by hand.
Thanks for testing!
/ac
Thanks.
/ac
diff --git a/drivers/pci/hotplug/shpchp_pci.c b/drivers/pci/hotplug/shpchp_pci.c
index aa315e5..7e8457b 100644
--- a/drivers/pci/hotplug/shpchp_pci.c
+++ b/drivers/pci/hotplug/shpchp_pci.c
@@ -110,11 +110,7 @@ int __ref shpchp_configure_device(struct slot *p_slot)
return -EINVAL;
}
- num = pci_scan_slot(parent, PCI_DEVFN(p_slot->device, 0));
- if (num == 0) {
- ctrl_err(ctrl, "No new device found\n");
- return -ENODEV;
- }
+ pci_do_scan_bus(parent);
for (fn = 0; fn < 8; fn++) {
dev = pci_get_slot(parent, PCI_DEVFN(p_slot->device, fn));
@@ -126,40 +122,10 @@ int __ref shpchp_configure_device(struct slot *p_slot)
pci_dev_put(dev);
continue;
}
- if ((dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) ||
- (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)) {
- /* Find an unused bus number for the new bridge */
- struct pci_bus *child;
- unsigned char busnr, start = parent->secondary;
- unsigned char end = parent->subordinate;
- for (busnr = start; busnr <= end; busnr++) {
- if (!pci_find_bus(pci_domain_nr(parent),
- busnr))
- break;
- }
- if (busnr > end) {
- ctrl_err(ctrl,
- "No free bus for hot-added bridge\n");
- pci_dev_put(dev);
- continue;
- }
- child = pci_add_new_bus(parent, dev, busnr);
- if (!child) {
- ctrl_err(ctrl, "Cannot add new bus for %s\n",
- pci_name(dev));
- pci_dev_put(dev);
- continue;
- }
- child->subordinate = pci_do_scan_bus(child);
- pci_bus_size_bridges(child);
- }
program_fw_provided_values(dev);
pci_dev_put(dev);
}
- pci_bus_assign_resources(parent);
- pci_bus_add_devices(parent);
- pci_enable_bridges(parent);
return 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
--
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
