[Broken] PCI: clean up resource alignment management

[Broken] PCI: clean up resource alignment management

[Jeff Chua]

Linus, Ivan,

 I'm having problem loading pccard (Sony PC300 broadband card) with the
 recent git download, and bisected down to this commit.

 commit 884525655d07fdee9245716b998ecdc45cdd8007
 Author: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
 Date:   Sun Mar 30 19:50:14 2008 +0400

    PCI: clean up resource alignment management


 The symptom that I'm seeing is ...

 yenta_cardbus 0000:15:00.0: device not available because of BAR 7
 [100:1ff] collisions
 yenta_cardbus: probe of 0000:15:00.0 failed with error -16

 Reverting the commit solves the problem.

 Let me know if you need me to debug further.

 Thanks,
 Jeff.

Re: [Broken] PCI: clean up resource alignment management

[Linus Torvalds]

On Wed, 23 Apr 2008, Jeff Chua wrote:
> 
> I'm having problem loading pccard (Sony PC300 broadband card) with the
> recent git download, and bisected down to this commit.
> 
> commit 884525655d07fdee9245716b998ecdc45cdd8007
> Author: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Date:   Sun Mar 30 19:50:14 2008 +0400
> 
>     PCI: clean up resource alignment management

Ok, at worst we'll have to revert it, but before doing that, can you set 
up a bugzilla entry with a before-and-after version of "lspci -vv", full 
dmesg, and the output of /proc/iomem and /proc/ioports?

> The symptom that I'm seeing is ...
> 
> yenta_cardbus 0000:15:00.0: device not available because of BAR 7 [100:1ff] collisions
> yenta_cardbus: probe of 0000:15:00.0 failed with error -16

I suspect there wasn't a _real_ collision there, but the allocation failed 
because of the alignment bits not being set up right for cardbus bridges, 
but I'm not seeing the bug right now.

Ivan?

			Linus

Re: [Broken] PCI: clean up resource alignment management

[Ivan Kokshaysky]

On Tue, Apr 22, 2008 at 10:25:34AM -0700, Linus Torvalds wrote:
> On Wed, 23 Apr 2008, Jeff Chua wrote:
> > yenta_cardbus 0000:15:00.0: device not available because of BAR 7 [100:1ff] collisions
> > yenta_cardbus: probe of 0000:15:00.0 failed with error -16
> 
> I suspect there wasn't a _real_ collision there, but the allocation failed 
> because of the alignment bits not being set up right for cardbus bridges, 
> but I'm not seeing the bug right now.
> 
> Ivan?

Yes, exactly. My fault - I somehow missed the cardbus stuff...

The patch below should fix that.

Ivan.

---
PCI: make resource_alignment() work for cardbus bridges

Signed-off-by: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
---
 drivers/pci/setup-bus.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f9b7bdd..dbd80db 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -418,11 +418,11 @@ static void pci_bus_size_cardbus(struct pci_bus *bus)
 	 */
 	b_res[0].start = pci_cardbus_io_size;
 	b_res[0].end = b_res[0].start + pci_cardbus_io_size - 1;
-	b_res[0].flags |= IORESOURCE_IO;
+	b_res[0].flags |= IORESOURCE_IO | IORESOURCE_STARTALIGN;
 
 	b_res[1].start = pci_cardbus_io_size;
 	b_res[1].end = b_res[1].start + pci_cardbus_io_size - 1;
-	b_res[1].flags |= IORESOURCE_IO;
+	b_res[1].flags |= IORESOURCE_IO | IORESOURCE_STARTALIGN;
 
 	/*
 	 * Check whether prefetchable memory is supported
@@ -443,15 +443,16 @@ static void pci_bus_size_cardbus(struct pci_bus *bus)
 	if (ctrl & PCI_CB_BRIDGE_CTL_PREFETCH_MEM0) {
 		b_res[2].start = pci_cardbus_mem_size;
 		b_res[2].end = b_res[2].start + pci_cardbus_mem_size - 1;
-		b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+		b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH |
+				  IORESOURCE_STARTALIGN;
 
 		b_res[3].start = pci_cardbus_mem_size;
 		b_res[3].end = b_res[3].start + pci_cardbus_mem_size - 1;
-		b_res[3].flags |= IORESOURCE_MEM;
+		b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_STARTALIGN;
 	} else {
 		b_res[3].start = pci_cardbus_mem_size * 2;
 		b_res[3].end = b_res[3].start + pci_cardbus_mem_size * 2 - 1;
-		b_res[3].flags |= IORESOURCE_MEM;
+		b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_STARTALIGN;
 	}
 }
 

Re: [Broken] PCI: clean up resource alignment management

[Linus Torvalds]

On Wed, 23 Apr 2008, Ivan Kokshaysky wrote:
> 
> Yes, exactly. My fault - I somehow missed the cardbus stuff...

Ok, this patch looks sane, but ...

Don't cardbus bus resources work the same way as _normal_ IO resources, ie 
the alignment is the same as the size?

So I think we could clean this up a bit, and just use IORESOURCE_SIZEALIGN 
for Cardbus bridges, which leads to much more obvious code (ie just keep 
the base at zero)?

(In fact, I think cardbus bridges have a much more flexible alignment than 
that size thing, and can be aligned at finer granularity, but I forget the 
exact details, and since we didn't add a special "alignment" field, we 
can't take advantage of it anyway).

Patch entirely UNTESTED!

		Linus

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

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f9b7bdd..8ddb918 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -416,13 +416,13 @@ static void pci_bus_size_cardbus(struct pci_bus *bus)
 	 * Reserve some resources for CardBus.  We reserve
 	 * a fixed amount of bus space for CardBus bridges.
 	 */
-	b_res[0].start = pci_cardbus_io_size;
-	b_res[0].end = b_res[0].start + pci_cardbus_io_size - 1;
-	b_res[0].flags |= IORESOURCE_IO;
+	b_res[0].start = 0;
+	b_res[0].end = pci_cardbus_io_size - 1;
+	b_res[0].flags |= IORESOURCE_IO | IORESOURCE_SIZEALIGN;
 
-	b_res[1].start = pci_cardbus_io_size;
-	b_res[1].end = b_res[1].start + pci_cardbus_io_size - 1;
-	b_res[1].flags |= IORESOURCE_IO;
+	b_res[1].start = 0;
+	b_res[1].end = pci_cardbus_io_size - 1;
+	b_res[1].flags |= IORESOURCE_IO | IORESOURCE_SIZEALIGN;
 
 	/*
 	 * Check whether prefetchable memory is supported
@@ -441,17 +441,17 @@ static void pci_bus_size_cardbus(struct pci_bus *bus)
 	 * twice the size.
 	 */
 	if (ctrl & PCI_CB_BRIDGE_CTL_PREFETCH_MEM0) {
-		b_res[2].start = pci_cardbus_mem_size;
-		b_res[2].end = b_res[2].start + pci_cardbus_mem_size - 1;
-		b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+		b_res[2].start = 0;
+		b_res[2].end = pci_cardbus_mem_size - 1;
+		b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH | IORESOURCE_SIZEALIGN;
 
-		b_res[3].start = pci_cardbus_mem_size;
-		b_res[3].end = b_res[3].start + pci_cardbus_mem_size - 1;
-		b_res[3].flags |= IORESOURCE_MEM;
+		b_res[3].start = 0;
+		b_res[3].end = pci_cardbus_mem_size - 1;
+		b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_SIZEALIGN;
 	} else {
-		b_res[3].start = pci_cardbus_mem_size * 2;
-		b_res[3].end = b_res[3].start + pci_cardbus_mem_size * 2 - 1;
-		b_res[3].flags |= IORESOURCE_MEM;
+		b_res[3].start = 0;
+		b_res[3].end = pci_cardbus_mem_size * 2 - 1;
+		b_res[3].flags |= IORESOURCE_MEM | IORESOURCE_SIZEALIGN;
 	}
 }
 

Re: [Broken] PCI: clean up resource alignment management

[Linus Torvalds]

On Tue, 22 Apr 2008, Linus Torvalds wrote:
> 
> (In fact, I think cardbus bridges have a much more flexible alignment than 
> that size thing, and can be aligned at finer granularity, but I forget the 
> exact details, and since we didn't add a special "alignment" field, we 
> can't take advantage of it anyway).

Yeah, I double-checked: the IO alignment is 4 bytes, the MEM alignment is 
4k.

So if we had a separate alignment word we could use that, but in the 
meantime I think IORESOURCE_SIZEALIGN is good enough, and a bit more 
readable than the STARTALIGN thing.

Now we just need to have somebody test it. It turns out that I no longer 
have any Cardbus cards, since my laptops now all have built-in wireless. 
Boo hiss for me.

			Linus

Re: [Broken] PCI: clean up resource alignment management

[Jesse Barnes]

On Tuesday, April 22, 2008 3:13 pm Linus Torvalds wrote:
> On Tue, 22 Apr 2008, Linus Torvalds wrote:
> > (In fact, I think cardbus bridges have a much more flexible alignment
> > than that size thing, and can be aligned at finer granularity, but I
> > forget the exact details, and since we didn't add a special "alignment"
> > field, we can't take advantage of it anyway).
>
> Yeah, I double-checked: the IO alignment is 4 bytes, the MEM alignment is
> 4k.
>
> So if we had a separate alignment word we could use that, but in the
> meantime I think IORESOURCE_SIZEALIGN is good enough, and a bit more
> readable than the STARTALIGN thing.
>
> Now we just need to have somebody test it. It turns out that I no longer
> have any Cardbus cards, since my laptops now all have built-in wireless.
> Boo hiss for me.

I may be in the same boat, I'll have to dig around and see if I have any...  
That said, I think an explicit alignment field would make things even cleaner 
(and like you said allow us to take advantage of bus specific alignment 
better).

Jesse

Re: [Broken] PCI: clean up resource alignment management

[Jeff Chua]

On Wed, Apr 23, 2008 at 5:59 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>  On Wed, 23 Apr 2008, Ivan Kokshaysky wrote:
>  > Yes, exactly. My fault - I somehow missed the cardbus stuff...
>  Ok, this patch looks sane, but ..
>  Patch entirely UNTESTED!

I just tested Linus's patch and it works.

Thanks,
Jeff.

Re: [Broken] PCI: clean up resource alignment management

[Linus Torvalds]

On Tue, 22 Apr 2008, Jesse Barnes wrote:

> On Tuesday, April 22, 2008 3:13 pm Linus Torvalds wrote:
> >
> > Now we just need to have somebody test it. It turns out that I no longer
> > have any Cardbus cards, since my laptops now all have built-in wireless.
> > Boo hiss for me.
> 
> I may be in the same boat, I'll have to dig around and see if I have any...  
> That said, I think an explicit alignment field would make things even cleaner 
> (and like you said allow us to take advantage of bus specific alignment 
> better).

Never mind. The error happens even without a card or driver, just plain 
cardbus. 

I'll test the fix and check it in if it looks ok,

		Linus

Re: [Broken] PCI: clean up resource alignment management

[Linus Torvalds]

On Wed, 23 Apr 2008, Jeff Chua wrote:

> On Wed, Apr 23, 2008 at 5:59 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >  On Wed, 23 Apr 2008, Ivan Kokshaysky wrote:
> >  > Yes, exactly. My fault - I somehow missed the cardbus stuff...
> >  Ok, this patch looks sane, but ..
> >  Patch entirely UNTESTED!
> 
> I just tested Linus's patch and it works.

Thanks, and I could test it myself (without any actual card, but at least 
I could see the failure to even set up the bridge, and the fix). 

So I committed it.

When we did the original commit that caused this, we had considered having 
a separate "alignment" value, but I had discarded it because I didn't 
think there was any actual hardware that could even use it. But this 
cardbus thing shows that I was wrong - the two bits may have been clever, 
and it works no worse than the old setup (and slightly better), but I 
think the separate alignment field would probably have been better.

Anyway, it's probably not worth worrying about now. It's not like we've 
ever _needed_ the finer-granularity alignment, so I think we're ok with 
the current setup, but if we ever decide to add the alignment field after 
all, somebody should remind me/Ink about the cardbus thing.

			Linus

Re: [Broken] PCI: clean up resource alignment management

[Jeff Chua]

On Wed, Apr 23, 2008 at 9:26 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:

>  Thanks, and I could test it myself (without any actual card, but at least
>  I could see the failure to even set up the bridge, and the fix).
>
>  So I committed it.

Cool. Thank you.  And the patch does works. I'm using the wireless modem now.

Jeff.