[patch 18/19] mv643xx ethernet driver IRQ registration fix

[patch 18/19] mv643xx ethernet driver IRQ registration fix

[akpm]

From: Giridhar Pemmasani <giri@lmc.cs.sunysb.edu>

During initialization, mv643xx driver registers IRQ before setting up tx/rx
rings.  This causes kernel oops because mv643xx_poll, which gets called
right after registering IRQ, calls netif_rx_complete, which accesses the rx
ring (I don't have the oops message anymore; I just remember this sequence
of calls).  Attached (tested) patch first initializes the rx/tx rings and
then registers the IRQ.

Signed-off-by: Giridhar Pemmasani <pgiri@yahoo.com>
Cc: Dale Farnsworth <dale@farnsworth.org>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/mv643xx_eth.c |   40 +++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff -puN drivers/net/mv643xx_eth.c~mv643xx-ethernet-driver-irq-registration-fix drivers/net/mv643xx_eth.c
--- a/drivers/net/mv643xx_eth.c~mv643xx-ethernet-driver-irq-registration-fix
+++ a/drivers/net/mv643xx_eth.c
@@ -787,14 +787,6 @@ static int mv643xx_eth_open(struct net_d
 	unsigned int size;
 	int err;
 
-	err = request_irq(dev->irq, mv643xx_eth_int_handler,
-			IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, dev);
-	if (err) {
-		printk(KERN_ERR "Can not assign IRQ number to MV643XX_eth%d\n",
-								port_num);
-		return -EAGAIN;
-	}
-
 	eth_port_init(mp);
 
 	memset(&mp->timeout, 0, sizeof(struct timer_list));
@@ -806,8 +798,7 @@ static int mv643xx_eth_open(struct net_d
 								GFP_KERNEL);
 	if (!mp->rx_skb) {
 		printk(KERN_ERR "%s: Cannot allocate Rx skb ring\n", dev->name);
-		err = -ENOMEM;
-		goto out_free_irq;
+		return -ENOMEM;
 	}
 	mp->tx_skb = kmalloc(sizeof(*mp->tx_skb) * mp->tx_ring_size,
 								GFP_KERNEL);
@@ -861,13 +852,8 @@ static int mv643xx_eth_open(struct net_d
 							dev->name, size);
 		printk(KERN_ERR "%s: Freeing previously allocated TX queues...",
 							dev->name);
-		if (mp->rx_sram_size)
-			iounmap(mp->p_tx_desc_area);
-		else
-			dma_free_coherent(NULL, mp->tx_desc_area_size,
-					mp->p_tx_desc_area, mp->tx_desc_dma);
 		err = -ENOMEM;
-		goto out_free_tx_skb;
+		goto out_free_tx_ring;
 	}
 	memset((void *)mp->p_rx_desc_area, 0, size);
 
@@ -875,6 +861,14 @@ static int mv643xx_eth_open(struct net_d
 
 	mv643xx_eth_rx_refill_descs(dev);	/* Fill RX ring with skb's */
 
+	err = request_irq(dev->irq, mv643xx_eth_int_handler,
+			IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, dev);
+	if (err) {
+		printk(KERN_ERR "Can not assign IRQ number to MV643XX_eth%d\n",
+			port_num);
+		goto out_free_rx_ring;
+	}
+
 	/* Clear any pending ethernet port interrupts */
 	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_REG(port_num), 0);
 	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num), 0);
@@ -900,12 +894,22 @@ static int mv643xx_eth_open(struct net_d
 
 	return 0;
 
+out_free_rx_ring:
+	if (mp->rx_sram_size)
+		iounmap(mp->p_rx_desc_area);
+	else
+		dma_free_coherent(NULL, mp->rx_desc_area_size,
+			mp->p_rx_desc_area, mp->rx_desc_dma);
+out_free_tx_ring:
+	if (mp->tx_sram_size)
+		iounmap(mp->p_tx_desc_area);
+	else
+		dma_free_coherent(NULL, mp->tx_desc_area_size,
+				mp->p_tx_desc_area, mp->tx_desc_dma);
 out_free_tx_skb:
 	kfree(mp->tx_skb);
 out_free_rx_skb:
 	kfree(mp->rx_skb);
-out_free_irq:
-	free_irq(dev->irq, dev);
 
 	return err;
 }
_

Re: [patch 18/19] mv643xx ethernet driver IRQ registration fix

[Jeff Garzik]

akpm@linux-foundation.org wrote:
> From: Giridhar Pemmasani <giri@lmc.cs.sunysb.edu>
> 
> During initialization, mv643xx driver registers IRQ before setting up tx/rx
> rings.  This causes kernel oops because mv643xx_poll, which gets called
> right after registering IRQ, calls netif_rx_complete, which accesses the rx
> ring (I don't have the oops message anymore; I just remember this sequence
> of calls).  Attached (tested) patch first initializes the rx/tx rings and
> then registers the IRQ.
> 
> Signed-off-by: Giridhar Pemmasani <pgiri@yahoo.com>
> Cc: Dale Farnsworth <dale@farnsworth.org>
> Cc: Jeff Garzik <jeff@garzik.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/net/mv643xx_eth.c |   40 +++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 18 deletions(-)

seems sane enough to me, but I would like to get this via Dale, who has 
been a fairly active maintainer so far

Re: [patch 18/19] mv643xx ethernet driver IRQ registration fix

[Dale Farnsworth]

On Tue, Mar 06, 2007 at 02:42:02AM -0800, akpm@linux-foundation.org wrote:
> From: Giridhar Pemmasani <giri@lmc.cs.sunysb.edu>
> 
> During initialization, mv643xx driver registers IRQ before setting up tx/rx
> rings.  This causes kernel oops because mv643xx_poll, which gets called
> right after registering IRQ, calls netif_rx_complete, which accesses the rx
> ring (I don't have the oops message anymore; I just remember this sequence
> of calls).  Attached (tested) patch first initializes the rx/tx rings and
> then registers the IRQ.

I believe a better fix is to disable any pending interrupt sources
before calling request_irq().  I sent the patch below to Giri for
confirmation, but haven't heard back.  I should've copied Andrew.

Giri, have you had a chance to test this alternative patch?

Thanks,
-Dale

> Date: Fri, 2 Mar 2007 17:03:53 -0700
> To: Giridhar Pemmasani <giri@lmc.cs.sunysb.edu>
> Message-ID: <20070303000353.GA1022@xyzzy.farnsworth.org>
> In-Reply-To: <es8ahm$64p$1@sea.gmane.org>
> 
> On Fri, Mar 02, 2007 at 04:52:06AM +0000, Giridhar Pemmasani wrote:
> > During initialization, mv643xx driver registers IRQ before setting up tx/rx
> > rings. This causes kernel oops because mv643xx_poll, which gets called
> > right after registering IRQ, calls netif_rx_complete, which accesses the rx
> > ring (I don't have the oops message anymore; I just remember this sequence
> > of calls). Attached (tested) patch first initializes the rx/tx rings and
> > then registers the IRQ.
> > 
> > Giri
> > 
> > Signed-off-by: Giridhar Pemmasani <pgiri@yahoo.com>
> 
> Giridhar, does the following patch fix your problem, in place of the
> patch you supplied?
> 
> -Dale

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 3e045a6..192b390 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -787,6 +787,12 @@ static int mv643xx_eth_open(struct net_device *dev)
 	unsigned int size;
 	int err;
 
+	/* Clear any pending ethernet port interrupts */
+	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_REG(port_num), 0);
+	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num), 0);
+	/* wait for previous write to complete */
+	mv_read (MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num));
+
 	err = request_irq(dev->irq, mv643xx_eth_int_handler,
 			IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, dev);
 	if (err) {
@@ -875,10 +881,6 @@ static int mv643xx_eth_open(struct net_device *dev)
 
 	mv643xx_eth_rx_refill_descs(dev);	/* Fill RX ring with skb's */
 
-	/* Clear any pending ethernet port interrupts */
-	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_REG(port_num), 0);
-	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num), 0);
-
 	eth_port_start(dev);
 
 	/* Interrupt Coalescing */

Re: [patch 18/19] mv643xx ethernet driver IRQ registration fix

[Giridhar Pemmasani]

--- Dale Farnsworth <dale@farnsworth.org> wrote:

> On Tue, Mar 06, 2007 at 02:42:02AM -0800, akpm@linux-foundation.org wrote:
> > From: Giridhar Pemmasani <giri@lmc.cs.sunysb.edu>
> > 
> > During initialization, mv643xx driver registers IRQ before setting up
> tx/rx
> > rings.  This causes kernel oops because mv643xx_poll, which gets called
> > right after registering IRQ, calls netif_rx_complete, which accesses the
> rx
> > ring (I don't have the oops message anymore; I just remember this
> sequence
> > of calls).  Attached (tested) patch first initializes the rx/tx rings and
> > then registers the IRQ.
> 
> I believe a better fix is to disable any pending interrupt sources
> before calling request_irq().  I sent the patch below to Giri for
> confirmation, but haven't heard back.  I should've copied Andrew.
> 
> Giri, have you had a chance to test this alternative patch?

I will get my hands on the box this Friday. As soon as I can, I will test and
give feedback.

FWIW, I had a patch earlier that was similar (not same) as you suggest. That
patch was based on the changes from 2.6.15 to 2.6.16 when it broke. In
2.6.15, interrupts as well as rx/tx are disabled until rx/tx rings are setup.
Anyway, I will test this and let you know.

Giri


 
____________________________________________________________________________________
Be a PS3 game guru.
Get your game face on with the latest PS3 news and previews at Yahoo! Games.
http://videogames.yahoo.com/platform?platform=120121

Re: [patch 18/19] mv643xx ethernet driver IRQ registration fix

[Giridhar Pemmasani]

--- Dale Farnsworth <dale@farnsworth.org> wrote:

> On Tue, Mar 06, 2007 at 02:42:02AM -0800, akpm@linux-foundation.org wrote:
> > From: Giridhar Pemmasani <giri@lmc.cs.sunysb.edu>
> > 
> > During initialization, mv643xx driver registers IRQ before setting up
> tx/rx
> > rings.  This causes kernel oops because mv643xx_poll, which gets called
> > right after registering IRQ, calls netif_rx_complete, which accesses the
> rx
> > ring (I don't have the oops message anymore; I just remember this
> sequence
> > of calls).  Attached (tested) patch first initializes the rx/tx rings and
> > then registers the IRQ.
> 
> I believe a better fix is to disable any pending interrupt sources
> before calling request_irq().  I sent the patch below to Giri for
> confirmation, but haven't heard back.  I should've copied Andrew.
> 
> Giri, have you had a chance to test this alternative patch?

I got a chance to test this patch today. I can confirm that this patch fixes
the issue reported.

Thanks,
Giri


 
____________________________________________________________________________________
The fish are biting. 
Get more visitors on your site using Yahoo! Search Marketing.
http://searchmarketing.yahoo.com/arp/sponsoredsearch_v2.php

[PATCH] mv643xx: Clear pending interrupts before calling request_irq

[Dale Farnsworth]

From: Dale Farnsworth <dale@farnsworth.org>

Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
Acked-by: Giridhar Pemmasani <pgiri@yahoo.com>

---

Giri, thank you very much for reporting the problem and confirming
the fix.

Jeff, please apply.

Andrew, this patch supersedes the -mm patch:
	mv643xx-ethernet-driver-irq-registration-fix.patch

 drivers/net/mv643xx_eth.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 3e045a6..192b390 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -787,6 +787,12 @@ static int mv643xx_eth_open(struct net_device *dev)
 	unsigned int size;
 	int err;
 
+	/* Clear any pending ethernet port interrupts */
+	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_REG(port_num), 0);
+	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num), 0);
+	/* wait for previous write to complete */
+	mv_read (MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num));
+
 	err = request_irq(dev->irq, mv643xx_eth_int_handler,
 			IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, dev);
 	if (err) {
@@ -875,10 +881,6 @@ static int mv643xx_eth_open(struct net_device *dev)
 
 	mv643xx_eth_rx_refill_descs(dev);	/* Fill RX ring with skb's */
 
-	/* Clear any pending ethernet port interrupts */
-	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_REG(port_num), 0);
-	mv_write(MV643XX_ETH_INTERRUPT_CAUSE_EXTEND_REG(port_num), 0);
-
 	eth_port_start(dev);
 
 	/* Interrupt Coalescing */

Re: [PATCH] mv643xx: Clear pending interrupts before calling request_irq

[Jeff Garzik]

Dale Farnsworth wrote:
> From: Dale Farnsworth <dale@farnsworth.org>
> 
> Signed-off-by: Dale Farnsworth <dale@farnsworth.org>
> Acked-by: Giridhar Pemmasani <pgiri@yahoo.com>
> 
> ---
> 
> Giri, thank you very much for reporting the problem and confirming
> the fix.
> 
> Jeff, please apply.
> 
> Andrew, this patch supersedes the -mm patch:
> 	mv643xx-ethernet-driver-irq-registration-fix.patch
> 
>  drivers/net/mv643xx_eth.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

applied