Re: [PATCH 2/2] pci: only release that resource index is less than3 -v5
From: Kenji Kaneshige
Date: Thu Oct 29 2009 - 02:34:52 EST
Yinghai Lu wrote:
after
| commit 308cf8e13f42f476dfd6552aeff58fdc0788e566
|
| PCI: get larger bridge ranges when space is available
found one of resource of peer root bus (0x00) get released from root
resource. later one hotplug device can not get big range anymore.
other peer root buses is ok.
it turns out it is from transparent path.
those resources will be used for pci bridge BAR updated.
so need to limit it to 3.
v2: Jesse doesn't like it is in find_free_bus_resource...
try to move out of pci_bus_size_bridges loop.
need to apply after:
[PATCH] pci: pciehp update the slot bridge res to get big range for pcie devices - v4
v3: add pci_setup_bridge calling after pci_bridge_release_not_used_res.
only clear release those res for x86.
v4: Bjorn want to release use dev instead of bus.
v5: Kenji pointed out it will have problem with several level bridge.
so let only handle leaf bridge.
Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
---
arch/x86/pci/i386.c | 6 ++
drivers/pci/hotplug/pciehp_pci.c | 2
drivers/pci/setup-bus.c | 81 ++++++++++++++++++++++++++++++++++++++-
include/linux/pci.h | 2
4 files changed, 90 insertions(+), 1 deletion(-)
Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -319,6 +319,42 @@ static void pci_bridge_check_ranges(stru
}
}
+void pci_bridge_release_not_used_res(struct pci_bus *bus)
+{
+ int idx;
+ bool changed = false;
+ struct pci_dev *dev;
+ struct resource *r;
+ unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
+ IORESOURCE_PREFETCH;
+
+ /* for pci bridges res only */
+ dev = bus->self;
+ for (idx = PCI_BRIDGE_RESOURCES; idx < PCI_BRIDGE_RESOURCES + 3;
+ idx++) {
If this function is only for normal PCI bridge, we need additional
check at the head of this function.
if ((dev->class >> 8) == PCI_CLASS_BRIDGE_CARDBUS)
return;
if not, 3 should be 4 instead. And we can do as follows
for (i = PCI_BRIDGE_RESOURCE; i <= PCI_BRIDGE_RESOURCE_END; i++) {
+ r = &dev->resource[idx];
+ if (r->flags & type_mask) {
How about
if (!(r->flags & type_mask))
continue;
+ if (!r->parent)
+ continue;
+ /*
+ * if there is no child under that, we should release
+ * and use it.
+ */
I think this comment should be updated because whether "we should
release and use it" is decided by the caller of this function.
+ if (!r->child && !release_resource(r)) {
+ dev_info(&dev->dev,
+ "resource %d %pRt released\n",
+ idx, r);
+ r->flags = 0;
+ changed = true;
+ }
How about
if (r->child)
continue;
if (!release_resource(r)) {
...
}
+ }
+ }
+
+ if (changed)
+ pci_setup_bridge(bus);
The pci_setup_bridge() is called even if some resources are used
by children. For example, mem resource is used by children, but
prefetchable mem resource is not used by children. In this case,
I think mem resource is released and pci_setup_bridge() is called
while prefetcable mem resource is being used. I'm worrying that it
might cause something wrong.
+}
+EXPORT_SYMBOL(pci_bridge_release_not_used_res);
+
/* Helper function for sizing routines: find first available
bus resource of a given type. Note: we intentionally skip
the bus resources which have already been assigned (that is,
@@ -576,6 +612,48 @@ void __ref pci_bus_size_bridges(struct p
}
EXPORT_SYMBOL(pci_bus_size_bridges);
+
+/* only release those resources that is on leaf bridge */
+void __ref pci_bus_release_bridges_not_used_res(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+ bool is_leaf_bridge = true;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ struct pci_bus *b = dev->subordinate;
+ if (!b)
+ continue;
+
+ switch (dev->class >> 8) {
+ case PCI_CLASS_BRIDGE_CARDBUS:
+ is_leaf_bridge = false;
+ break;
+
+ case PCI_CLASS_BRIDGE_PCI:
+ default:
+ is_leaf_bridge = false;
+ pci_bus_release_bridges_not_used_res(b);
+ break;
+ }
+ }
+
+ /* The root bus? */
+ if (!bus->self)
+ return;
+
+ switch (bus->self->class >> 8) {
+ case PCI_CLASS_BRIDGE_CARDBUS:
+ break;
+
+ case PCI_CLASS_BRIDGE_PCI:
+ default:
+ if (is_leaf_bridge)
+ pci_bridge_release_not_used_res(bus);
+ break;
+ }
+}
How about like below. This might need to be called with "pci_bus_sem" held.
void __ref pci_bus_release_bridges_not_used_res(struct pci_bus *bus)
{
struct pci_bus *b;
/* If the bus is cardbus, do nothing */
if (bus->self && (bus->self->class >> 8) == PCI_CLASS_BRIDGE_CARDBUS)
return;
/* We only release the resources under the leaf bridge */
if (list_empty(&bus->children)) {
if (!pci_is_root_bus(bus))
pci_bridge_release_not_used_res(bus);
return;
}
/* Scan child buses if the bus is not leaf */
list_for_each_entry(b, &bus->children, node)
pci_bus_release_bridges_not_used_res(b);
}
+EXPORT_SYMBOL(pci_bus_release_bridges_not_used_res);
+
void __ref pci_bridge_assign_resources(const struct pci_dev *bridge)
{
struct pci_bus *b;
@@ -644,7 +722,8 @@ static void pci_bus_dump_res(struct pci_
for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
struct resource *res = bus->resource[i];
- if (!res || !res->end)
+
+ if (!res || !res->end || !res->flags)
continue;
dev_printk(KERN_DEBUG, &bus->dev, "resource %d %pRt\n", i, res);
Index: linux-2.6/drivers/pci/hotplug/pciehp_pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/hotplug/pciehp_pci.c
+++ linux-2.6/drivers/pci/hotplug/pciehp_pci.c
@@ -98,6 +98,7 @@ int pciehp_configure_device(struct slot
pci_dev_put(dev);
}
+ pci_bridge_release_not_used_res(parent);
I guess this is for the slots that are empty at the boot time on non x86
systems. And it only works at the first hot-add. Correct? How about doing
this at the pciehp's slot initialization.
pci_bus_size_bridges(parent);
pci_clear_master(bridge);
pci_bridge_assign_resources(bridge);
@@ -171,6 +172,7 @@ int pciehp_unconfigure_device(struct slo
}
pci_dev_put(temp);
}
+ pci_bridge_release_not_used_res(parent);
How about call pci_bus_release_bridges_not_used_res() instead and
make pci_bridge_release_not_used_res() static function.
return rc;
}
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -759,6 +759,8 @@ int pci_vpd_truncate(struct pci_dev *dev
void pci_bridge_assign_resources(const struct pci_dev *bridge);
void pci_bus_assign_resources(const struct pci_bus *bus);
void pci_bus_size_bridges(struct pci_bus *bus);
+void pci_bus_release_bridges_not_used_res(struct pci_bus *bus);
+void pci_bridge_release_not_used_res(struct pci_bus *bus);
int pci_claim_resource(struct pci_dev *, int);
void pci_assign_unassigned_resources(void);
void pdev_enable_device(struct pci_dev *);
Index: linux-2.6/arch/x86/pci/i386.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/i386.c
+++ linux-2.6/arch/x86/pci/i386.c
@@ -194,6 +194,7 @@ static void __init pcibios_allocate_reso
static int __init pcibios_assign_resources(void)
{
struct pci_dev *dev = NULL;
+ struct pci_bus *bus;
struct resource *r;
if (!(pci_probe & PCI_ASSIGN_ROMS)) {
@@ -213,6 +214,11 @@ static int __init pcibios_assign_resourc
}
}
+ /* Try to release bridge resources, that there is not child under it */
+ list_for_each_entry(bus, &pci_root_buses, node) {
+ pci_bus_release_bridges_not_used_res(bus);
+ }
+
If this is only for PCIe hotplug, I don't think we need it here (as
you're doing for non x86 platforms). If not, I think you should do
it in the another patch.
Thanks,
Kenji Kaneshige
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/