Re: [PATCH 1/1] PCI: allocate essential resources before reserving hotplug resources

[Ram Pai <linuxram@us.ibm.com>]

On Tue, Jan 18, 2011 at 01:52:06PM -0700, Bjorn Helgaas wrote:
> On Friday, January 14, 2011 11:19:15 am Ram Pai wrote:
> >     PCI: reserve  additional  resources  for hotplug bridges
> >     only after all essential resources are allocated.
> >     
> >     Linux  tries  to reserve minimal  resources for  hotplug
> >     bridges.  This  works   fine as long as there are enough
> >     resources  to  satisfy  all  other  essential   resource
> >     requirements.  However  if  enough  resources   are  not
> >     available    to    satisfy  both the essential resources 
> >     and   the   reservation  for   hotplug  resources,   the
> >     resource-allocator reports error and returns failure.
> >     
> >     This patch distinguishes between essential resources and
> >     nice-to-have   resources.   Any   failure   to  allocate
> >     nice-to-have resources are ignored.
> >     
> >     This  behavior  can  be  particularly  useful to trigger
> >     automatic  reallocation, if  the  OS  discovers  genuine
> >     resource  allocation  conflicts  or  genuine unallocated
> >     requests caused  by  not-so-smart allocation behavior of 
> >     the native BIOS.
> >     
> >     https://bugzilla.kernel.org/show_bug.cgi?id=15960
> >     captures  the  details  of  the issue and the motivation
> >     behind this patch.
> 
> Please don't indent and right-justify the changelog.

Ooops. I did not see your comments earlier. I just out a patch without
looking at your comments.

> 
> This is a PCI-specific issue, so I'm not comfortable adding add_size
> to struct resource.  That penalizes all resource users while only
> helping PCI.  Also, the struct resource lives forever, and the
> add_size information is only useful while we're configuring the
> bridge.

Any suggestion on how to do this without adding a field to struct resource?

RP
> 
> Bjorn
> 
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > 
> > drivers/pci/setup-bus.c |  142 +++++++++++++++++++++++++++++++++++-------------
> > include/linux/ioport.h  |    1 
> > 2 files changed, 105 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index 66cb8f4..76a7fb7 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -91,7 +91,26 @@ static void __dev_sort_resources(struct pci_dev *dev,
> >  	pdev_sort_resources(dev, head);
> >  }
> >  
> > -static void __assign_resources_sorted(struct resource_list *head,
> > +
> > +static void adjust_resources_sorted(struct resource_list *head)
> > +{
> > +	struct resource *res;
> > +	struct resource_list *list, *tmp;
> > +	int idx;
> > +
> > +	for (list = head->next; list;) {
> > +		res = list->res;
> > +		idx = res - &list->dev->resource[0];
> > +		if (res->add_size)
> > +			adjust_resource(res, res->start, 
> > +				resource_size(res) + res->add_size);
> > +		tmp = list;
> > +		list = list->next;
> > +		kfree(tmp);
> > +	}
> > +}
> > +
> > +static void assign_requested_resources_sorted(struct resource_list *head,
> >  				 struct resource_list_x *fail_head)
> >  {
> >  	struct resource *res;
> > @@ -114,14 +133,25 @@ static void __assign_resources_sorted(struct resource_list *head,
> >  			}
> >  			res->start = 0;
> >  			res->end = 0;
> > +			res->add_size = 0;
> >  			res->flags = 0;
> >  		}
> >  		tmp = list;
> >  		list = list->next;
> > -		kfree(tmp);
> >  	}
> >  }
> >  
> > +static void __assign_resources_sorted(struct resource_list *head,
> > +				 struct resource_list_x *fail_head)
> > +{
> > +	/* Satisfy the must-have resource requests */
> > +	assign_requested_resources_sorted(head, fail_head);
> > +
> > +	/* Try to satisfy any additional nice-to-have resource 
> > +		requests */
> > +	adjust_resources_sorted(head);
> > +}
> > +
> >  static void pdev_assign_resources_sorted(struct pci_dev *dev,
> >  				 struct resource_list_x *fail_head)
> >  {
> > @@ -404,15 +434,37 @@ static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned lon
> >  	return NULL;
> >  }
> >  
> > +static resource_size_t calculate_iosize(resource_size_t size, 
> > +		resource_size_t min_size, 
> > +		resource_size_t size1, 
> > +		resource_size_t old_size, 
> > +		resource_size_t align)
> > +{
> > +	if (size < min_size)
> > +		size = min_size;
> > +	if (old_size == 1 )
> > +		old_size = 0;
> > +	/* To be fixed in 2.5: we should have sort of HAVE_ISA
> > +	   flag in the struct pci_bus. */
> > +#if defined(CONFIG_ISA) || defined(CONFIG_EISA)
> > +	size = (size & 0xff) + ((size & ~0xffUL) << 2);
> > +#endif
> > +	size = ALIGN(size + size1, align);
> > +	if (size < old_size)
> > +		size = old_size;
> > +	return size;
> > +}
> > +
> >  /* Sizing the IO windows of the PCI-PCI bridge is trivial,
> >     since these windows have 4K granularity and the IO ranges
> >     of non-bridge PCI devices are limited to 256 bytes.
> >     We must be careful with the ISA aliasing though. */
> > -static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size)
> > +static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size, 
> > +			resource_size_t add_size)
> >  {
> >  	struct pci_dev *dev;
> >  	struct resource *b_res = find_free_bus_resource(bus, IORESOURCE_IO);
> > -	unsigned long size = 0, size1 = 0, old_size;
> > +	unsigned long size = 0, size1 = 0;
> >  
> >  	if (!b_res)
> >   		return;
> > @@ -435,20 +487,16 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size)
> >  				size1 += r_size;
> >  		}
> >  	}
> > -	if (size < min_size)
> > -		size = min_size;
> > -	old_size = resource_size(b_res);
> > -	if (old_size == 1)
> > -		old_size = 0;
> > -/* To be fixed in 2.5: we should have sort of HAVE_ISA
> > -   flag in the struct pci_bus. */
> > -#if defined(CONFIG_ISA) || defined(CONFIG_EISA)
> > -	size = (size & 0xff) + ((size & ~0xffUL) << 2);
> > -#endif
> > -	size = ALIGN(size + size1, 4096);
> > -	if (size < old_size)
> > -		size = old_size;
> > -	if (!size) {
> > +
> > +	size = calculate_iosize(size, min_size, size1, 
> > +			resource_size(b_res), 4096);
> > +	if (add_size)
> > +		size1 = calculate_iosize(size, min_size+add_size, size1, 
> > +				resource_size(b_res), 4096);
> > +	else
> > +		size1  = size;
> > +
> > +	if (!size && !size1) {
> >  		if (b_res->start || b_res->end)
> >  			dev_info(&bus->self->dev, "disabling bridge window "
> >  				 "%pR to [bus %02x-%02x] (unused)\n", b_res,
> > @@ -459,16 +507,35 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size)
> >  	/* Alignment of the IO window is always 4K */
> >  	b_res->start = 4096;
> >  	b_res->end = b_res->start + size - 1;
> > +	b_res->add_size = size1-size;
> >  	b_res->flags |= IORESOURCE_STARTALIGN;
> >  }
> >  
> > +
> > +static resource_size_t calculate_memsize(resource_size_t size, 
> > +		resource_size_t min_size, 
> > +		resource_size_t size1, 
> > +		resource_size_t old_size, 
> > +		resource_size_t align)
> > +{
> > +	if (size < min_size)
> > +		size = min_size;
> > +	if (old_size == 1 )
> > +		old_size = 0;
> > +	if (size < old_size)
> > +		size = old_size;
> > +	size = ALIGN(size + size1, align);
> > +	return size;
> > +}
> > +
> >  /* Calculate the size of the bus and minimal alignment which
> >     guarantees that all child resources fit in this size. */
> >  static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> > -			 unsigned long type, resource_size_t min_size)
> > +			 unsigned long type, resource_size_t min_size, 
> > +				resource_size_t add_size)
> >  {
> >  	struct pci_dev *dev;
> > -	resource_size_t min_align, align, size, old_size;
> > +	resource_size_t min_align, align, size, size1;
> >  	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
> >  	int order, max_order;
> >  	struct resource *b_res = find_free_bus_resource(bus, type);
> > @@ -516,13 +583,6 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >  			mem64_mask &= r->flags & IORESOURCE_MEM_64;
> >  		}
> >  	}
> > -	if (size < min_size)
> > -		size = min_size;
> > -	old_size = resource_size(b_res);
> > -	if (old_size == 1)
> > -		old_size = 0;
> > -	if (size < old_size)
> > -		size = old_size;
> >  
> >  	align = 0;
> >  	min_align = 0;
> > @@ -537,8 +597,14 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >  			min_align = align1 >> 1;
> >  		align += aligns[order];
> >  	}
> > -	size = ALIGN(size, min_align);
> > -	if (!size) {
> > +
> > +	size = calculate_memsize(size, min_size, 0, resource_size(b_res), align);
> > +	if (add_size)
> > +		size1 = calculate_memsize(size, min_size+add_size, 0, resource_size(b_res), align);
> > +	else
> > +		size1  = size;
> > +
> > +	if (!size && !size1) {
> >  		if (b_res->start || b_res->end)
> >  			dev_info(&bus->self->dev, "disabling bridge window "
> >  				 "%pR to [bus %02x-%02x] (unused)\n", b_res,
> > @@ -548,8 +614,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> >  	}
> >  	b_res->start = min_align;
> >  	b_res->end = size + min_align - 1;
> > -	b_res->flags |= IORESOURCE_STARTALIGN;
> > -	b_res->flags |= mem64_mask;
> > +	b_res->add_size = size1 - size;
> > +	b_res->flags |= IORESOURCE_STARTALIGN | mem64_mask;
> >  	return 1;
> >  }
> >  
> > @@ -606,7 +672,7 @@ void __ref pci_bus_size_bridges(struct pci_bus *bus)
> >  {
> >  	struct pci_dev *dev;
> >  	unsigned long mask, prefmask;
> > -	resource_size_t min_mem_size = 0, min_io_size = 0;
> > +	resource_size_t additional_mem_size = 0, additional_io_size = 0;
> >  
> >  	list_for_each_entry(dev, &bus->devices, bus_list) {
> >  		struct pci_bus *b = dev->subordinate;
> > @@ -637,11 +703,11 @@ void __ref pci_bus_size_bridges(struct pci_bus *bus)
> >  	case PCI_CLASS_BRIDGE_PCI:
> >  		pci_bridge_check_ranges(bus);
> >  		if (bus->self->is_hotplug_bridge) {
> > -			min_io_size  = pci_hotplug_io_size;
> > -			min_mem_size = pci_hotplug_mem_size;
> > +			additional_io_size  = pci_hotplug_io_size;
> > +			additional_mem_size = pci_hotplug_mem_size;
> >  		}
> >  	default:
> > -		pbus_size_io(bus, min_io_size);
> > +		pbus_size_io(bus, 0, additional_io_size);
> >  		/* If the bridge supports prefetchable range, size it
> >  		   separately. If it doesn't, or its prefetchable window
> >  		   has already been allocated by arch code, try
> > @@ -649,11 +715,11 @@ void __ref pci_bus_size_bridges(struct pci_bus *bus)
> >  		   resources. */
> >  		mask = IORESOURCE_MEM;
> >  		prefmask = IORESOURCE_MEM | IORESOURCE_PREFETCH;
> > -		if (pbus_size_mem(bus, prefmask, prefmask, min_mem_size))
> > +		if (pbus_size_mem(bus, prefmask, prefmask, 0, additional_mem_size))
> >  			mask = prefmask; /* Success, size non-prefetch only. */
> >  		else
> > -			min_mem_size += min_mem_size;
> > -		pbus_size_mem(bus, mask, IORESOURCE_MEM, min_mem_size);
> > +			additional_mem_size += additional_mem_size;
> > +		pbus_size_mem(bus, mask, IORESOURCE_MEM, 0, additional_mem_size);
> >  		break;
> >  	}
> >  }
> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > index e9bb22c..d00c61c 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -18,6 +18,7 @@
> >  struct resource {
> >  	resource_size_t start;
> >  	resource_size_t end;
> > +	resource_size_t add_size;
> >  	const char *name;
> >  	unsigned long flags;
> >  	struct resource *parent, *sibling, *child;
> > 
> --
> 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

-- 
Ram Pai
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
linuxram@us.ibm.com