| Submitter | Yinghai Lu |
|---|---|
| Date | 2009-10-26 21:23:39 |
| Message ID | <4AE6135B.2070003@kernel.org> |
| Download | mbox | patch |
| Permalink | /patch/55980/ |
| State | Superseded |
| Headers | show |
Comments
On Mon, 2009-10-26 at 14:23 -0700, Yinghai Lu wrote: > =================================================================== > --- linux-2.6.orig/drivers/pci/setup-bus.c > +++ linux-2.6/drivers/pci/setup-bus.c > @@ -322,33 +322,54 @@ static void pci_bridge_check_ranges(stru > } > } > > -/* 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, > - have non-NULL parent resource). */ > -static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type) > +void pci_bridge_release_not_used_res(struct pci_bus *bus) > { > int i; > struct resource *r; > unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM | > IORESOURCE_PREFETCH; > > - for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) { > + /* for pci bridges res only */ > + for (i = 0; i < 3; i++) { I think the "i = 0; i < 3" part is to check the bridge apertures, i.e., the I/O base/limit, the memory base/limit, and the prefetchable memory base/limit. Right? We need some way to indicate that more clearly than using a hard-coded "3". > r = bus->resource[i]; > - if (r == &ioport_resource || r == &iomem_resource) > - continue; > - if (r && (r->flags & type_mask) == type) { > + if (r && (r->flags & type_mask)) { > if (!r->parent) > - return r; > + continue; > /* > * if there is no child under that, we should release > * and use it. don't need to reset it, pbus_size_* will > * set it again > */ > if (!r->child && !release_resource(r)) We got this resource pointer out of a struct pci_bus, and we release it here. We must have previously done a request_resource(), allocate_resource(), or similar. Where does that happen? Are the requests and releases nested correctly? I would think that somewhere, we would be doing a request_resource() and assigning the resource to pci_bus->resource[x]. But there are very few assignments to the pci_bus resources: setup_resource (only for "pci=use_crs") pci_read_bridge_bases (just a copy from upstream bus resources) pci_alloc_child_bus (copy from upstream bridge resources) pci_create_bus (set to &ioport_resource or &iomem_resource) My guess is that this release_resource() releases something we copied from the bridge in pci_alloc_child_bus(). But that doesn't seem right, because we aren't changing the bridge programming here. Maybe I'm being confused by the fact that we find the resource pointer from the pci_bus table, but that's only a pointer to the actual struct resource that lives in the pci_dev of the upstream bridge. Bjorn > - return r; > + dev_printk(KERN_DEBUG, &bus->dev, > + "resource %d %s %pR released\n", i, > + (r->flags & IORESOURCE_IO) ? "io: " : > + ((r->flags & IORESOURCE_PREFETCH)? > + "pref mem":"mem:"), > + r); > } > } > +} > +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, > + have non-NULL parent resource). */ > +static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type) > +{ > + int i; > + struct resource *r; > + unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM | > + IORESOURCE_PREFETCH; > + > + for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) { > + r = bus->resource[i]; > + if (r == &ioport_resource || r == &iomem_resource) > + continue; > + if (r && (r->flags & type_mask) == type && !r->parent) > + return r; > + } > return NULL; > } > > @@ -588,6 +609,41 @@ void __ref pci_bus_size_bridges(struct p > } > EXPORT_SYMBOL(pci_bus_size_bridges); > > +static void pci_bus_release_bridges_not_used_res(struct pci_bus *bus) > +{ > + struct pci_dev *dev; > + > + 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: > + break; > + > + case PCI_CLASS_BRIDGE_PCI: > + default: > + 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: > + pci_bridge_release_not_used_res(bus); > + break; > + } > +} > + > void __ref pci_bridge_assign_resources(const struct pci_dev *bridge) > { > struct pci_bus *b; > @@ -687,6 +743,11 @@ pci_assign_unassigned_resources(void) > { > struct pci_bus *bus; > > + /* 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); > + } > + > /* Depth first, calculate sizes and alignments of all > subordinate buses. */ > list_for_each_entry(bus, &pci_root_buses, node) { > 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 > @@ -171,6 +171,7 @@ int pciehp_unconfigure_device(struct slo > } > pci_dev_put(temp); > } > + pci_bridge_release_not_used_res(parent); > > 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,7 @@ 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_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 *); -- 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
Bjorn Helgaas wrote: > On Mon, 2009-10-26 at 14:23 -0700, Yinghai Lu wrote: > >> =================================================================== >> --- linux-2.6.orig/drivers/pci/setup-bus.c >> +++ linux-2.6/drivers/pci/setup-bus.c >> @@ -322,33 +322,54 @@ static void pci_bridge_check_ranges(stru >> } >> } >> >> -/* 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, >> - have non-NULL parent resource). */ >> -static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type) >> +void pci_bridge_release_not_used_res(struct pci_bus *bus) >> { >> int i; >> struct resource *r; >> unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM | >> IORESOURCE_PREFETCH; >> >> - for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) { >> + /* for pci bridges res only */ >> + for (i = 0; i < 3; i++) { > > I think the "i = 0; i < 3" part is to check the bridge apertures, i.e., > the I/O base/limit, the memory base/limit, and the prefetchable memory > base/limit. Right? We need some way to indicate that more clearly than > using a hard-coded "3". yes. the 0x1c, 0x20, and 0x28 > >> r = bus->resource[i]; >> - if (r == &ioport_resource || r == &iomem_resource) >> - continue; >> - if (r && (r->flags & type_mask) == type) { >> + if (r && (r->flags & type_mask)) { >> if (!r->parent) >> - return r; >> + continue; >> /* >> * if there is no child under that, we should release >> * and use it. don't need to reset it, pbus_size_* will >> * set it again >> */ >> if (!r->child && !release_resource(r)) > > We got this resource pointer out of a struct pci_bus, and we release it > here. We must have previously done a request_resource(), > allocate_resource(), or similar. Where does that happen? Are the > requests and releases nested correctly? > > I would think that somewhere, we would be doing a request_resource() and > assigning the resource to pci_bus->resource[x]. But there are very few > assignments to the pci_bus resources: > setup_resource (only for "pci=use_crs") > pci_read_bridge_bases (just a copy from upstream bus resources) > pci_alloc_child_bus (copy from upstream bridge resources) > pci_create_bus (set to &ioport_resource or &iomem_resource) > > My guess is that this release_resource() releases something we copied > from the bridge in pci_alloc_child_bus(). But that doesn't seem right, > because we aren't changing the bridge programming here. in arch/x86/pci/i386.c:: pcibios_resource_survey() ==> pcibios_allocate_bus_resources() YH -- 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 Monday 26 October 2009 06:20:28 pm Yinghai Lu wrote: > Bjorn Helgaas wrote: > > On Mon, 2009-10-26 at 14:23 -0700, Yinghai Lu wrote: > > > >> =================================================================== > >> --- linux-2.6.orig/drivers/pci/setup-bus.c > >> +++ linux-2.6/drivers/pci/setup-bus.c > >> @@ -322,33 +322,54 @@ static void pci_bridge_check_ranges(stru > >> } > >> } > >> > >> -/* 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, > >> - have non-NULL parent resource). */ > >> -static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type) > >> +void pci_bridge_release_not_used_res(struct pci_bus *bus) > >> { > >> int i; > >> struct resource *r; > >> unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM | > >> IORESOURCE_PREFETCH; > >> > >> - for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) { > >> + /* for pci bridges res only */ > >> + for (i = 0; i < 3; i++) { > > > > I think the "i = 0; i < 3" part is to check the bridge apertures, i.e., > > the I/O base/limit, the memory base/limit, and the prefetchable memory > > base/limit. Right? We need some way to indicate that more clearly than > > using a hard-coded "3". > > yes. the 0x1c, 0x20, and 0x28 Do you have a proposal for something other than "3"? The magic number "3" does appear other places, e.g., pci_read_bridge_bases() but I don't think we should add new uses of it. Make up a descriptive #define if nothing else. > >> r = bus->resource[i]; > >> - if (r == &ioport_resource || r == &iomem_resource) > >> - continue; > >> - if (r && (r->flags & type_mask) == type) { > >> + if (r && (r->flags & type_mask)) { > >> if (!r->parent) > >> - return r; > >> + continue; > >> /* > >> * if there is no child under that, we should release > >> * and use it. don't need to reset it, pbus_size_* will > >> * set it again > >> */ > >> if (!r->child && !release_resource(r)) > > > > We got this resource pointer out of a struct pci_bus, and we release it > > here. We must have previously done a request_resource(), > > allocate_resource(), or similar. Where does that happen? Are the > > requests and releases nested correctly? > > > > I would think that somewhere, we would be doing a request_resource() and > > assigning the resource to pci_bus->resource[x]. But there are very few > > assignments to the pci_bus resources: > > setup_resource (only for "pci=use_crs") > > pci_read_bridge_bases (just a copy from upstream bus resources) > > pci_alloc_child_bus (copy from upstream bridge resources) > > pci_create_bus (set to &ioport_resource or &iomem_resource) > > > > My guess is that this release_resource() releases something we copied > > from the bridge in pci_alloc_child_bus(). But that doesn't seem right, > > because we aren't changing the bridge programming here. > > in arch/x86/pci/i386.c:: > pcibios_resource_survey() ==> pcibios_allocate_bus_resources() The asymmetry here bothers me. The pci_claim_resource() in pcibios_allocate_bus_resources() is done on a *device*, while the release_resource() you added is done on a *bus*. And the claim is done in arch-specific code, while the release is done in generic code. And there's an interval where the bridge device resources don't match the hardware apertures, and it's hard to figure out when they are in sync again. This might all work fine, but it's harder to understand than it should be. 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
Patch
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 @@ -322,33 +322,54 @@ static void pci_bridge_check_ranges(stru } } -/* 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, - have non-NULL parent resource). */ -static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type) +void pci_bridge_release_not_used_res(struct pci_bus *bus) { int i; struct resource *r; unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM | IORESOURCE_PREFETCH; - for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) { + /* for pci bridges res only */ + for (i = 0; i < 3; i++) { r = bus->resource[i]; - if (r == &ioport_resource || r == &iomem_resource) - continue; - if (r && (r->flags & type_mask) == type) { + if (r && (r->flags & type_mask)) { if (!r->parent) - return r; + continue; /* * if there is no child under that, we should release * and use it. don't need to reset it, pbus_size_* will * set it again */ if (!r->child && !release_resource(r)) - return r; + dev_printk(KERN_DEBUG, &bus->dev, + "resource %d %s %pR released\n", i, + (r->flags & IORESOURCE_IO) ? "io: " : + ((r->flags & IORESOURCE_PREFETCH)? + "pref mem":"mem:"), + r); } } +} +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, + have non-NULL parent resource). */ +static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned long type) +{ + int i; + struct resource *r; + unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM | + IORESOURCE_PREFETCH; + + for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) { + r = bus->resource[i]; + if (r == &ioport_resource || r == &iomem_resource) + continue; + if (r && (r->flags & type_mask) == type && !r->parent) + return r; + } return NULL; } @@ -588,6 +609,41 @@ void __ref pci_bus_size_bridges(struct p } EXPORT_SYMBOL(pci_bus_size_bridges); +static void pci_bus_release_bridges_not_used_res(struct pci_bus *bus) +{ + struct pci_dev *dev; + + 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: + break; + + case PCI_CLASS_BRIDGE_PCI: + default: + 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: + pci_bridge_release_not_used_res(bus); + break; + } +} + void __ref pci_bridge_assign_resources(const struct pci_dev *bridge) { struct pci_bus *b; @@ -687,6 +743,11 @@ pci_assign_unassigned_resources(void) { struct pci_bus *bus; + /* 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); + } + /* Depth first, calculate sizes and alignments of all subordinate buses. */ list_for_each_entry(bus, &pci_root_buses, node) { 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 @@ -171,6 +171,7 @@ int pciehp_unconfigure_device(struct slo } pci_dev_put(temp); } + pci_bridge_release_not_used_res(parent); 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,7 @@ 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_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 *);