Re: [PATCH 1/2] pci: release that leaf bridge' resource index is not used -v6

[Yinghai Lu <yinghai@kernel.org>]

Jesse Barnes wrote:
> On Thu, 29 Oct 2009 02:52:00 -0700
> Yinghai Lu <yinghai@kernel.org> wrote:
> 
>> only for index < 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.
>> v6: address Kenji's request (new pci_bus_release...). and change
>> applying order move back release to pci_assign_unassigned_resource
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> Starting to look better...
> 
>> ---
>>  drivers/pci/setup-bus.c |   65
>> +++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/pci.h     |    1 2 files changed, 65 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
>> @@ -573,13 +573,68 @@ void __ref pci_bus_assign_resources(cons
>>  }
>>  EXPORT_SYMBOL(pci_bus_assign_resources);
>>  
>> +static void pci_bridge_release_not_used_res(struct pci_bus *bus)
> 
> Can we call these functions something else?  Maybe
> pci_bridge_release_unused() with a kdoc comment describing exactly what
> it does and why?

ok.

> 
>> +{
>> +	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++) {
>> +		r = &dev->resource[idx];
>> +		if (!(r->flags & type_mask))
>> +			continue;
>> +		if (!r->parent)
>> +			continue;
>> +		/* if there is no child under that, we should
>> release it */
>> +		if (r->child)
>> +			continue;
>> +		if (!release_resource(r)) {
>> +			dev_info(&dev->dev, "resource %d %pRt
>> released\n",
>> +				 idx, r);
>> +			r->flags = 0;
>> +			changed = true;
>> +		}
>> +	}
>> +
>> +	if (changed)
>> +		pci_setup_bridge(bus);
>> +}
>> +
>> +void __ref pci_bus_release_bridges_not_used_res(struct pci_bus *bus)
> 
> Likewise, maybe pci_bus_release_unused_bridge_res or something, with
> comments again.

ok

> 
>> +{
>> +	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);
>> +
>>  static void pci_bus_dump_res(struct pci_bus *bus)
>>  {
>>          int i;
>>  
>>          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); @@ -608,6 +663,14 @@
>> pci_assign_unassigned_resources(void) {
>>  	struct pci_bus *bus;
>>  
>> +	/*
>> +	 * Try to release leaf bridge's 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);
>> +	}
>> +
> 
> Also I wonder if we need to make this a command line option that isn't
> run by default?

thinking:
1. try not to touch bridge resource and do the allocation and record the fail device
2. release leaf bridge for those failed device
3. do second around bus_size and allocation.

> 
> Really, as Eric says, we need a real dynamic allocation system with the
> ability to move devices around at some point.  Any volunteers? :)

that will need suspend driver and rescan allocation and wakeup driver...

YH