[PATCH] sky2: Avoid transmitting during sky2_restart()

[PATCH] sky2: Avoid transmitting during sky2_restart()

[Mike McCormack]

Hi Stephen,

"ifconfig eth1 up; pktget eth1" will crashed my machine within 10
seconds. (eth1 is sky2)
It appears that sky2_tx_timeout causes a restart, and packets in the
tx queue are free'd twice (once in sky2_status_intr and once in
sky2_down).
Furthermore, if sky2_xmit_frame is called during sky2_restart, bad
things will happen.

This patch fixes both problems and was tested on top of my previous
sky2_down fix.

thanks,

Mike

---

Block the transmit queue during sky2_restart().
Don't free transmit packets in sky2_status_intr() during restart,
 as they'll be free'd in sky2_tx_clean()

Signed-off-by: Mike McCormack <mikem@ring3k.org>
---
 drivers/net/sky2.c |    9 ++++++++-
 drivers/net/sky2.h |    1 +
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 21ec0ca..77fd1b9 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1540,6 +1540,8 @@ static inline int tx_dist(unsigned tail, unsigned head)
 /* Number of list elements available for next tx */
 static inline int tx_avail(const struct sky2_port *sky2)
 {
+	if (unlikely(sky2->hw->flags & SKY2_HW_RESTARTING))
+		return 0;
 	return sky2->tx_pending - tx_dist(sky2->tx_cons, sky2->tx_prod);
 }

@@ -2366,7 +2368,8 @@ static inline void sky2_tx_done(struct
net_device *dev, u16 last)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);

-	if (netif_running(dev)) {
+	if (netif_running(dev) &&
+	    !(sky2->hw->flags & SKY2_HW_RESTARTING)) {
 		netif_tx_lock(dev);
 		sky2_tx_complete(sky2, last);
 		netif_tx_unlock(dev);
@@ -3081,6 +3084,7 @@ static void sky2_restart(struct work_struct *work)
 	int i, err;

 	rtnl_lock();
+	hw->flags |= SKY2_HW_RESTARTING;
 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
 		if (netif_running(dev))
@@ -3092,6 +3096,7 @@ static void sky2_restart(struct work_struct *work)
 	sky2_reset(hw);
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 	napi_enable(&hw->napi);
+	hw->flags &= ~SKY2_HW_RESTARTING;

 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
@@ -3102,6 +3107,8 @@ static void sky2_restart(struct work_struct *work)
 				       dev->name, err);
 				dev_close(dev);
 			}
+			else
+				netif_wake_queue(dev);
 		}
 	}

diff --git a/drivers/net/sky2.h b/drivers/net/sky2.h
index b5549c9..e71c161 100644
--- a/drivers/net/sky2.h
+++ b/drivers/net/sky2.h
@@ -2073,6 +2073,7 @@ struct sky2_hw {
 #define SKY2_HW_NEW_LE		0x00000020	/* new LSOv2 format */
 #define SKY2_HW_AUTO_TX_SUM	0x00000040	/* new IP decode for Tx */
 #define SKY2_HW_ADV_POWER_CTL	0x00000080	/* additional PHY power regs */
+#define SKY2_HW_RESTARTING	0x00000100	/* true while restarting */

 	u8	     	     chip_id;
 	u8		     chip_rev;
-- 
1.5.6.5

Re: [PATCH] sky2: Avoid transmitting during sky2_restart()

[Stephen Hemminger]

On Sun, 12 Jul 2009 14:39:29 +0900
Mike McCormack <mikem@ring3k.org> wrote:

> Hi Stephen,
> 
> "ifconfig eth1 up; pktget eth1" will crashed my machine within 10
> seconds. (eth1 is sky2)
> It appears that sky2_tx_timeout causes a restart, and packets in the
> tx queue are free'd twice (once in sky2_status_intr and once in
> sky2_down).
> Furthermore, if sky2_xmit_frame is called during sky2_restart, bad
> things will happen.
> 
> This patch fixes both problems and was tested on top of my previous
> sky2_down fix.
> 
> thanks,
> 
> Mike

This should be fixed by managing the queue properly, not
by using a state flag. I will correct the problem.

Re: [PATCH] sky2: Avoid transmitting during sky2_restart()

[Mike McCormack]

2009/7/13 Stephen Hemminger <shemminger@linux-foundation.org>:
> On Sun, 12 Jul 2009 14:39:29 +0900
> Mike McCormack <mikem@ring3k.org> wrote:
>
>> Hi Stephen,
>>
>> "ifconfig eth1 up; pktget eth1" will crashed my machine within 10
>> seconds. (eth1 is sky2)
>> It appears that sky2_tx_timeout causes a restart, and packets in the
>> tx queue are free'd twice (once in sky2_status_intr and once in
>> sky2_down).
>> Furthermore, if sky2_xmit_frame is called during sky2_restart, bad
>> things will happen.
>>
>> This patch fixes both problems and was tested on top of my previous
>> sky2_down fix.
>>
>> thanks,
>>
>> Mike
>
> This should be fixed by managing the queue properly, not
> by using a state flag. I will correct the problem.

I think some kind of state needs to be used, as sky2_tx_complete()
will wake the tx queue if you use netif_tx_stop_queue() and
netif_tx_lock() is used elsewhere, and is not recursive.  I am happy
to change the patch if you have a suggestion on which function to use.

thanks,

Mike

[PATCH] sky2: Avoid transmitting during sky2_restart()

[Mike McCormack]

Hi Stephen,

I've improved this patch a little to stop the transmit queue before
starting to return NETDEV_TX_BUSY in sky2_xmit_frame().

If you have any suggestions for how to get rid of the
SKY2_HW_RESTARTING state please let me know.  As far as I can tell,
the queue cannot be stopped during a restart using netif_stop_queue
and netif_tx_lock, as those functions are used elsewhere in the code.

thanks,

Mike

---

Block the transmit queue during sky2_restart().
Don't free transmit packets in sky2_status_intr() during restart,
 as they'll be free'd in sky2_tx_clean()

Signed-off-by: Mike McCormack <mikem@ring3k.org>
---
 drivers/net/sky2.c |   19 ++++++++++++++++++-
 drivers/net/sky2.h |    1 +
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 661abd0..826d764 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1533,6 +1533,8 @@ static inline int tx_dist(unsigned tail, unsigned head)
 /* Number of list elements available for next tx */
 static inline int tx_avail(const struct sky2_port *sky2)
 {
+	if (unlikely(sky2->hw->flags & SKY2_HW_RESTARTING))
+		return 0;
 	return sky2->tx_pending - tx_dist(sky2->tx_cons, sky2->tx_prod);
 }

@@ -2359,7 +2361,8 @@ static inline void sky2_tx_done(struct
net_device *dev, u16 last)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);

-	if (netif_running(dev)) {
+	if (netif_running(dev) &&
+	    !(sky2->hw->flags & SKY2_HW_RESTARTING)) {
 		netif_tx_lock(dev);
 		sky2_tx_complete(sky2, last);
 		netif_tx_unlock(dev);
@@ -3074,6 +3077,17 @@ static void sky2_restart(struct work_struct *work)
 	int i, err;

 	rtnl_lock();
+
+	/* stop transmit queues */
+	napi_disable(&hw->napi);
+	for (i = 0; i < hw->ports; i++) {
+		dev = hw->dev[i];
+		if (netif_running(dev))
+			netif_stop_queue(dev);
+	}
+	hw->flags |= SKY2_HW_RESTARTING;
+	napi_enable (&hw->napi);
+
 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
 		if (netif_running(dev))
@@ -3085,6 +3099,7 @@ static void sky2_restart(struct work_struct *work)
 	sky2_reset(hw);
 	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 	napi_enable(&hw->napi);
+	hw->flags &= ~SKY2_HW_RESTARTING;

 	for (i = 0; i < hw->ports; i++) {
 		dev = hw->dev[i];
@@ -3095,6 +3110,8 @@ static void sky2_restart(struct work_struct *work)
 				       dev->name, err);
 				dev_close(dev);
 			}
+			else
+				netif_wake_queue(dev);
 		}
 	}

diff --git a/drivers/net/sky2.h b/drivers/net/sky2.h
index b5549c9..e71c161 100644
--- a/drivers/net/sky2.h
+++ b/drivers/net/sky2.h
@@ -2073,6 +2073,7 @@ struct sky2_hw {
 #define SKY2_HW_NEW_LE		0x00000020	/* new LSOv2 format */
 #define SKY2_HW_AUTO_TX_SUM	0x00000040	/* new IP decode for Tx */
 #define SKY2_HW_ADV_POWER_CTL	0x00000080	/* additional PHY power regs */
+#define SKY2_HW_RESTARTING	0x00000100	/* true while restarting */

 	u8	     	     chip_id;
 	u8		     chip_rev;
-- 
1.5.6.5