[PATCH] Re : [BUG] cardbus/hotplugging still broken in 2.5.56

[PATCH] Re : [BUG] cardbus/hotplugging still broken in 2.5.56

[Yaacov Akiba Slama]

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

Hello all !

I have a xircom (RBEM56G-100) lan+modem, and it seems that the following 
patch solves the problem of ressources collisions.
Now, I can use 2.5.58mm1 in my laptop and I am happy.

Yaacov Akiba Slama

[-- Attachment #2: cardbus.patch --]
[-- Type: text/plain, Size: 708 bytes --]

--- a/drivers/pcmcia/cardbus.c	2003-01-14 19:38:49.000000000 +0200
+++ b/drivers/pcmcia/cardbus.c	2003-01-14 19:57:13.000000000 +0200
@@ -285,18 +285,19 @@
 		dev->dev.dma_mask = &dev->dma_mask;
 
 		pci_setup_device(dev);
-		if (pci_enable_device(dev))
-			continue;
 
 		strcpy(dev->dev.bus_id, dev->slot_name);
 
 		/* FIXME: Do we need to enable the expansion ROM? */
 		for (r = 0; r < 7; r++) {
 			struct resource *res = dev->resource + r;
-			if (res->flags)
+			if (!res->start && res->end)
 				pci_assign_resource(dev, r);
 		}
 
+		if (pci_enable_device(dev))
+			continue;
+
 		/* Does this function have an interrupt at all? */
 		pci_readb(dev, PCI_INTERRUPT_PIN, &irq_pin);
 		if (irq_pin) {

Re: [PATCH] Re: [BUG] cardbus/hotplugging still broken in 2.5.56

[Yaacov Akiba Slama]

jens.taprogge@rwth-aachen.de wrote:

>I am not sure if you have seen the patch I posted on l-k. It should fix
>both issues.
>
I don't know enough about pci/cardbus, but in 
arch/i386/pci.c::pcibios_assign_resources, I can see the following :

if (!r->start && r->end)
                pci_assign_resource(dev, idx);

without testing the result and without freeing the ressource for all 
index if it fails on one index. So I don't know if your tests are necessary.
Beside that, testing (!r->start && r->end) seems to be more in sync 
with arch/i386/pci.c than testing r->flags

Thanks,
Yaacov Akiba Slama

>
>Jens
>
>On Wed, Jan 15, 2003 at 10:13:39PM +0200, Yaacov Akiba Slama wrote:
>  
>
>>Jens Taprogge wrote :
>>
>>    
>>
>>>You are not freeing the possibly already allocated resources in case of
>>>a failure of either pci_assign_resource() or pca_enable_device(). In
>>>fact you are not even checking if pci_assign_resource() fails. That
>>>seems wrong to me.
>>>      
>>>
>>There are two separate issues :
>>1) Fix the "ressource collisions" problem (and irq not known).
>>2) Freeing ressources in case of failure of some functions.
>>
>>My patch solves the first issue only in order to make cardbus with rom work.
>>The point 2 is a janitor work.
>>
>>Thanks,
>>Yaacov Akiba Slama
>>    
>>
>
>  
>

Re: [PATCH] Re: [BUG] cardbus/hotplugging still broken in 2.5.56

[jens.taprogge]

I am not sure if you have seen the patch I posted on l-k. It should fix
both issues. 

Jens

On Wed, Jan 15, 2003 at 10:13:39PM +0200, Yaacov Akiba Slama wrote:
> Jens Taprogge wrote :
> 
> >You are not freeing the possibly already allocated resources in case of
> >a failure of either pci_assign_resource() or pca_enable_device(). In
> >fact you are not even checking if pci_assign_resource() fails. That
> >seems wrong to me.
> 
> There are two separate issues :
> 1) Fix the "ressource collisions" problem (and irq not known).
> 2) Freeing ressources in case of failure of some functions.
> 
> My patch solves the first issue only in order to make cardbus with rom work.
> The point 2 is a janitor work.
> 
> Thanks,
> Yaacov Akiba Slama

-- 
Jens Taprogge

Re: [PATCH] Re: [BUG] cardbus/hotplugging still broken in 2.5.56

[Dave Jones]

On Wed, Jan 15, 2003 at 08:47:38PM +0100, Jens Taprogge wrote:
 > You are not freeing the possibly already allocated resources in case of
 > a failure of either pci_assign_resource() or pca_enable_device(). In
 > fact you are not even checking if pci_assign_resource() fails. That
 > seems wrong to me.
 > 
 > Btw.: who is in charge of this file? Dave Jones?

Not me, more likely David Hinds.

		Dave

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH] Re: [BUG] cardbus/hotplugging still broken in 2.5.56

[Yaacov Akiba Slama]

Jens Taprogge wrote :

>You are not freeing the possibly already allocated resources in case of
>a failure of either pci_assign_resource() or pca_enable_device(). In
>fact you are not even checking if pci_assign_resource() fails. That
>seems wrong to me.

There are two separate issues :
1) Fix the "ressource collisions" problem (and irq not known).
2) Freeing ressources in case of failure of some functions.

My patch solves the first issue only in order to make cardbus with rom work.
The point 2 is a janitor work.

Thanks,
Yaacov Akiba Slama

Re: [PATCH] Re: [BUG] cardbus/hotplugging still broken in 2.5.56

[Jens Taprogge]

You are not freeing the possibly already allocated resources in case of
a failure of either pci_assign_resource() or pca_enable_device(). In
fact you are not even checking if pci_assign_resource() fails. That
seems wrong to me.

Btw.: who is in charge of this file? Dave Jones? Is someone working on
integrating it with PCI Hotplug?

Best Regards 
Jens


On Wed, Jan 15, 2003 at 06:26:36PM +0200, Yaacov Akiba Slama wrote:
> Can you test the enclosed patch. It seems to be both simple and to 
> resolve the resource collisions and the "No IRQ known" problem.
> I added a smal comment (and modified another) so future janitors won't 
> move pci_enable above pci_assign_resource again.
> If everyone is ok, I can send it to Linus for inclusion in BK (I have 
> the green light of Dave Jones).
> 
> Thanks,
> Yaacov Akiba Slama
> 
> Mikael Pettersson wrote:
> 
> >Jens Taprogge writes:
> >> The cardbus problems are caused by 
> >> 
> >> ChangeSet@1.797.145.6  2002-11-25 18:31:10-08:00 davej@codemonkey.org.uk
> >> 
> >> as far as I can tell. 
> >> 
> >> pci_enable_device() will fail at least on i386 (see
> >> arch/i386/pci/i386.c: pcibios_enable_resource (line 260)) if the
> >> resources have not been assigned previously. Hence the ostensible
> >> resource collisions.
> >> 
> >> The attached patch should fix the problem.
> >> 
> >> I have send the patch to Dave Jones some time ago but did not hear from
> >> him yet.
> >> 
> >> I am not subscribed to the list so please cc me on replys. 
> >
> >Thanks. Your patch fixed the cardbus hotplug issue perfectly on my laptop.
> >It survives multiple insert/eject cycles without any problems.
> >
> >The patch posted by Yaacov Akiba Slama today also fixed cardbus hotplug
> >for me, but with his patch the kernel still prints "PCI: No IRQ known for
> >interrupt pin A of device xx:xx.x. Please try using pci=biosirq" when the
> >cardbus NIC is inserted; Jens Taprogge's patch silenced that warning.
> >
> >/Mikael
> >
> > 
> >

> --- drivers/pcmcia/cardbus.c.original	2003-01-14 19:38:49.000000000 +0200
> +++ drivers/pcmcia/cardbus.c	2003-01-15 18:21:40.000000000 +0200
> @@ -285,25 +285,29 @@
>  		dev->dev.dma_mask = &dev->dma_mask;
>  
>  		pci_setup_device(dev);
> -		if (pci_enable_device(dev))
> -			continue;
>  
>  		strcpy(dev->dev.bus_id, dev->slot_name);
>  
> -		/* FIXME: Do we need to enable the expansion ROM? */
> +		/* We need to assign resources for expansion ROM. */
>  		for (r = 0; r < 7; r++) {
>  			struct resource *res = dev->resource + r;
> -			if (res->flags)
> +			if (!res->start && res->end)
>  				pci_assign_resource(dev, r);
>  		}
>  
>  		/* Does this function have an interrupt at all? */
>  		pci_readb(dev, PCI_INTERRUPT_PIN, &irq_pin);
> -		if (irq_pin) {
> +		if (irq_pin)
>  			dev->irq = irq;
> -			pci_writeb(dev, PCI_INTERRUPT_LINE, irq);
> -		}
> +		
> +		/* pci_enable_device needs to be called after pci_assign_resource */
> +		/* because it returns an error if (!res->start && res->end).      */
> +		if (pci_enable_device(dev))
> +			continue;
>  
> +		if (irq_pin)
> +			pci_writeb(dev, PCI_INTERRUPT_LINE, irq);
> +		
>  		device_register(&dev->dev);
>  		pci_insert_device(dev, bus);
>  	}


-- 
Jens Taprogge

[PATCH] Re: [BUG] cardbus/hotplugging still broken in 2.5.56

[Yaacov Akiba Slama]

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

Can you test the enclosed patch. It seems to be both simple and to 
resolve the resource collisions and the "No IRQ known" problem.
I added a smal comment (and modified another) so future janitors won't 
move pci_enable above pci_assign_resource again.
If everyone is ok, I can send it to Linus for inclusion in BK (I have 
the green light of Dave Jones).

Thanks,
Yaacov Akiba Slama

Mikael Pettersson wrote:

>Jens Taprogge writes:
> > The cardbus problems are caused by 
> > 
> > ChangeSet@1.797.145.6  2002-11-25 18:31:10-08:00 davej@codemonkey.org.uk
> > 
> > as far as I can tell. 
> > 
> > pci_enable_device() will fail at least on i386 (see
> > arch/i386/pci/i386.c: pcibios_enable_resource (line 260)) if the
> > resources have not been assigned previously. Hence the ostensible
> > resource collisions.
> > 
> > The attached patch should fix the problem.
> > 
> > I have send the patch to Dave Jones some time ago but did not hear from
> > him yet.
> > 
> > I am not subscribed to the list so please cc me on replys. 
>
>Thanks. Your patch fixed the cardbus hotplug issue perfectly on my laptop.
>It survives multiple insert/eject cycles without any problems.
>
>The patch posted by Yaacov Akiba Slama today also fixed cardbus hotplug
>for me, but with his patch the kernel still prints "PCI: No IRQ known for
>interrupt pin A of device xx:xx.x. Please try using pci=biosirq" when the
>cardbus NIC is inserted; Jens Taprogge's patch silenced that warning.
>
>/Mikael
>
>  
>

[-- Attachment #2: cardbus-rom.patch --]
[-- Type: text/plain, Size: 1137 bytes --]

--- drivers/pcmcia/cardbus.c.original	2003-01-14 19:38:49.000000000 +0200
+++ drivers/pcmcia/cardbus.c	2003-01-15 18:21:40.000000000 +0200
@@ -285,25 +285,29 @@
 		dev->dev.dma_mask = &dev->dma_mask;
 
 		pci_setup_device(dev);
-		if (pci_enable_device(dev))
-			continue;
 
 		strcpy(dev->dev.bus_id, dev->slot_name);
 
-		/* FIXME: Do we need to enable the expansion ROM? */
+		/* We need to assign resources for expansion ROM. */
 		for (r = 0; r < 7; r++) {
 			struct resource *res = dev->resource + r;
-			if (res->flags)
+			if (!res->start && res->end)
 				pci_assign_resource(dev, r);
 		}
 
 		/* Does this function have an interrupt at all? */
 		pci_readb(dev, PCI_INTERRUPT_PIN, &irq_pin);
-		if (irq_pin) {
+		if (irq_pin)
 			dev->irq = irq;
-			pci_writeb(dev, PCI_INTERRUPT_LINE, irq);
-		}
+		
+		/* pci_enable_device needs to be called after pci_assign_resource */
+		/* because it returns an error if (!res->start && res->end).      */
+		if (pci_enable_device(dev))
+			continue;
 
+		if (irq_pin)
+			pci_writeb(dev, PCI_INTERRUPT_LINE, irq);
+		
 		device_register(&dev->dev);
 		pci_insert_device(dev, bus);
 	}