Patchwork sky2: Fix a race between sky2_down and sky2_poll

login
register
mail settings
Submitter Mike McCormack
Date June 12, 2009, 1:16 p.m.
Message ID <392fb48f0906120616n123d8b57h9ef7c8e1735d26bf@mail.gmail.com>
Download mbox | patch
Permalink /patch/28637/
State Rejected
Delegated to: David Miller
Headers show

Comments

Mike McCormack - June 12, 2009, 1:16 p.m.
2009/6/12 Stephen Hemminger <shemminger@linux-foundation.org>

> Does the following fix the problem?

I tried your patch out, but it still got a crash.

The patch below does fix the problem. Does this look better?

thanks,

Mike

-----------------------------

Subject: [PATCH] sky2: Shutdown receive path cleanly in sky2_down

If sky2_down was called while receiving a packet, receive interrupts
would still occur after the receive buffer was free'd causing a crash in
sky2_status_intr.

To avoid further receive interrupts after sky2_down:

* make sure NAPI and interrupts are fully disabled during shutdown
* call sky2_rx_stop after shutting down the receiver
* clear received packets after shutdown

Signed-off-by: Mike McCormack <mikem@ring3k.org>
---
 drivers/net/sky2.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)
Stephen Hemminger - June 12, 2009, 2:16 p.m.
On Fri, 12 Jun 2009 22:16:42 +0900
Mike McCormack <mikem@ring3k.org> wrote:

> 2009/6/12 Stephen Hemminger <shemminger@linux-foundation.org>
> 
> > Does the following fix the problem?
> 
> I tried your patch out, but it still got a crash.
> 
> The patch below does fix the problem. Does this look better?
> 
> thanks,
> 
> Mike
> 
> -----------------------------
> 
> Subject: [PATCH] sky2: Shutdown receive path cleanly in sky2_down
> 
> If sky2_down was called while receiving a packet, receive interrupts
> would still occur after the receive buffer was free'd causing a crash in
> sky2_status_intr.
> 
> To avoid further receive interrupts after sky2_down:
> 
> * make sure NAPI and interrupts are fully disabled during shutdown
> * call sky2_rx_stop after shutting down the receiver
> * clear received packets after shutdown
> 
> Signed-off-by: Mike McCormack <mikem@ring3k.org>
> ---
>  drivers/net/sky2.c |   21 +++++++++++++++------
>  1 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index a2ff9cb..136b362 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -1805,10 +1805,13 @@ static int sky2_down(struct net_device *dev)
>  	/* Disable port IRQ */
>  	imask = sky2_read32(hw, B0_IMSK);
>  	imask &= ~portirq_msk[port];
> -	sky2_write32(hw, B0_IMSK, imask);
> +	sky2_write32(hw, B0_IMSK, 0);
> 

This breaks on 2 port boards. On dual port boards, both ports share
the same IRQ.

Patch

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index a2ff9cb..136b362 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1805,10 +1805,13 @@  static int sky2_down(struct net_device *dev)
 	/* Disable port IRQ */
 	imask = sky2_read32(hw, B0_IMSK);
 	imask &= ~portirq_msk[port];
-	sky2_write32(hw, B0_IMSK, imask);
+	sky2_write32(hw, B0_IMSK, 0);

 	synchronize_irq(hw->pdev->irq);

+	/* disable soft interrupts */
+	napi_disable(&hw->napi);
+
 	sky2_gmac_reset(hw, port);

 	/* Stop transmitter */
@@ -1822,9 +1825,6 @@  static int sky2_down(struct net_device *dev)
 	ctrl &= ~(GM_GPCR_TX_ENA | GM_GPCR_RX_ENA);
 	gma_write16(hw, port, GM_GP_CTRL, ctrl);

-	/* Make sure no packets are pending */
-	napi_synchronize(&hw->napi);
-
 	sky2_write8(hw, SK_REG(port, GPHY_CTRL), GPC_RST_SET);

 	/* Workaround shared GMAC reset */
@@ -1850,11 +1850,11 @@  static int sky2_down(struct net_device *dev)

 	sky2_write32(hw, RB_ADDR(txqaddr[port], RB_CTRL), RB_RST_SET);

-	sky2_rx_stop(sky2);
-
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
 	sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);

+	sky2_rx_stop(sky2);
+
 	sky2_phy_power_down(hw, port);

 	/* turn off LED's */
@@ -1878,6 +1878,15 @@  static int sky2_down(struct net_device *dev)
 	sky2->rx_ring = NULL;
 	sky2->tx_ring = NULL;

+	/* clear receive interrupt condition and IRQ */
+	hw->st_idx = sky2_read16(hw, STAT_PUT_IDX);
+
+	/* re-enable interrupts */
+	sky2_write32(hw, B0_IMSK, imask);
+
+	/* re-enable soft interrupts */
+	napi_enable(&hw->napi);
+
 	return 0;
 }