Re: [PATCH 1/4] PCI: refactor io size calculation code

Re: [PATCH 1/4] PCI: refactor io size calculation code

[Daniel Hellstrom]

>
>
>refactor  code  that  calculates  the  io  size in pbus_size_io()
>and pbus_mem_io() into separate functions.
>
>Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
>---
> drivers/pci/setup-bus.c |   66 ++++++++++++++++++++++++++++++-----------------
> 1 files changed, 42 insertions(+), 24 deletions(-)
>
>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>index 66cb8f4..2121215 100644
>--- a/drivers/pci/setup-bus.c
>+++ b/drivers/pci/setup-bus.c
>@@ -404,6 +404,43 @@ static struct resource *find_free_bus_resource(struct pci_bus *bus, unsigned lon
> 	return NULL;
> }
>
...

>+
>+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;
>+}
>+
> /* 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.
>
>  
>
...

>@@ -516,14 +542,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;
> 	for (order = 0; order <= max_order; order++) {
>@@ -537,7 +555,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> 			min_align = align1 >> 1;
> 		align += aligns[order];
> 	}
>-	size = ALIGN(size, min_align);
>+	size = calculate_memsize(size, min_size, 0, resource_size(b_res), align);
>  
>
On my SPARC32/LEON4 PCI system I get overlapped areas, double mapped 
resources. Some BARs on PCIBUS0 are in the same non-prefetchable memory 
range as the secondary bus PCIBUS1. Changing align to min_align in the 
above call to calculate_memsize() fixes the problem, and the memory 
allocation is the same as with 2.6.36.4 kernel.

I belive this is just a typo.

Best Regards,
Daniel Hellstrom
Aeroflex Gaisler

> 	if (!size) {
> 		if (b_res->start || b_res->end)
> 			dev_info(&bus->self->dev, "disabling bridge window "
>-- 
>1.6.5.2
>

Re: [PATCH 1/4] PCI: refactor io size calculation code

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 3059 bytes --]

On Mon, Apr 11, 2011 at 6:18 AM, Daniel Hellstrom <daniel@gaisler.com> wrote:
>In commit 13583b16592a ("PCI: refactor io size calculation code"):
>>
>> refactor  code  that  calculates  the  io  size in pbus_size_io()
>> and pbus_mem_io() into separate functions.
>>
>> Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
>> ---
>> drivers/pci/setup-bus.c |   66
>> ++++++++++++++++++++++++++++++-----------------
>> 1 files changed, 42 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 66cb8f4..2121215 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -404,6 +404,43 @@ static struct resource *find_free_bus_resource(struct
>> pci_bus *bus, unsigned lon
>>        return NULL;
>> }
>>
> ...
>
>> +
>> +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;
>> +}
>> +
>> /* 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.
>>
>>
>
> ...
>
>> @@ -516,14 +542,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;
>>        for (order = 0; order <= max_order; order++) {
>> @@ -537,7 +555,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
>> long mask,
>>                        min_align = align1 >> 1;
>>                align += aligns[order];
>>        }
>> -       size = ALIGN(size, min_align);
>> +       size = calculate_memsize(size, min_size, 0, resource_size(b_res),
>> align);
>>
>
> On my SPARC32/LEON4 PCI system I get overlapped areas, double mapped
> resources. Some BARs on PCIBUS0 are in the same non-prefetchable memory
> range as the secondary bus PCIBUS1. Changing align to min_align in the above
> call to calculate_memsize() fixes the problem, and the memory allocation is
> the same as with 2.6.36.4 kernel.
>
> I belive this is just a typo.

It does seem that way. The original code used 'min_align' and 'align'
itself is meaningless in that place.

Can you confirm that the patch you talk about as fixing things is the attached?

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 827 bytes --]

 drivers/pci/setup-bus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 89d0a6a88df7..ebf51ad1b714 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -676,10 +676,10 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			min_align = align1 >> 1;
 		align += aligns[order];
 	}
-	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), align);
+	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
 	size1 = !add_size ? size :
 		calculate_memsize(size, min_size+add_size, 0,
-				resource_size(b_res), align);
+				resource_size(b_res), min_align);
 	if (!size0 && !size1) {
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window "

Re: [PATCH 1/4] PCI: refactor io size calculation code

[Daniel Hellstrom]

Linus Torvalds wrote:

>On Mon, Apr 11, 2011 at 6:18 AM, Daniel Hellstrom <daniel@gaisler.com> wrote:
>  
>
>>In commit 13583b16592a ("PCI: refactor io size calculation code"):
>>    
>>
>>>refactor  code  that  calculates  the  io  size in pbus_size_io()
>>>and pbus_mem_io() into separate functions.
>>>
>>>Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
>>>---
>>>drivers/pci/setup-bus.c |   66
>>>++++++++++++++++++++++++++++++-----------------
>>>1 files changed, 42 insertions(+), 24 deletions(-)
>>>
>>>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>>>index 66cb8f4..2121215 100644
>>>--- a/drivers/pci/setup-bus.c
>>>+++ b/drivers/pci/setup-bus.c
>>>@@ -404,6 +404,43 @@ static struct resource *find_free_bus_resource(struct
>>>pci_bus *bus, unsigned lon
>>>       return NULL;
>>>}
>>>
>>>      
>>>
>>...
>>
>>    
>>
>>>+
>>>+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;
>>>+}
>>>+
>>>/* 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.
>>>
>>>
>>>      
>>>
>>...
>>
>>    
>>
>>>@@ -516,14 +542,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;
>>>       for (order = 0; order <= max_order; order++) {
>>>@@ -537,7 +555,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
>>>long mask,
>>>                       min_align = align1 >> 1;
>>>               align += aligns[order];
>>>       }
>>>-       size = ALIGN(size, min_align);
>>>+       size = calculate_memsize(size, min_size, 0, resource_size(b_res),
>>>align);
>>>
>>>      
>>>
>>On my SPARC32/LEON4 PCI system I get overlapped areas, double mapped
>>resources. Some BARs on PCIBUS0 are in the same non-prefetchable memory
>>range as the secondary bus PCIBUS1. Changing align to min_align in the above
>>call to calculate_memsize() fixes the problem, and the memory allocation is
>>the same as with 2.6.36.4 kernel.
>>
>>I belive this is just a typo.
>>    
>>
>
>It does seem that way. The original code used 'min_align' and 'align'
>itself is meaningless in that place.
>  
>
Agree.

>Can you confirm that the patch you talk about as fixing things is the attached?
>
>                  Linus
>  
>
Yes, the below patch works on the system that triggered the problem 
before. The last min_align was added in a later patch, I missed it 
initially.

Thanks,
Daniel

>------------------------------------------------------------------------
>
> drivers/pci/setup-bus.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>index 89d0a6a88df7..ebf51ad1b714 100644
>--- a/drivers/pci/setup-bus.c
>+++ b/drivers/pci/setup-bus.c
>@@ -676,10 +676,10 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
> 			min_align = align1 >> 1;
> 		align += aligns[order];
> 	}
>-	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), align);
>+	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
> 	size1 = !add_size ? size :
> 		calculate_memsize(size, min_size+add_size, 0,
>-				resource_size(b_res), align);
>+				resource_size(b_res), min_align);
> 	if (!size0 && !size1) {
> 		if (b_res->start || b_res->end)
> 			dev_info(&bus->self->dev, "disabling bridge window "
>  
>

Re: [PATCH 1/4] PCI: refactor io size calculation code

[Ram Pai]

On Mon, Apr 11, 2011 at 07:42:13AM -0700, Linus Torvalds wrote:
> On Mon, Apr 11, 2011 at 6:18 AM, Daniel Hellstrom <daniel@gaisler.com> wrote:
> >In commit 13583b16592a ("PCI: refactor io size calculation code"):
> >>
> >> refactor  code  that  calculates  the  io  size in pbus_size_io()
> >> and pbus_mem_io() into separate functions.
> >>
> >> Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx>
> >> ---
> >> drivers/pci/setup-bus.c |   66
> >> ++++++++++++++++++++++++++++++-----------------
> >> 1 files changed, 42 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> >> index 66cb8f4..2121215 100644
> >> --- a/drivers/pci/setup-bus.c
> >> +++ b/drivers/pci/setup-bus.c
> >> @@ -404,6 +404,43 @@ static struct resource *find_free_bus_resource(struct
> >> pci_bus *bus, unsigned lon
> >>        return NULL;
> >> }
> >>
> > ...
> >
> >> +
> >> +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;
> >> +}
> >> +
> >> /* 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.
> >>
> >>
> >
> > ...
> >
> >> @@ -516,14 +542,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;
> >>        for (order = 0; order <= max_order; order++) {
> >> @@ -537,7 +555,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
> >> long mask,
> >>                        min_align = align1 >> 1;
> >>                align += aligns[order];
> >>        }
> >> -       size = ALIGN(size, min_align);
> >> +       size = calculate_memsize(size, min_size, 0, resource_size(b_res),
> >> align);
> >>
> >
> > On my SPARC32/LEON4 PCI system I get overlapped areas, double mapped
> > resources. Some BARs on PCIBUS0 are in the same non-prefetchable memory
> > range as the secondary bus PCIBUS1. Changing align to min_align in the above
> > call to calculate_memsize() fixes the problem, and the memory allocation is
> > the same as with 2.6.36.4 kernel.
> >
> > I belive this is just a typo.


Yes it is my mistake. I got the parameter wrong. Your fix should correct the problem.

RP

[PATCH 0/4] PCI: preallocate resource patch series

[Ram Pai]

    Linux tries to pre-allocate minimal resources to hotplug bridges. This
    works fine as long as there are enough resources  to satisfy all other
    genuine resource requirements. However if enough resources are not
    available to satisfy any of these nice-to-have pre-allocations, the
    resource-allocator reports errors and returns failure.

    This patch distinguishes between must-have resource from nice-to-have
    resource.  Any failure to allocate nice-to-have resources are ignored.

    This behavior can be particularly useful to trigger automatic
    reallocation when the OS discovers genuine allocation-conflicts
    or genuine unallocated-requests caused by buggy allocation behavior
    of the native BIOS/uEFI.

    https://bugzilla.kernel.org/show_bug.cgi?id=15960 captures the movitation
    behind the patch. This patch is verified to resolve the above bug.

    The first 3 patches refactors common code and builds the ground work
    for the 4 patch which contains the core piece of the fix.

    patch 1/4: refactors the io size calculation logic
    patch 2/4: refactors the list freeing logic
    patch 3/4: refactors the resource reset logic
    patch 4/4: contains the fix to the original problem

    drivers/pci/setup-bus.c   |  269 ++++++++++++++++++++++++++++++++++------------

    Signed-off-by: Ram Pai <linuxram@us.ibm.com>

[PATCH 1/4] PCI: refactor io size calculation code

[Ram Pai]

refactor  code  that  calculates  the  io  size in pbus_size_io()
and pbus_mem_io() into separate functions.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 drivers/pci/setup-bus.c |   66 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 66cb8f4..2121215 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -404,6 +404,43 @@ 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;
+}
+
+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;
+}
+
 /* 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.
@@ -412,7 +449,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_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,19 +472,8 @@ 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;
+	size = calculate_iosize(size, min_size, size1,
+			resource_size(b_res), 4096);
 	if (!size) {
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window "
@@ -468,7 +494,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			 unsigned long type, resource_size_t min_size)
 {
 	struct pci_dev *dev;
-	resource_size_t min_align, align, size, old_size;
+	resource_size_t min_align, align, size;
 	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,14 +542,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;
 	for (order = 0; order <= max_order; order++) {
@@ -537,7 +555,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			min_align = align1 >> 1;
 		align += aligns[order];
 	}
-	size = ALIGN(size, min_align);
+	size = calculate_memsize(size, min_size, 0, resource_size(b_res), align);
 	if (!size) {
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window "
-- 
1.6.5.2