Re: [PATCH] pci: Allow very large resource windows

Re: [PATCH] pci: Allow very large resource windows

[Guo Chao]

>On Mon, May 19, 2014 at 02:03:14PM +0100, Alan wrote:
>> From: Alan <alan@xxxxxxxxxxxxxxx>
>> 
>> This is needed for some of the Xeon Phi type systems.
>> 
>> Signed-off-by: Alan Cox <alan@xxxxxxxxxxxxxxx>
>
> I applied this to my pci/resource branch for v3.16.  Nikhil
> posted essentially the same patch a couple years ago, so I added
> his signed-off-by and adopted his use of ARRAY_SIZE() to connect the
> "order > 13" test with the aligns[] declaration.

Well, that's not enough for our 16G BAR ...

I thought Yinghai fixed it permanently last time:

	http://permalink.gmane.org/gmane.linux.kernel.pci/29142

Are you still working on this, Yinghai?

Thanks,
Guo Chao

Re: [PATCH] pci: Allow very large resource windows

[Yinghai Lu]

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

On Tue, Jun 10, 2014 at 11:01 PM, Guo Chao <yan@linux.vnet.ibm.com> wrote:
> Well, that's not enough for our 16G BAR ...
>
> I thought Yinghai fixed it permanently last time:
>
>         http://permalink.gmane.org/gmane.linux.kernel.pci/29142
>
> Are you still working on this, Yinghai?

Hi Guo,

Please check updated version on your setup.

Thanks

Yinghai

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

Subject: [PATCH -v3] PCI: Fix bus align checking to support more than 8G

So add mmio64 mask checking, only allow more than 4G when bridge and all child
support 64bit res.  Still keep old 2G checking for other path.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

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
@@ -928,15 +928,50 @@ static int pbus_size_mem(struct pci_bus
 {
 	struct pci_dev *dev;
 	resource_size_t min_align, align, size, size0, size1;
-	resource_size_t aligns[14];	/* Alignments from 1Mb to 8Gb */
+	resource_size_t aligns[44];	/* Alignments from 1Mb to 2^63 */
 	int order, max_order;
 	struct resource *b_res = find_free_bus_resource(bus,
 					mask | IORESOURCE_PREFETCH, type);
 	resource_size_t children_add_size = 0;
+	unsigned int mem64_mask;
 
 	if (!b_res)
 		return -ENOSPC;
 
+	mem64_mask = b_res->flags & IORESOURCE_MEM_64;
+
+	/* kernel does not support 64bit res */
+	if (sizeof(resource_size_t) == 4)
+		mem64_mask &= ~IORESOURCE_MEM_64;
+
+	if (!mem64_mask)
+		goto mem64_mask_check_done;
+
+	/* check if mem64 support is supported and needed at first */
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		int i;
+
+		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+			struct resource *r = &dev->resource[i];
+
+			if (r->parent || ((r->flags & mask) != type &&
+					  (r->flags & mask) != type2 &&
+					  (r->flags & mask) != type3))
+				continue;
+#ifdef CONFIG_PCI_IOV
+			/* Skip SRIOV checking, as optional res */
+			if (realloc_head && i >= PCI_IOV_RESOURCES &&
+					i <= PCI_IOV_RESOURCE_END)
+				continue;
+#endif
+			mem64_mask &= r->flags & IORESOURCE_MEM_64;
+
+			if (!mem64_mask)
+				goto mem64_mask_check_done;
+		}
+	}
+mem64_mask_check_done:
+
 	memset(aligns, 0, sizeof(aligns));
 	max_order = 0;
 	size = 0;
@@ -973,7 +1008,8 @@ static int pbus_size_mem(struct pci_bus
 			order = __ffs(align) - 20;
 			if (order < 0)
 				order = 0;
-			if (order >= ARRAY_SIZE(aligns)) {
+			if (order >= ARRAY_SIZE(aligns) ||
+			    (!mem64_mask && order > 11 /* 2Gb */)) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
 					 (unsigned long long) align);

Re: [PATCH] pci: Allow very large resource windows

[Guo Chao]

On Wed, Jun 11, 2014 at 10:23:35AM -0700, Yinghai Lu wrote:
> On Tue, Jun 10, 2014 at 11:01 PM, Guo Chao <yan@linux.vnet.ibm.com> wrote:
> > Well, that's not enough for our 16G BAR ...
> >
> > I thought Yinghai fixed it permanently last time:
> >
> >         http://permalink.gmane.org/gmane.linux.kernel.pci/29142
> >
> > Are you still working on this, Yinghai?
> 
> Hi Guo,
> 
> Please check updated version on your setup.
> 
> Thanks
> 
> Yinghai

> Subject: [PATCH -v3] PCI: Fix bus align checking to support more than 8G
> 
> So add mmio64 mask checking, only allow more than 4G when bridge and all child
> support 64bit res.  Still keep old 2G checking for other path.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 

Tested-by: Guo Chao <yan@linux.vnet.ibm.com>

Thanks

> ---
>  drivers/pci/setup-bus.c |   40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> 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
> @@ -928,15 +928,50 @@ static int pbus_size_mem(struct pci_bus
>  {
>  	struct pci_dev *dev;
>  	resource_size_t min_align, align, size, size0, size1;
> -	resource_size_t aligns[14];	/* Alignments from 1Mb to 8Gb */
> +	resource_size_t aligns[44];	/* Alignments from 1Mb to 2^63 */
>  	int order, max_order;
>  	struct resource *b_res = find_free_bus_resource(bus,
>  					mask | IORESOURCE_PREFETCH, type);
>  	resource_size_t children_add_size = 0;
> +	unsigned int mem64_mask;
>  
>  	if (!b_res)
>  		return -ENOSPC;
>  
> +	mem64_mask = b_res->flags & IORESOURCE_MEM_64;
> +
> +	/* kernel does not support 64bit res */
> +	if (sizeof(resource_size_t) == 4)
> +		mem64_mask &= ~IORESOURCE_MEM_64;
> +
> +	if (!mem64_mask)
> +		goto mem64_mask_check_done;
> +
> +	/* check if mem64 support is supported and needed at first */
> +	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		int i;
> +
> +		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> +			struct resource *r = &dev->resource[i];
> +
> +			if (r->parent || ((r->flags & mask) != type &&
> +					  (r->flags & mask) != type2 &&
> +					  (r->flags & mask) != type3))
> +				continue;
> +#ifdef CONFIG_PCI_IOV
> +			/* Skip SRIOV checking, as optional res */
> +			if (realloc_head && i >= PCI_IOV_RESOURCES &&
> +					i <= PCI_IOV_RESOURCE_END)
> +				continue;
> +#endif
> +			mem64_mask &= r->flags & IORESOURCE_MEM_64;
> +
> +			if (!mem64_mask)
> +				goto mem64_mask_check_done;
> +		}
> +	}
> +mem64_mask_check_done:
> +
>  	memset(aligns, 0, sizeof(aligns));
>  	max_order = 0;
>  	size = 0;
> @@ -973,7 +1008,8 @@ static int pbus_size_mem(struct pci_bus
>  			order = __ffs(align) - 20;
>  			if (order < 0)
>  				order = 0;
> -			if (order >= ARRAY_SIZE(aligns)) {
> +			if (order >= ARRAY_SIZE(aligns) ||
> +			    (!mem64_mask && order > 11 /* 2Gb */)) {
>  				dev_warn(&dev->dev, "disabling BAR %d: %pR "
>  					 "(bad alignment %#llx)\n", i, r,
>  					 (unsigned long long) align);

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Wed, Jun 11, 2014 at 10:23:35AM -0700, Yinghai Lu wrote:
> On Tue, Jun 10, 2014 at 11:01 PM, Guo Chao <yan@linux.vnet.ibm.com> wrote:
> > Well, that's not enough for our 16G BAR ...
> >
> > I thought Yinghai fixed it permanently last time:
> >
> >         http://permalink.gmane.org/gmane.linux.kernel.pci/29142
> >
> > Are you still working on this, Yinghai?
> 
> Hi Guo,
> 
> Please check updated version on your setup.
> 
> Thanks
> 
> Yinghai

> Subject: [PATCH -v3] PCI: Fix bus align checking to support more than 8G
> 
> So add mmio64 mask checking, only allow more than 4G when bridge and all child
> support 64bit res.  Still keep old 2G checking for other path.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/setup-bus.c |   40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> 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
> @@ -928,15 +928,50 @@ static int pbus_size_mem(struct pci_bus
>  {
>  	struct pci_dev *dev;
>  	resource_size_t min_align, align, size, size0, size1;
> -	resource_size_t aligns[14];	/* Alignments from 1Mb to 8Gb */
> +	resource_size_t aligns[44];	/* Alignments from 1Mb to 2^63 */
>  	int order, max_order;
>  	struct resource *b_res = find_free_bus_resource(bus,
>  					mask | IORESOURCE_PREFETCH, type);
>  	resource_size_t children_add_size = 0;
> +	unsigned int mem64_mask;
>  
>  	if (!b_res)
>  		return -ENOSPC;
>  
> +	mem64_mask = b_res->flags & IORESOURCE_MEM_64;
> +
> +	/* kernel does not support 64bit res */
> +	if (sizeof(resource_size_t) == 4)
> +		mem64_mask &= ~IORESOURCE_MEM_64;
> +
> +	if (!mem64_mask)
> +		goto mem64_mask_check_done;
> +
> +	/* check if mem64 support is supported and needed at first */
> +	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		int i;
> +
> +		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> +			struct resource *r = &dev->resource[i];
> +
> +			if (r->parent || ((r->flags & mask) != type &&
> +					  (r->flags & mask) != type2 &&
> +					  (r->flags & mask) != type3))
> +				continue;
> +#ifdef CONFIG_PCI_IOV
> +			/* Skip SRIOV checking, as optional res */
> +			if (realloc_head && i >= PCI_IOV_RESOURCES &&
> +					i <= PCI_IOV_RESOURCE_END)
> +				continue;
> +#endif
> +			mem64_mask &= r->flags & IORESOURCE_MEM_64;
> +
> +			if (!mem64_mask)
> +				goto mem64_mask_check_done;
> +		}
> +	}
> +mem64_mask_check_done:

What happens if we increase the size of aligns[], but omit this loop and
the mem64_mask stuff?  Is mem64_mask just an optimization, so the code
would still work without it?  If the existing code works for 8GB bars
(without mem64_mask), when does it break and start requiring mem64_mask?

>  	memset(aligns, 0, sizeof(aligns));
>  	max_order = 0;
>  	size = 0;
> @@ -973,7 +1008,8 @@ static int pbus_size_mem(struct pci_bus
>  			order = __ffs(align) - 20;
>  			if (order < 0)
>  				order = 0;
> -			if (order >= ARRAY_SIZE(aligns)) {
> +			if (order >= ARRAY_SIZE(aligns) ||
> +			    (!mem64_mask && order > 11 /* 2Gb */)) {
>  				dev_warn(&dev->dev, "disabling BAR %d: %pR "
>  					 "(bad alignment %#llx)\n", i, r,
>  					 (unsigned long long) align);

Re: [PATCH] pci: Allow very large resource windows

[Yinghai Lu]

On Wed, Jul 2, 2014 at 2:07 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> Subject: [PATCH -v3] PCI: Fix bus align checking to support more than 8G
>>
>> So add mmio64 mask checking, only allow more than 4G when bridge and all child
>> support 64bit res.  Still keep old 2G checking for other path.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  drivers/pci/setup-bus.c |   40 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> 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
>> @@ -928,15 +928,50 @@ static int pbus_size_mem(struct pci_bus
>>  {
>>       struct pci_dev *dev;
>>       resource_size_t min_align, align, size, size0, size1;
>> -     resource_size_t aligns[14];     /* Alignments from 1Mb to 8Gb */
>> +     resource_size_t aligns[44];     /* Alignments from 1Mb to 2^63 */
>>       int order, max_order;
>>       struct resource *b_res = find_free_bus_resource(bus,
>>                                       mask | IORESOURCE_PREFETCH, type);
>>       resource_size_t children_add_size = 0;
>> +     unsigned int mem64_mask;
>>
>>       if (!b_res)
>>               return -ENOSPC;
>>
>> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
>> +
>> +     /* kernel does not support 64bit res */
>> +     if (sizeof(resource_size_t) == 4)
>> +             mem64_mask &= ~IORESOURCE_MEM_64;
>> +
>> +     if (!mem64_mask)
>> +             goto mem64_mask_check_done;
>> +
>> +     /* check if mem64 support is supported and needed at first */
>> +     list_for_each_entry(dev, &bus->devices, bus_list) {
>> +             int i;
>> +
>> +             for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>> +                     struct resource *r = &dev->resource[i];
>> +
>> +                     if (r->parent || ((r->flags & mask) != type &&
>> +                                       (r->flags & mask) != type2 &&
>> +                                       (r->flags & mask) != type3))
>> +                             continue;
>> +#ifdef CONFIG_PCI_IOV
>> +                     /* Skip SRIOV checking, as optional res */
>> +                     if (realloc_head && i >= PCI_IOV_RESOURCES &&
>> +                                     i <= PCI_IOV_RESOURCE_END)
>> +                             continue;
>> +#endif
>> +                     mem64_mask &= r->flags & IORESOURCE_MEM_64;
>> +
>> +                     if (!mem64_mask)
>> +                             goto mem64_mask_check_done;
>> +             }
>> +     }
>> +mem64_mask_check_done:
>
> What happens if we increase the size of aligns[], but omit this loop and
> the mem64_mask stuff?  Is mem64_mask just an optimization, so the code
> would still work without it?  If the existing code works for 8GB bars
> (without mem64_mask), when does it break and start requiring mem64_mask?

Yes, you are right. with current version __pci_bus_size_bridges(), we could
get rid of the checking. As child 32bit pref mmio will go underwith
bridge 32bit non-pref mmio.

We only need keep

>> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
>> +
>> +     /* kernel does not support 64bit res */
>> +     if (sizeof(resource_size_t) == 4)
>> +             mem64_mask &= ~IORESOURCE_MEM_64;

Let me know if you want me to send new version.

Thanks

Yinghai

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Wed, Jul 02, 2014 at 03:54:29PM -0700, Yinghai Lu wrote:
> On Wed, Jul 2, 2014 at 2:07 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >
> >> Subject: [PATCH -v3] PCI: Fix bus align checking to support more than 8G
> >>
> >> So add mmio64 mask checking, only allow more than 4G when bridge and all child
> >> support 64bit res.  Still keep old 2G checking for other path.
> >>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>
> >> ---
> >>  drivers/pci/setup-bus.c |   40 ++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 38 insertions(+), 2 deletions(-)
> >>
> >> 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
> >> @@ -928,15 +928,50 @@ static int pbus_size_mem(struct pci_bus
> >>  {
> >>       struct pci_dev *dev;
> >>       resource_size_t min_align, align, size, size0, size1;
> >> -     resource_size_t aligns[14];     /* Alignments from 1Mb to 8Gb */
> >> +     resource_size_t aligns[44];     /* Alignments from 1Mb to 2^63 */
> >>       int order, max_order;
> >>       struct resource *b_res = find_free_bus_resource(bus,
> >>                                       mask | IORESOURCE_PREFETCH, type);
> >>       resource_size_t children_add_size = 0;
> >> +     unsigned int mem64_mask;
> >>
> >>       if (!b_res)
> >>               return -ENOSPC;
> >>
> >> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
> >> +
> >> +     /* kernel does not support 64bit res */
> >> +     if (sizeof(resource_size_t) == 4)
> >> +             mem64_mask &= ~IORESOURCE_MEM_64;
> >> +
> >> +     if (!mem64_mask)
> >> +             goto mem64_mask_check_done;
> >> +
> >> +     /* check if mem64 support is supported and needed at first */
> >> +     list_for_each_entry(dev, &bus->devices, bus_list) {
> >> +             int i;
> >> +
> >> +             for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> >> +                     struct resource *r = &dev->resource[i];
> >> +
> >> +                     if (r->parent || ((r->flags & mask) != type &&
> >> +                                       (r->flags & mask) != type2 &&
> >> +                                       (r->flags & mask) != type3))
> >> +                             continue;
> >> +#ifdef CONFIG_PCI_IOV
> >> +                     /* Skip SRIOV checking, as optional res */
> >> +                     if (realloc_head && i >= PCI_IOV_RESOURCES &&
> >> +                                     i <= PCI_IOV_RESOURCE_END)
> >> +                             continue;
> >> +#endif
> >> +                     mem64_mask &= r->flags & IORESOURCE_MEM_64;
> >> +
> >> +                     if (!mem64_mask)
> >> +                             goto mem64_mask_check_done;
> >> +             }
> >> +     }
> >> +mem64_mask_check_done:
> >
> > What happens if we increase the size of aligns[], but omit this loop and
> > the mem64_mask stuff?  Is mem64_mask just an optimization, so the code
> > would still work without it?  If the existing code works for 8GB bars
> > (without mem64_mask), when does it break and start requiring mem64_mask?
> 
> Yes, you are right. with current version __pci_bus_size_bridges(), we could
> get rid of the checking. As child 32bit pref mmio will go underwith
> bridge 32bit non-pref mmio.
> 
> We only need keep
> 
> >> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
> >> +
> >> +     /* kernel does not support 64bit res */
> >> +     if (sizeof(resource_size_t) == 4)
> >> +             mem64_mask &= ~IORESOURCE_MEM_64;

I think you're fixing two things at once, and they should be split
into two separate patches:

  1) Change aligns[] size to increase support alignment from 8GB to 2^63

     I'm not sure about going all the way to aligns[44].  That array
     by itself puts 352 bytes in the stack frame (240 of which are
     added by this patch), which seems excessive.  I suspect that
     supporting BARs up to 64GB or 128GB would be enough for the
     foreseeable future.

  2) Adding mem64_mask

     I think the idea is that even if we have a 64-bit window, we
     can't use anything above 4GB if we only have 32-bit resources.
     That's true, but I don't think we can enforce that in
     pbus_size_mem() because we're only figuring out how much space is
     needed; we have no idea where that space will be allocated.

And I think there are more problems:

  - I don't think we handle overflow of "size" correctly.  Assume that
    we have BARs of 2GB, 2GB, and 8GB.  If we have 32-bit resources,
    when we add those up, it will overflow and we'll mistakenly think
    we only need 8MB.

  - We shouldn't set "r->flags = 0".  The warning says we're disabling
    the BAR, but this *doesn't* disable the BAR, and in fact, there is
    no way to disable a single BAR.  What we can do is turn off
    PCI_COMMAND_MEMORY to disable all the memory BARs for the device.
    And to do that, we need to keep IORESOURCE_MEM in r->flags so
    pci_enable_resources() can tell that this is a memory BAR.

Bjorn

Re: [PATCH] pci: Allow very large resource windows

[Yinghai Lu]

On Thu, Jul 3, 2014 at 6:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> >> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
>> >> +
>> >> +     /* kernel does not support 64bit res */
>> >> +     if (sizeof(resource_size_t) == 4)
>> >> +             mem64_mask &= ~IORESOURCE_MEM_64;
>
> I think you're fixing two things at once, and they should be split
> into two separate patches:
>
>   1) Change aligns[] size to increase support alignment from 8GB to 2^63
>
>      I'm not sure about going all the way to aligns[44].  That array
>      by itself puts 352 bytes in the stack frame (240 of which are
>      added by this patch), which seems excessive.  I suspect that
>      supporting BARs up to 64GB or 128GB would be enough for the
>      foreseeable future.

ok, let's use 128G this time.

>
>   2) Adding mem64_mask
>
>      I think the idea is that even if we have a 64-bit window, we
>      can't use anything above 4GB if we only have 32-bit resources.
>      That's true, but I don't think we can enforce that in
>      pbus_size_mem() because we're only figuring out how much space is
>      needed; we have no idea where that space will be allocated.

with current version of __pci_bus_size_bridges, we separate pref_mem64 with
pref_mem32 to calling pbus_size_mem.

so if we have pref 64bit window, we will only size pref 64 bit
children under it.
and will only assign pref 64bit mem64 to them late.

If the bridge does not support 64bit pref windows, and child support
64bit pref mmio, then bridge will try to use pref 32 window, in that
case, .... could have
size overflow as you state follow.

>
> And I think there are more problems:
>
>   - I don't think we handle overflow of "size" correctly.  Assume that
>     we have BARs of 2GB, 2GB, and 8GB.  If we have 32-bit resources,
>     when we add those up, it will overflow and we'll mistakenly think
>     we only need 8MB.

in this case we should check the overflow.

>
>   - We shouldn't set "r->flags = 0".  The warning says we're disabling
>     the BAR, but this *doesn't* disable the BAR, and in fact, there is
>     no way to disable a single BAR.  What we can do is turn off
>     PCI_COMMAND_MEMORY to disable all the memory BARs for the device.
>     And to do that, we need to keep IORESOURCE_MEM in r->flags so
>     pci_enable_resources() can tell that this is a memory BAR.

when we try to size it,  means that bar is not assigned. with r->flags
= 0, means
we will ignore it all the way.

Thanks

Yinghai

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Thu, Jul 03, 2014 at 12:59:21PM -0700, Yinghai Lu wrote:
> On Thu, Jul 3, 2014 at 6:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>
> >> >> +     mem64_mask = b_res->flags & IORESOURCE_MEM_64;
> >> >> +
> >> >> +     /* kernel does not support 64bit res */
> >> >> +     if (sizeof(resource_size_t) == 4)
> >> >> +             mem64_mask &= ~IORESOURCE_MEM_64;
> >
> > I think you're fixing two things at once, and they should be split
> > into two separate patches:
> >
> >   1) Change aligns[] size to increase support alignment from 8GB to 2^63
> >
> >      I'm not sure about going all the way to aligns[44].  That array
> >      by itself puts 352 bytes in the stack frame (240 of which are
> >      added by this patch), which seems excessive.  I suspect that
> >      supporting BARs up to 64GB or 128GB would be enough for the
> >      foreseeable future.
> 
> ok, let's use 128G this time.
> 
> >
> >   2) Adding mem64_mask
> >
> >      I think the idea is that even if we have a 64-bit window, we
> >      can't use anything above 4GB if we only have 32-bit resources.
> >      That's true, but I don't think we can enforce that in
> >      pbus_size_mem() because we're only figuring out how much space is
> >      needed; we have no idea where that space will be allocated.
> 
> with current version of __pci_bus_size_bridges, we separate pref_mem64 with
> pref_mem32 to calling pbus_size_mem.
> 
> so if we have pref 64bit window, we will only size pref 64 bit
> children under it.
> and will only assign pref 64bit mem64 to them late.
> 
> If the bridge does not support 64bit pref windows, and child support
> 64bit pref mmio, then bridge will try to use pref 32 window, in that
> case, .... could have
> size overflow as you state follow.

This problem has nothing to do with overflow.  We're concerned with
IORESOURCE_MEM_64, which tells us the width of the bridge window
registers: obviously we can't put a 64-bit bus address in a 32-bit
window register.

pbus_size_mem() figures out how much space we need.  If we need 4GB or
more and the kernel only supports 32-bit resources, we can fail
immediately because we can't describe a 4GB window in a 32-bit
resource.

But if we need less than 4GB, pbus_size_mem() should succeed.  We
might fail *later*, if we can't allocate bus addresses that fit in a
32-bit bridge window register.  But in pbus_size_mem(), we have no
idea where the space will come from.

So I don't think there's any point in looking at the sizes of
individual BARs in pbus_size_mem().  We should only look at the total
size required.

> > And I think there are more problems:
> >
> >   - I don't think we handle overflow of "size" correctly.  Assume that
> >     we have BARs of 2GB, 2GB, and 8GB.  If we have 32-bit resources,
> >     when we add those up, it will overflow and we'll mistakenly think
> >     we only need 8MB.
> 
> in this case we should check the overflow.
> 
> >
> >   - We shouldn't set "r->flags = 0".  The warning says we're disabling
> >     the BAR, but this *doesn't* disable the BAR, and in fact, there is
> >     no way to disable a single BAR.  What we can do is turn off
> >     PCI_COMMAND_MEMORY to disable all the memory BARs for the device.
> >     And to do that, we need to keep IORESOURCE_MEM in r->flags so
> >     pci_enable_resources() can tell that this is a memory BAR.
> 
> when we try to size it,  means that bar is not assigned. with r->flags
> = 0, means
> we will ignore it all the way.

With "r->flags = 0", we will not try to assign resources to the BAR,
but the hardware BAR still exists and contains some address.  If the
device has other memory BARs, pci_enable_resources() will turn on
PCI_COMMAND_MEMORY.  Now the "r->flags == 0" BAR is enabled but the
address it contains might conflict with other devices.  That's the
problem.

To fix this, I think we need to do "r->flags |= IORESOURCE_UNSET |
IORESOURCE_DISABLED".

Bjorn

Re: [PATCH] pci: Allow very large resource windows

[Yinghai Lu]

On Thu, Jul 3, 2014 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Jul 03, 2014 at 12:59:21PM -0700, Yinghai Lu wrote:

>> when we try to size it,  means that bar is not assigned. with r->flags
>> = 0, means
>> we will ignore it all the way.
>
> With "r->flags = 0", we will not try to assign resources to the BAR,
> but the hardware BAR still exists and contains some address.  If the
> device has other memory BARs, pci_enable_resources() will turn on
> PCI_COMMAND_MEMORY.  Now the "r->flags == 0" BAR is enabled but the
> address it contains might conflict with other devices.  That's the
> problem.
>
> To fix this, I think we need to do "r->flags |= IORESOURCE_UNSET |
> IORESOURCE_DISABLED".

No, that is not right.

Current that r->flags = 0 and reset_resource() cover the bug in
pci_enable_resources() when assign and reassign resource fail.

In pci_enable_resources() if there is one resource with IO BAR or SRIOV BAR
is not assigned and have UNSET, it will prevent device from being enabled.

Most drivers could work without io port.

If you change r->flags = 0 to
r->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED

Those drivers would not work anymore.

I also know one driver that could work with pref mmio, but mmio is not assigned.
--- silicon bug, that it exposes non-needed mmio bar.

BTW I think we may need to clear the BAR in pci_claim_resource().

Maybe Linus or Greg have more idea about this.
Do we need to get more strict? will only enable PCI_COMMAND_MEMORY
unless all mmio BARs get assigned value?

Thanks

Yinghai

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Thu, Jul 10, 2014 at 7:12 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Jul 3, 2014 at 3:11 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Jul 03, 2014 at 12:59:21PM -0700, Yinghai Lu wrote:
>
>>> when we try to size it,  means that bar is not assigned. with r->flags
>>> = 0, means
>>> we will ignore it all the way.
>>
>> With "r->flags = 0", we will not try to assign resources to the BAR,
>> but the hardware BAR still exists and contains some address.  If the
>> device has other memory BARs, pci_enable_resources() will turn on
>> PCI_COMMAND_MEMORY.  Now the "r->flags == 0" BAR is enabled but the
>> address it contains might conflict with other devices.  That's the
>> problem.
>>
>> To fix this, I think we need to do "r->flags |= IORESOURCE_UNSET |
>> IORESOURCE_DISABLED".
>
> No, that is not right.
>
> Current that r->flags = 0 and reset_resource() cover the bug in
> pci_enable_resources() when assign and reassign resource fail.
>
> In pci_enable_resources() if there is one resource with IO BAR or SRIOV BAR
> is not assigned and have UNSET, it will prevent device from being enabled.
>
> Most drivers could work without io port.
>
> If you change r->flags = 0 to
> r->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED
>
> Those drivers would not work anymore.

If a driver doesn't need IO ports, it should use
pci_enable_device_mem().  Then pci_enable_device_flags() will build a
BAR bitmask that does not include the IO BARs, and
pci_enable_resources() will not look at the resources corresponding to
those BARs at all (so it doesn't matter what r->flags is), and it will
not turn on PCI_COMMAND_IO.

> I also know one driver that could work with pref mmio, but mmio is not assigned.
> --- silicon bug, that it exposes non-needed mmio bar.

The driver might work, but turning on PCI_COMMAND_MEMORY when an MMIO
BAR is not assigned is dangerous and could cause bus conflicts.  If we
can't safely turn on PCI_COMMAND_MEMORY and this driver stops working,
well, I think that's tough luck.

I explained why I think it is dangerous in the paragraph "With
'r->flags = 0', ..." above.  This a question of how the hardware
behaves, so if you think I'm wrong, you need to point out something in
the spec that says the hardware works differently than how I think it
does.

> BTW I think we may need to clear the BAR in pci_claim_resource().
>
> Maybe Linus or Greg have more idea about this.
> Do we need to get more strict? will only enable PCI_COMMAND_MEMORY
> unless all mmio BARs get assigned value?

I think we should turn on PCI_COMMAND_MEMORY only when the driver
requests IORESOURCE_MEM and all MMIO BARs are assigned.

Bjorn

Re: [PATCH] pci: Allow very large resource windows

[Yinghai Lu]

On Fri, Jul 11, 2014 at 11:00 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> I explained why I think it is dangerous in the paragraph "With
> 'r->flags = 0', ..." above.  This a question of how the hardware
> behaves, so if you think I'm wrong, you need to point out something in
> the spec that says the hardware works differently than how I think it
> does.

If the BAR have all 0, cpu can not access it as hw will forward access to ram
controller according to routing table.

Yinghai

Re: [PATCH] pci: Allow very large resource windows

[Linus Torvalds]

On Fri, Jul 11, 2014 at 11:09 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> If the BAR have all 0, cpu can not access it as hw will forward access to ram
> controller according to routing table.

On PC's, yes (also likely with many devices - there are definitely
devices out there were setting the BAR to zero disables it).

On other platforms? Not necessarily at all, and some devices have no
way of disabling the BAR (even clearing the IO/MEM bits in the
control/status register does *not* necessarily disable things like
root bridge BARs). I'm pretty sure we had it actually happen (I'd like
to say "Powerpc", but I wouldn't really bet on it)

Anyway, the rule is that "r->flags = 0" means that it doesn't exist.
And that rule makes sense, and is completely unambiguous. Relying on
the BAR _value_ being zero is bogus and wrong.

Probing that finds a valid BAR, but an unassigned one, will set the
UNSET bit. Plus IORESOURCE_MEM or IO should also always be set for a
valid bar. So a value of zero really is completely unambiguous - it
does not exist. It's not unassigned, it really isn't there.

Of course, it's possible that we get this wrong, and initial probing
will obviously start out with a zero value, so there is clearly one
valid situation where we go from "does not exist" to something else,
but in general, "r->flags == 0" really is special.

           Linus

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Fri, Jul 11, 2014 at 12:21 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 11, 2014 at 11:09 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>> If the BAR have all 0, cpu can not access it as hw will forward access to ram
>> controller according to routing table.
>
> On PC's, yes (also likely with many devices - there are definitely
> devices out there were setting the BAR to zero disables it).

Ooh, we might trip over this some day on a platform with host bridge
address translation that maps to PCI bus address 0.  I don't think we
have anything in the allocator that avoids bus address 0 in that case.

Bjorn

Re: [PATCH] pci: Allow very large resource windows

[Yinghai Lu]

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

On Fri, Jul 11, 2014 at 11:40 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Jul 11, 2014 at 12:21 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Fri, Jul 11, 2014 at 11:09 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>
>>> If the BAR have all 0, cpu can not access it as hw will forward access to ram
>>> controller according to routing table.
>>
>> On PC's, yes (also likely with many devices - there are definitely
>> devices out there were setting the BAR to zero disables it).
>
> Ooh, we might trip over this some day on a platform with host bridge
> address translation that maps to PCI bus address 0.  I don't think we
> have anything in the allocator that avoids bus address 0 in that case.

Anyway, I draft RFC one that replace flags = 0 with flags |=
IORESOURCE_UNSET | IORESOURCE_DISABLEd.

Let's see how many drivers will fail.

Yinghai

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

Subject: [RFC PATCH] PCI: don't set flags to 0 when assign resource fail

make flags take IORESOURCE_UNSET | IORESOURCE_DISABLE instead.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/setup-bus.c |   49 ++++++++++++++++++++++++------------------------
 drivers/pci/setup-res.c |   11 ++++++++++
 2 files changed, 36 insertions(+), 24 deletions(-)

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
@@ -136,7 +136,8 @@ static void pdev_sort_resources(struct p
 		if (r->flags & IORESOURCE_PCI_FIXED)
 			continue;
 
-		if (!(r->flags) || r->parent)
+		if (!(r->flags) || r->parent ||
+		    (r->flags & IORESOURCE_DISABLED))
 			continue;
 
 		r_align = pci_resource_alignment(dev, r);
@@ -190,13 +191,6 @@ static void __dev_sort_resources(struct
 	pdev_sort_resources(dev, head);
 }
 
-static inline void reset_resource(struct resource *res)
-{
-	res->start = 0;
-	res->end = 0;
-	res->flags = 0;
-}
-
 /**
  * reassign_resources_sorted() - satisfy any additional resource requests
  *
@@ -242,7 +236,7 @@ static void reassign_resources_sorted(st
 			res->start = add_res->start;
 			res->end = res->start + add_size - 1;
 			if (pci_assign_resource(add_res->dev, idx))
-				reset_resource(res);
+				res->flags |= IORESOURCE_DISABLED;
 		} else {
 			resource_size_t align = add_res->min_align;
 			res->flags |= add_res->flags &
@@ -294,7 +288,7 @@ static void assign_requested_resources_s
 						    0 /* don't care */,
 						    0 /* don't care */);
 			}
-			reset_resource(res);
+			res->flags |= IORESOURCE_DISABLED;
 		}
 	}
 }
@@ -547,7 +541,7 @@ static void pci_setup_bridge_io(struct p
 	/* Set up the top and bottom of the PCI I/O segment for this bus. */
 	res = bus->resource[0];
 	pcibios_resource_to_bus(bridge->bus, &region, res);
-	if (res->flags & IORESOURCE_IO) {
+	if ((res->flags & IORESOURCE_IO) && !(res->flags & IORESOURCE_UNSET)) {
 		pci_read_config_word(bridge, PCI_IO_BASE, &l);
 		io_base_lo = (region.start >> 8) & io_mask;
 		io_limit_lo = (region.end >> 8) & io_mask;
@@ -578,7 +572,8 @@ static void pci_setup_bridge_mmio(struct
 	/* Set up the top and bottom of the PCI Memory segment for this bus. */
 	res = bus->resource[1];
 	pcibios_resource_to_bus(bridge->bus, &region, res);
-	if (res->flags & IORESOURCE_MEM) {
+	if ((res->flags & IORESOURCE_MEM) &&
+	    !(res->flags & IORESOURCE_UNSET)) {
 		l = (region.start >> 16) & 0xfff0;
 		l |= region.end & 0xfff00000;
 		dev_info(&bridge->dev, "  bridge window %pR\n", res);
@@ -604,7 +599,8 @@ static void pci_setup_bridge_mmio_pref(s
 	bu = lu = 0;
 	res = bus->resource[2];
 	pcibios_resource_to_bus(bridge->bus, &region, res);
-	if (res->flags & IORESOURCE_PREFETCH) {
+	if ((res->flags & IORESOURCE_PREFETCH) &&
+	    !(res->flags & IORESOURCE_UNSET)) {
 		l = (region.start >> 16) & 0xfff0;
 		l |= region.end & 0xfff00000;
 		if (res->flags & IORESOURCE_MEM_64) {
@@ -661,6 +657,7 @@ static void pci_bridge_check_ranges(stru
 
 	b_res = &bridge->resource[PCI_BRIDGE_RESOURCES];
 	b_res[1].flags |= IORESOURCE_MEM;
+	b_res[1].flags &= ~IORESOURCE_DISABLED;
 
 	pci_read_config_word(bridge, PCI_IO_BASE, &io);
 	if (!io) {
@@ -668,8 +665,10 @@ static void pci_bridge_check_ranges(stru
 		pci_read_config_word(bridge, PCI_IO_BASE, &io);
 		pci_write_config_word(bridge, PCI_IO_BASE, 0x0);
 	}
-	if (io)
+	if (io) {
 		b_res[0].flags |= IORESOURCE_IO;
+		b_res[0].flags &= ~IORESOURCE_DISABLED;
+	}
 
 	/*  DECchip 21050 pass 2 errata: the bridge may miss an address
 	    disconnect boundary by one PCI data phase.
@@ -686,6 +685,7 @@ static void pci_bridge_check_ranges(stru
 	}
 	if (pmem) {
 		b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+		b_res[2].flags &= ~IORESOURCE_DISABLED;
 		if ((pmem & PCI_PREF_RANGE_TYPE_MASK) ==
 		    PCI_PREF_RANGE_TYPE_64) {
 			b_res[2].flags |= IORESOURCE_MEM_64;
@@ -830,8 +830,10 @@ static void pbus_size_io(struct pci_bus
 			struct resource *r = &dev->resource[i];
 			unsigned long r_size;
 
-			if (r->parent || !(r->flags & IORESOURCE_IO))
+			if (r->parent || !(r->flags & IORESOURCE_IO) ||
+			    (r->flags & IORESOURCE_DISABLED))
 				continue;
+
 			r_size = resource_size(r);
 
 			if (r_size < 0x400)
@@ -860,7 +862,7 @@ static void pbus_size_io(struct pci_bus
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window %pR to %pR (unused)\n",
 				 b_res, &bus->busn_res);
-		b_res->flags = 0;
+		b_res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 		return;
 	}
 
@@ -947,8 +949,10 @@ static int pbus_size_mem(struct pci_bus
 
 			if (r->parent || ((r->flags & mask) != type &&
 					  (r->flags & mask) != type2 &&
-					  (r->flags & mask) != type3))
+					  (r->flags & mask) != type3) ||
+			    (r->flags & IORESOURCE_DISABLED))
 				continue;
+
 			r_size = resource_size(r);
 #ifdef CONFIG_PCI_IOV
 			/* put SRIOV requested res to the optional list */
@@ -973,7 +977,8 @@ static int pbus_size_mem(struct pci_bus
 			if (order >= ARRAY_SIZE(aligns)) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR (bad alignment %#llx)\n",
 					 i, r, (unsigned long long) align);
-				r->flags = 0;
+				r->flags |= IORESOURCE_UNSET |
+					    IORESOURCE_DISABLED;
 				continue;
 			}
 			size += r_size;
@@ -1001,7 +1006,7 @@ static int pbus_size_mem(struct pci_bus
 		if (b_res->start || b_res->end)
 			dev_info(&bus->self->dev, "disabling bridge window %pR to %pR (unused)\n",
 				 b_res, &bus->busn_res);
-		b_res->flags = 0;
+		b_res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 		return 0;
 	}
 	b_res->start = min_align;
@@ -1367,7 +1372,7 @@ static void pci_bridge_release_resources
 		/* keep the old size */
 		r->end = resource_size(r) - 1;
 		r->start = 0;
-		r->flags = 0;
+		r->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 
 		/* avoiding touch the one without PREF */
 		if (type & IORESOURCE_PREFETCH)
@@ -1622,8 +1627,6 @@ again:
 		res->start = fail_res->start;
 		res->end = fail_res->end;
 		res->flags = fail_res->flags;
-		if (fail_res->dev->subordinate)
-			res->flags = 0;
 	}
 	free_list(&fail_head);
 
@@ -1688,8 +1691,6 @@ again:
 		res->start = fail_res->start;
 		res->end = fail_res->end;
 		res->flags = fail_res->flags;
-		if (fail_res->dev->subordinate)
-			res->flags = 0;
 	}
 	free_list(&fail_head);
 
Index: linux-2.6/drivers/pci/setup-res.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-res.c
+++ linux-2.6/drivers/pci/setup-res.c
@@ -362,6 +362,17 @@ int pci_enable_resources(struct pci_dev
 			continue;
 
 		if (r->flags & IORESOURCE_UNSET) {
+			/* bridge BAR could be disabled one by one */
+			if (dev->subordinate && i >= PCI_BRIDGE_RESOURCES &&
+						i <= PCI_BRIDGE_RESOURCE_END)
+				continue;
+
+#ifdef CONFIG_PCI_IOV
+			/* SRIOV ? */
+			if (i >= PCI_IOV_RESOURCES && i <= PCI_IOV_RESOURCE_END)
+				continue;
+#endif
+
 			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
 				i, r);
 			return -EINVAL;

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Fri, Jul 11, 2014 at 06:22:08PM -0700, Yinghai Lu wrote:
> On Fri, Jul 11, 2014 at 11:40 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Fri, Jul 11, 2014 at 12:21 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >> On Fri, Jul 11, 2014 at 11:09 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>>
> >>> If the BAR have all 0, cpu can not access it as hw will forward access to ram
> >>> controller according to routing table.
> >>
> >> On PC's, yes (also likely with many devices - there are definitely
> >> devices out there were setting the BAR to zero disables it).
> >
> > Ooh, we might trip over this some day on a platform with host bridge
> > address translation that maps to PCI bus address 0.  I don't think we
> > have anything in the allocator that avoids bus address 0 in that case.
> 
> Anyway, I draft RFC one that replace flags = 0 with flags |=
> IORESOURCE_UNSET | IORESOURCE_DISABLEd.

I think we should do something like this patch, but we need to clarify how
we want to use the IORESOURCE flags.

Here's what I think these IORESOURCE flags mean:

  IORESOURCE_MEM        Hardware decodes memory accesses (when enabled)

  IORESOURCE_UNSET      We have not allocated space for this decoder,
                        although the hardware still contains some address

  IORESOURCE_DISABLED   This hardware decoder is disabled, e.g., a bridge
                        window with LIMIT < BASE, or a ROM BAR with its
                        enable bit cleared

For PCI, this would imply:

  - The resource for a standard BAR should never have IORESOURCE_DISABLED
    because it can't be individually disabled.

  - We can turn on PCI_COMMAND_MEMORY only if every IORESOURCE_MEM resource
    has either (IORESOURCE_DISABLED set) or (IORESOURCE_UNSET cleared).

> Let's see how many drivers will fail.
> 
> Yinghai

> Subject: [RFC PATCH] PCI: don't set flags to 0 when assign resource fail
> 
> make flags take IORESOURCE_UNSET | IORESOURCE_DISABLE instead.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/setup-bus.c |   49 ++++++++++++++++++++++++------------------------
>  drivers/pci/setup-res.c |   11 ++++++++++
>  2 files changed, 36 insertions(+), 24 deletions(-)
> 
> 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
> @@ -136,7 +136,8 @@ static void pdev_sort_resources(struct p
>  		if (r->flags & IORESOURCE_PCI_FIXED)
>  			continue;
>  
> -		if (!(r->flags) || r->parent)
> +		if (!(r->flags) || r->parent ||
> +		    (r->flags & IORESOURCE_DISABLED))
>  			continue;
>  
>  		r_align = pci_resource_alignment(dev, r);
> @@ -190,13 +191,6 @@ static void __dev_sort_resources(struct
>  	pdev_sort_resources(dev, head);
>  }
>  
> -static inline void reset_resource(struct resource *res)
> -{
> -	res->start = 0;
> -	res->end = 0;
> -	res->flags = 0;
> -}
> -
>  /**
>   * reassign_resources_sorted() - satisfy any additional resource requests
>   *
> @@ -242,7 +236,7 @@ static void reassign_resources_sorted(st
>  			res->start = add_res->start;
>  			res->end = res->start + add_size - 1;
>  			if (pci_assign_resource(add_res->dev, idx))
> -				reset_resource(res);
> +				res->flags |= IORESOURCE_DISABLED;
>  		} else {
>  			resource_size_t align = add_res->min_align;
>  			res->flags |= add_res->flags &
> @@ -294,7 +288,7 @@ static void assign_requested_resources_s
>  						    0 /* don't care */,
>  						    0 /* don't care */);
>  			}
> -			reset_resource(res);
> +			res->flags |= IORESOURCE_DISABLED;
>  		}
>  	}
>  }
> @@ -547,7 +541,7 @@ static void pci_setup_bridge_io(struct p
>  	/* Set up the top and bottom of the PCI I/O segment for this bus. */
>  	res = bus->resource[0];
>  	pcibios_resource_to_bus(bridge->bus, &region, res);
> -	if (res->flags & IORESOURCE_IO) {
> +	if ((res->flags & IORESOURCE_IO) && !(res->flags & IORESOURCE_UNSET)) {
>  		pci_read_config_word(bridge, PCI_IO_BASE, &l);
>  		io_base_lo = (region.start >> 8) & io_mask;
>  		io_limit_lo = (region.end >> 8) & io_mask;
> @@ -578,7 +572,8 @@ static void pci_setup_bridge_mmio(struct
>  	/* Set up the top and bottom of the PCI Memory segment for this bus. */
>  	res = bus->resource[1];
>  	pcibios_resource_to_bus(bridge->bus, &region, res);
> -	if (res->flags & IORESOURCE_MEM) {
> +	if ((res->flags & IORESOURCE_MEM) &&
> +	    !(res->flags & IORESOURCE_UNSET)) {
>  		l = (region.start >> 16) & 0xfff0;
>  		l |= region.end & 0xfff00000;
>  		dev_info(&bridge->dev, "  bridge window %pR\n", res);
> @@ -604,7 +599,8 @@ static void pci_setup_bridge_mmio_pref(s
>  	bu = lu = 0;
>  	res = bus->resource[2];
>  	pcibios_resource_to_bus(bridge->bus, &region, res);
> -	if (res->flags & IORESOURCE_PREFETCH) {
> +	if ((res->flags & IORESOURCE_PREFETCH) &&
> +	    !(res->flags & IORESOURCE_UNSET)) {
>  		l = (region.start >> 16) & 0xfff0;
>  		l |= region.end & 0xfff00000;
>  		if (res->flags & IORESOURCE_MEM_64) {
> @@ -661,6 +657,7 @@ static void pci_bridge_check_ranges(stru
>  
>  	b_res = &bridge->resource[PCI_BRIDGE_RESOURCES];
>  	b_res[1].flags |= IORESOURCE_MEM;
> +	b_res[1].flags &= ~IORESOURCE_DISABLED;
>  
>  	pci_read_config_word(bridge, PCI_IO_BASE, &io);
>  	if (!io) {
> @@ -668,8 +665,10 @@ static void pci_bridge_check_ranges(stru
>  		pci_read_config_word(bridge, PCI_IO_BASE, &io);
>  		pci_write_config_word(bridge, PCI_IO_BASE, 0x0);
>  	}
> -	if (io)
> +	if (io) {
>  		b_res[0].flags |= IORESOURCE_IO;
> +		b_res[0].flags &= ~IORESOURCE_DISABLED;
> +	}
>  
>  	/*  DECchip 21050 pass 2 errata: the bridge may miss an address
>  	    disconnect boundary by one PCI data phase.
> @@ -686,6 +685,7 @@ static void pci_bridge_check_ranges(stru
>  	}
>  	if (pmem) {
>  		b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
> +		b_res[2].flags &= ~IORESOURCE_DISABLED;
>  		if ((pmem & PCI_PREF_RANGE_TYPE_MASK) ==
>  		    PCI_PREF_RANGE_TYPE_64) {
>  			b_res[2].flags |= IORESOURCE_MEM_64;
> @@ -830,8 +830,10 @@ static void pbus_size_io(struct pci_bus
>  			struct resource *r = &dev->resource[i];
>  			unsigned long r_size;
>  
> -			if (r->parent || !(r->flags & IORESOURCE_IO))
> +			if (r->parent || !(r->flags & IORESOURCE_IO) ||
> +			    (r->flags & IORESOURCE_DISABLED))
>  				continue;
> +
>  			r_size = resource_size(r);
>  
>  			if (r_size < 0x400)
> @@ -860,7 +862,7 @@ static void pbus_size_io(struct pci_bus
>  		if (b_res->start || b_res->end)
>  			dev_info(&bus->self->dev, "disabling bridge window %pR to %pR (unused)\n",
>  				 b_res, &bus->busn_res);
> -		b_res->flags = 0;
> +		b_res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
>  		return;
>  	}
>  
> @@ -947,8 +949,10 @@ static int pbus_size_mem(struct pci_bus
>  
>  			if (r->parent || ((r->flags & mask) != type &&
>  					  (r->flags & mask) != type2 &&
> -					  (r->flags & mask) != type3))
> +					  (r->flags & mask) != type3) ||
> +			    (r->flags & IORESOURCE_DISABLED))
>  				continue;
> +
>  			r_size = resource_size(r);
>  #ifdef CONFIG_PCI_IOV
>  			/* put SRIOV requested res to the optional list */
> @@ -973,7 +977,8 @@ static int pbus_size_mem(struct pci_bus
>  			if (order >= ARRAY_SIZE(aligns)) {
>  				dev_warn(&dev->dev, "disabling BAR %d: %pR (bad alignment %#llx)\n",
>  					 i, r, (unsigned long long) align);
> -				r->flags = 0;
> +				r->flags |= IORESOURCE_UNSET |
> +					    IORESOURCE_DISABLED;
>  				continue;
>  			}
>  			size += r_size;
> @@ -1001,7 +1006,7 @@ static int pbus_size_mem(struct pci_bus
>  		if (b_res->start || b_res->end)
>  			dev_info(&bus->self->dev, "disabling bridge window %pR to %pR (unused)\n",
>  				 b_res, &bus->busn_res);
> -		b_res->flags = 0;
> +		b_res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
>  		return 0;
>  	}
>  	b_res->start = min_align;
> @@ -1367,7 +1372,7 @@ static void pci_bridge_release_resources
>  		/* keep the old size */
>  		r->end = resource_size(r) - 1;
>  		r->start = 0;
> -		r->flags = 0;
> +		r->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
>  
>  		/* avoiding touch the one without PREF */
>  		if (type & IORESOURCE_PREFETCH)
> @@ -1622,8 +1627,6 @@ again:
>  		res->start = fail_res->start;
>  		res->end = fail_res->end;
>  		res->flags = fail_res->flags;
> -		if (fail_res->dev->subordinate)
> -			res->flags = 0;
>  	}
>  	free_list(&fail_head);
>  
> @@ -1688,8 +1691,6 @@ again:
>  		res->start = fail_res->start;
>  		res->end = fail_res->end;
>  		res->flags = fail_res->flags;
> -		if (fail_res->dev->subordinate)
> -			res->flags = 0;
>  	}
>  	free_list(&fail_head);
>  
> Index: linux-2.6/drivers/pci/setup-res.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-res.c
> +++ linux-2.6/drivers/pci/setup-res.c
> @@ -362,6 +362,17 @@ int pci_enable_resources(struct pci_dev
>  			continue;
>  
>  		if (r->flags & IORESOURCE_UNSET) {
> +			/* bridge BAR could be disabled one by one */
> +			if (dev->subordinate && i >= PCI_BRIDGE_RESOURCES &&
> +						i <= PCI_BRIDGE_RESOURCE_END)
> +				continue;
> +
> +#ifdef CONFIG_PCI_IOV
> +			/* SRIOV ? */
> +			if (i >= PCI_IOV_RESOURCES && i <= PCI_IOV_RESOURCE_END)
> +				continue;
> +#endif
> +
>  			dev_err(&dev->dev, "can't enable device: BAR %d %pR not assigned\n",
>  				i, r);
>  			return -EINVAL;

[PATCH] pci: Allow very large resource windows

[Alan]

From: Alan <alan@linux.intel.com>

This is needed for some of the Xeon Phi type systems.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---
 drivers/pci/setup-bus.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index d219d44..4184112 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -921,7 +921,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 {
 	struct pci_dev *dev;
 	resource_size_t min_align, align, size, size0, size1;
-	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
+	resource_size_t aligns[14];	/* Alignments from 1Mb to 8Gb */
 	int order, max_order;
 	struct resource *b_res = find_free_bus_resource(bus, type);
 	unsigned int mem64_mask = 0;
@@ -960,7 +960,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			/* For bridges size != alignment */
 			align = pci_resource_alignment(dev, r);
 			order = __ffs(align) - 20;
-			if (order > 11) {
+			if (order > 13) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
 					 (unsigned long long) align);

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Mon, May 19, 2014 at 02:03:14PM +0100, Alan wrote:
> From: Alan <alan@linux.intel.com>
> 
> This is needed for some of the Xeon Phi type systems.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>

I applied this to my pci/resource branch for v3.16.  Nikhil
posted essentially the same patch a couple years ago, so I added
his signed-off-by and adopted his use of ARRAY_SIZE() to connect the
"order > 13" test with the aligns[] declaration.

Bjorn

> ---
>  drivers/pci/setup-bus.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index d219d44..4184112 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -921,7 +921,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  {
>  	struct pci_dev *dev;
>  	resource_size_t min_align, align, size, size0, size1;
> -	resource_size_t aligns[12];	/* Alignments from 1Mb to 2Gb */
> +	resource_size_t aligns[14];	/* Alignments from 1Mb to 8Gb */
>  	int order, max_order;
>  	struct resource *b_res = find_free_bus_resource(bus, type);
>  	unsigned int mem64_mask = 0;
> @@ -960,7 +960,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  			/* For bridges size != alignment */
>  			align = pci_resource_alignment(dev, r);
>  			order = __ffs(align) - 20;
> -			if (order > 11) {
> +			if (order > 13) {
>  				dev_warn(&dev->dev, "disabling BAR %d: %pR "
>  					 "(bad alignment %#llx)\n", i, r,
>  					 (unsigned long long) align);
> 

Re: [PATCH] pci: Allow very large resource windows

[Kevin Hilman]

Hi Bjorn,

Bjorn Helgaas <bhelgaas@google.com> writes:

> On Mon, May 19, 2014 at 02:03:14PM +0100, Alan wrote:
>> From: Alan <alan@linux.intel.com>
>> 
>> This is needed for some of the Xeon Phi type systems.
>> 
>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>
> I applied this to my pci/resource branch for v3.16.  Nikhil
> posted essentially the same patch a couple years ago, so I added
> his signed-off-by and adopted his use of ARRAY_SIZE() to connect the
> "order > 13" test with the aligns[] declaration.

This patch (with the ARRAY_SIZE changes) has hit next-20140253 (in the
form of commit 272c5a886c57) and according to my bisect, is the cause of
a new boot failure on a 32-bit ARM platform (Marvell Armada 370,
Mirabox, boot log excerpt down below[2].)

While debugging, I found that Alan's original patch (without the
ARRAY_SIZE change) booted just fine so I started looking closely at the
ARRAY_SIZE() change.  After some high-tech printk debugging, I noticed
that order was negative when doing the compare, and found that this hack
got things booting again:

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 47482b27fa12..792db3477fd5 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -966,7 +966,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			/* For bridges size != alignment */
 			align = pci_resource_alignment(dev, r);
 			order = __ffs(align) - 20;
-			if (order >= ARRAY_SIZE(aligns)) {
+			if (order >= (int)ARRAY_SIZE(aligns)) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
 					 (unsigned long long) align);

which led me to realize that the ARRAY_SIZE() change converted the >=
into an unsigned compare where before it was a signed one, and as an
unsigned compare, it was always true, resulting in those BARs being
disabled.

Below is a patch[1] which fixes the problem for me, and attempts a more
detailed description of the problem in the changelog.

Kevin

[1]
>From 386d39a22e586fc1b060ad18c79247a50f2c0f8c Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@linaro.org>
Date: Fri, 23 May 2014 10:24:40 -0700
Subject: [PATCH] PCI: pbus_mem_size(): don't disable small BAR windows

Since commit 272c5a886c57 (PCI: Support BAR sizes up to 8GB), small
BAR windows get disabled.

For small BAR sizes, order (as computed by __ffs(align) - 20) may be
negative.  If order is negative, when checking if it's >=
ARRAY_SIZE(aligns) will always be true (since the result of
ARRAY_SIZE() is unsigned) and so those BARs will be disabled.

It doesn't make sense for order to be negative, and the code already
converts it to non-negative later on, so to fix this bug, ensure that
order is non-negative before comapring with ARRAY_SIZE().

NOTE: Before commit 272c5a886c57, this worked just fine because order
      wasn't compared with ARRAY_SIZE() but with a hard-coded int, so
      the resulting signed compare worked fine.

Signed-off-by: Kevin Hilman <khilman@linaro.org>
---
 drivers/pci/setup-bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 47482b27fa12..a4152566f531 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -966,6 +966,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 			/* For bridges size != alignment */
 			align = pci_resource_alignment(dev, r);
 			order = __ffs(align) - 20;
+			if (order < 0)
+				order = 0;
 			if (order >= ARRAY_SIZE(aligns)) {
 				dev_warn(&dev->dev, "disabling BAR %d: %pR "
 					 "(bad alignment %#llx)\n", i, r,
@@ -974,8 +976,6 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 				continue;
 			}
 			size += r_size;
-			if (order < 0)
-				order = 0;
 			/* Exclude ranges with size > align from
 			   calculation of the alignment. */
 			if (r_size == align)
-- 
1.9.2



[2]
[...]
mvebu-pcie pcie-controller.3: PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io  0x1000-0xfffff]
pci_bus 0000:00: root bus resource [mem 0xf8000000-0xffdfffff]
pci_bus 0000:00: root bus resource [bus 00-ff]
pci 0000:00:01.0: [11ab:6710] type 01 class 0x060400
pci 0000:00:02.0: [11ab:6710] type 01 class 0x060400
PCI: bus0: Fast back to back transfers disabled
pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring
pci 0000:00:02.0: bridge configuration invalid ([bus 00-00]), reconfiguring
PCI: bus1: Fast back to back transfers enabled
pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
pci 0000:02:00.0: [1b73:1009] type 00 class 0x0c0330
pci 0000:02:00.0: reg 0x10: [mem 0x42000000-0x4200ffff 64bit]
pci 0000:02:00.0: reg 0x18: [mem 0x42010000-0x42010fff 64bit]
pci 0000:02:00.0: reg 0x20: [mem 0x42011000-0x42011fff 64bit]
pci 0000:02:00.0: supports D1
pci 0000:02:00.0: PME# supported from D0 D1 D3hot
PCI: bus2: Fast back to back transfers disabled
pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
pci 0000:02:00.0: disabling BAR 0: [mem 0x42000000-0x4200ffff 64bit] (bad alignment 0x10000)
pci 0000:02:00.0: disabling BAR 2: [mem 0x42010000-0x42010fff 64bit] (bad alignment 0x1000)
pci 0000:02:00.0: disabling BAR 4: [mem 0x42011000-0x42011fff 64bit] (bad alignment 0x1000)
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:02.0: PCI bridge to [bus 02]
Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0080010
Internal error: : 1008 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.15.0-rc6-next-20140523-08540-gefd7e9bf5d99 #45
task: ee81ab80 ti: ee83a000 task.ti: ee83a000
PC is at quirk_usb_early_handoff+0x348/0x7dc
LR is at ioremap_page_range+0xfc/0x1b4
pc : [<c0297f54>]    lr : [<c01886d8>]    psr: a0000153
sp : ee83bd48  ip : c0019a20  fp : c0620964
r10: 00000100  r9 : ee83bdd0  r8 : 00010000
r7 : f0080000  r6 : c0607e20  r5 : eeb7a800  r4 : eeb7a800
r3 : 00000146  r2 : 00000000  r1 : f0090000  r0 : f0080000
Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 00004019  DAC: 00000015
Process swapper/0 (pid: 1, stack limit = 0xee83a248)
Stack: (0xee83bd48 to 0xee83c000)
bd40:                   c04428d4 ee83bd80 c062ae30 c053ab4c eeb7a800 c0607e20
bd60: c053ab4c 0000ffff ee83bdd0 00000100 c0620964 c01bc3a0 00000000 c01eda18
bd80: eeb7a800 eeb78814 eeb78800 00000001 00010000 c01ab390 eeb7a800 c01ab534
bda0: eeb7ac00 eeb78e14 eeb78e00 c01ab57c eeb77f80 eeb78e00 ee83be28 c0012d98
bdc0: f0074000 c0007fc8 feef0000 00000000 eeb77f80 eeb77f80 ee8e3410 c0303450
bde0: c01be984 c01be9cc ee83be24 00010100 00000002 00000021 00000000 c01bfb68
be00: eea1a0b8 000007cb 00000065 00000000 eeff2cb4 ee8e3410 ee83be24 10624dd3
be20: 00000000 eea18210 c05e69c8 00000001 ee83be24 c01be9cc c01be984 00000000
be40: 00000000 00000000 c0303450 c01bf794 c01be818 00000000 00000000 ee8e3410
be60: c05e6984 00000000 c05e6984 00000000 00000000 ee83a000 00000000 c01f05b0
be80: c01f0598 ee8e3410 c062c548 c01ef280 ee8e3410 c05e6984 ee8e3444 00000000
bea0: c059f558 c01ef434 00000000 c05e6984 c01ef3a8 c01eda80 ee80965c ee8d47b4
bec0: c05e6984 eeb1a000 c05edcc8 c01eea48 c0505490 c05c6de8 c05e6984 c05e6984
bee0: c05d5198 eeb70f40 c0607e00 c01efa74 00000000 c05d5198 c05d5198 c0008984
bf00: c0610304 ee82bc00 c042e950 00000010 c0607e00 c0563398 00000000 c00f5a90
bf20: 00000000 c05d84a4 60000153 00000001 ef7fce1b c04403d0 000000a2 c00366c0
bf40: c0562978 00000006 ef7fce26 00000006 c05d8474 c05c6de8 00000006 c05b5f30
bf60: c0607e00 000000a2 c05b5f3c c058b5a4 00000000 c058bd14 00000006 00000006
bf80: c058b5a4 00000000 00000000 c0422ffc 00000000 00000000 00000000 00000000
bfa0: 00000000 c0423004 00000000 c000e5b8 00000000 00000000 00000000 00000000
bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<c0297f54>] (quirk_usb_early_handoff) from [<c01bc3a0>] (pci_fixup_device+0xd4/0x184)
[<c01bc3a0>] (pci_fixup_device) from [<c01ab390>] (pci_bus_add_device+0x14/0x64)
[<c01ab390>] (pci_bus_add_device) from [<c01ab534>] (pci_bus_add_devices+0x3c/0x9c)
[<c01ab534>] (pci_bus_add_devices) from [<c01ab57c>] (pci_bus_add_devices+0x84/0x9c)
[<c01ab57c>] (pci_bus_add_devices) from [<c0012d98>] (pci_common_init_dev+0x184/0x2dc)
[<c0012d98>] (pci_common_init_dev) from [<c01bfb68>] (mvebu_pcie_probe+0x3ac/0x604)
[<c01bfb68>] (mvebu_pcie_probe) from [<c01f05b0>] (platform_drv_probe+0x18/0x48)
[<c01f05b0>] (platform_drv_probe) from [<c01ef280>] (driver_probe_device+0x100/0x228)
[<c01ef280>] (driver_probe_device) from [<c01ef434>] (__driver_attach+0x8c/0x90)
[<c01ef434>] (__driver_attach) from [<c01eda80>] (bus_for_each_dev+0x54/0x88)
[<c01eda80>] (bus_for_each_dev) from [<c01eea48>] (bus_add_driver+0xd4/0x1d0)
[<c01eea48>] (bus_add_driver) from [<c01efa74>] (driver_register+0x78/0xf4)
[<c01efa74>] (driver_register) from [<c0008984>] (do_one_initcall+0x80/0x1b8)
[<c0008984>] (do_one_initcall) from [<c058bd14>] (kernel_init_freeable+0xfc/0x1c8)
[<c058bd14>] (kernel_init_freeable) from [<c0423004>] (kernel_init+0x8/0xec)
[<c0423004>] (kernel_init) from [<c000e5b8>] (ret_from_fork+0x14/0x3c)
Code: e3a02000 ebf5fd27 e2507000 0affff59 (e5973010) 
---[ end trace 20c08b6c6486c9b6 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

Re: [PATCH] pci: Allow very large resource windows

[Bjorn Helgaas]

On Fri, May 23, 2014 at 11:51 AM, Kevin Hilman <khilman@linaro.org> wrote:
> Hi Bjorn,
>
> Bjorn Helgaas <bhelgaas@google.com> writes:
>
>> On Mon, May 19, 2014 at 02:03:14PM +0100, Alan wrote:
>>> From: Alan <alan@linux.intel.com>
>>>
>>> This is needed for some of the Xeon Phi type systems.
>>>
>>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>>
>> I applied this to my pci/resource branch for v3.16.  Nikhil
>> posted essentially the same patch a couple years ago, so I added
>> his signed-off-by and adopted his use of ARRAY_SIZE() to connect the
>> "order > 13" test with the aligns[] declaration.
>
> This patch (with the ARRAY_SIZE changes) has hit next-20140253 (in the
> form of commit 272c5a886c57) and according to my bisect, is the cause of
> a new boot failure on a 32-bit ARM platform (Marvell Armada 370,
> Mirabox, boot log excerpt down below[2].)
>
> While debugging, I found that Alan's original patch (without the
> ARRAY_SIZE change) booted just fine so I started looking closely at the
> ARRAY_SIZE() change.  After some high-tech printk debugging, I noticed
> that order was negative when doing the compare, and found that this hack
> got things booting again:
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 47482b27fa12..792db3477fd5 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -966,7 +966,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>                         /* For bridges size != alignment */
>                         align = pci_resource_alignment(dev, r);
>                         order = __ffs(align) - 20;
> -                       if (order >= ARRAY_SIZE(aligns)) {
> +                       if (order >= (int)ARRAY_SIZE(aligns)) {
>                                 dev_warn(&dev->dev, "disabling BAR %d: %pR "
>                                          "(bad alignment %#llx)\n", i, r,
>                                          (unsigned long long) align);
>
> which led me to realize that the ARRAY_SIZE() change converted the >=
> into an unsigned compare where before it was a signed one, and as an
> unsigned compare, it was always true, resulting in those BARs being
> disabled.
>
> Below is a patch[1] which fixes the problem for me, and attempts a more
> detailed description of the problem in the changelog.
>
> Kevin
>
> [1]
> From 386d39a22e586fc1b060ad18c79247a50f2c0f8c Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@linaro.org>
> Date: Fri, 23 May 2014 10:24:40 -0700
> Subject: [PATCH] PCI: pbus_mem_size(): don't disable small BAR windows
>
> Since commit 272c5a886c57 (PCI: Support BAR sizes up to 8GB), small
> BAR windows get disabled.
>
> For small BAR sizes, order (as computed by __ffs(align) - 20) may be
> negative.  If order is negative, when checking if it's >=
> ARRAY_SIZE(aligns) will always be true (since the result of
> ARRAY_SIZE() is unsigned) and so those BARs will be disabled.
>
> It doesn't make sense for order to be negative, and the code already
> converts it to non-negative later on, so to fix this bug, ensure that
> order is non-negative before comapring with ARRAY_SIZE().
>
> NOTE: Before commit 272c5a886c57, this worked just fine because order
>       wasn't compared with ARRAY_SIZE() but with a hard-coded int, so
>       the resulting signed compare worked fine.
>
> Signed-off-by: Kevin Hilman <khilman@linaro.org>

Ouch, sorry about that.  Thanks for your detailed analysis and fix.  I
folded it into the patch and added a comment in the code.

Bjorn

> ---
>  drivers/pci/setup-bus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 47482b27fa12..a4152566f531 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -966,6 +966,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>                         /* For bridges size != alignment */
>                         align = pci_resource_alignment(dev, r);
>                         order = __ffs(align) - 20;
> +                       if (order < 0)
> +                               order = 0;
>                         if (order >= ARRAY_SIZE(aligns)) {
>                                 dev_warn(&dev->dev, "disabling BAR %d: %pR "
>                                          "(bad alignment %#llx)\n", i, r,
> @@ -974,8 +976,6 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>                                 continue;
>                         }
>                         size += r_size;
> -                       if (order < 0)
> -                               order = 0;
>                         /* Exclude ranges with size > align from
>                            calculation of the alignment. */
>                         if (r_size == align)
> --
> 1.9.2
>
>
>
> [2]
> [...]
> mvebu-pcie pcie-controller.3: PCI host bridge to bus 0000:00
> pci_bus 0000:00: root bus resource [io  0x1000-0xfffff]
> pci_bus 0000:00: root bus resource [mem 0xf8000000-0xffdfffff]
> pci_bus 0000:00: root bus resource [bus 00-ff]
> pci 0000:00:01.0: [11ab:6710] type 01 class 0x060400
> pci 0000:00:02.0: [11ab:6710] type 01 class 0x060400
> PCI: bus0: Fast back to back transfers disabled
> pci 0000:00:01.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> pci 0000:00:02.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> PCI: bus1: Fast back to back transfers enabled
> pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
> pci 0000:02:00.0: [1b73:1009] type 00 class 0x0c0330
> pci 0000:02:00.0: reg 0x10: [mem 0x42000000-0x4200ffff 64bit]
> pci 0000:02:00.0: reg 0x18: [mem 0x42010000-0x42010fff 64bit]
> pci 0000:02:00.0: reg 0x20: [mem 0x42011000-0x42011fff 64bit]
> pci 0000:02:00.0: supports D1
> pci 0000:02:00.0: PME# supported from D0 D1 D3hot
> PCI: bus2: Fast back to back transfers disabled
> pci_bus 0000:02: busn_res: [bus 02-ff] end is updated to 02
> pci 0000:02:00.0: disabling BAR 0: [mem 0x42000000-0x4200ffff 64bit] (bad alignment 0x10000)
> pci 0000:02:00.0: disabling BAR 2: [mem 0x42010000-0x42010fff 64bit] (bad alignment 0x1000)
> pci 0000:02:00.0: disabling BAR 4: [mem 0x42011000-0x42011fff 64bit] (bad alignment 0x1000)
> pci 0000:00:01.0: PCI bridge to [bus 01]
> pci 0000:00:02.0: PCI bridge to [bus 02]
> Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0080010
> Internal error: : 1008 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.15.0-rc6-next-20140523-08540-gefd7e9bf5d99 #45
> task: ee81ab80 ti: ee83a000 task.ti: ee83a000
> PC is at quirk_usb_early_handoff+0x348/0x7dc
> LR is at ioremap_page_range+0xfc/0x1b4
> pc : [<c0297f54>]    lr : [<c01886d8>]    psr: a0000153
> sp : ee83bd48  ip : c0019a20  fp : c0620964
> r10: 00000100  r9 : ee83bdd0  r8 : 00010000
> r7 : f0080000  r6 : c0607e20  r5 : eeb7a800  r4 : eeb7a800
> r3 : 00000146  r2 : 00000000  r1 : f0090000  r0 : f0080000
> Flags: NzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c5387d  Table: 00004019  DAC: 00000015
> Process swapper/0 (pid: 1, stack limit = 0xee83a248)
> Stack: (0xee83bd48 to 0xee83c000)
> bd40:                   c04428d4 ee83bd80 c062ae30 c053ab4c eeb7a800 c0607e20
> bd60: c053ab4c 0000ffff ee83bdd0 00000100 c0620964 c01bc3a0 00000000 c01eda18
> bd80: eeb7a800 eeb78814 eeb78800 00000001 00010000 c01ab390 eeb7a800 c01ab534
> bda0: eeb7ac00 eeb78e14 eeb78e00 c01ab57c eeb77f80 eeb78e00 ee83be28 c0012d98
> bdc0: f0074000 c0007fc8 feef0000 00000000 eeb77f80 eeb77f80 ee8e3410 c0303450
> bde0: c01be984 c01be9cc ee83be24 00010100 00000002 00000021 00000000 c01bfb68
> be00: eea1a0b8 000007cb 00000065 00000000 eeff2cb4 ee8e3410 ee83be24 10624dd3
> be20: 00000000 eea18210 c05e69c8 00000001 ee83be24 c01be9cc c01be984 00000000
> be40: 00000000 00000000 c0303450 c01bf794 c01be818 00000000 00000000 ee8e3410
> be60: c05e6984 00000000 c05e6984 00000000 00000000 ee83a000 00000000 c01f05b0
> be80: c01f0598 ee8e3410 c062c548 c01ef280 ee8e3410 c05e6984 ee8e3444 00000000
> bea0: c059f558 c01ef434 00000000 c05e6984 c01ef3a8 c01eda80 ee80965c ee8d47b4
> bec0: c05e6984 eeb1a000 c05edcc8 c01eea48 c0505490 c05c6de8 c05e6984 c05e6984
> bee0: c05d5198 eeb70f40 c0607e00 c01efa74 00000000 c05d5198 c05d5198 c0008984
> bf00: c0610304 ee82bc00 c042e950 00000010 c0607e00 c0563398 00000000 c00f5a90
> bf20: 00000000 c05d84a4 60000153 00000001 ef7fce1b c04403d0 000000a2 c00366c0
> bf40: c0562978 00000006 ef7fce26 00000006 c05d8474 c05c6de8 00000006 c05b5f30
> bf60: c0607e00 000000a2 c05b5f3c c058b5a4 00000000 c058bd14 00000006 00000006
> bf80: c058b5a4 00000000 00000000 c0422ffc 00000000 00000000 00000000 00000000
> bfa0: 00000000 c0423004 00000000 c000e5b8 00000000 00000000 00000000 00000000
> bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [<c0297f54>] (quirk_usb_early_handoff) from [<c01bc3a0>] (pci_fixup_device+0xd4/0x184)
> [<c01bc3a0>] (pci_fixup_device) from [<c01ab390>] (pci_bus_add_device+0x14/0x64)
> [<c01ab390>] (pci_bus_add_device) from [<c01ab534>] (pci_bus_add_devices+0x3c/0x9c)
> [<c01ab534>] (pci_bus_add_devices) from [<c01ab57c>] (pci_bus_add_devices+0x84/0x9c)
> [<c01ab57c>] (pci_bus_add_devices) from [<c0012d98>] (pci_common_init_dev+0x184/0x2dc)
> [<c0012d98>] (pci_common_init_dev) from [<c01bfb68>] (mvebu_pcie_probe+0x3ac/0x604)
> [<c01bfb68>] (mvebu_pcie_probe) from [<c01f05b0>] (platform_drv_probe+0x18/0x48)
> [<c01f05b0>] (platform_drv_probe) from [<c01ef280>] (driver_probe_device+0x100/0x228)
> [<c01ef280>] (driver_probe_device) from [<c01ef434>] (__driver_attach+0x8c/0x90)
> [<c01ef434>] (__driver_attach) from [<c01eda80>] (bus_for_each_dev+0x54/0x88)
> [<c01eda80>] (bus_for_each_dev) from [<c01eea48>] (bus_add_driver+0xd4/0x1d0)
> [<c01eea48>] (bus_add_driver) from [<c01efa74>] (driver_register+0x78/0xf4)
> [<c01efa74>] (driver_register) from [<c0008984>] (do_one_initcall+0x80/0x1b8)
> [<c0008984>] (do_one_initcall) from [<c058bd14>] (kernel_init_freeable+0xfc/0x1c8)
> [<c058bd14>] (kernel_init_freeable) from [<c0423004>] (kernel_init+0x8/0xec)
> [<c0423004>] (kernel_init) from [<c000e5b8>] (ret_from_fork+0x14/0x3c)
> Code: e3a02000 ebf5fd27 e2507000 0affff59 (e5973010)
> ---[ end trace 20c08b6c6486c9b6 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b