Thread (57 messages) 57 messages, 10 authors, 2010-01-16

Re: [PATCH] sky2: safer transmit ring cleaning (v4)

From: Michael Breuer <hidden>
Date: 2010-01-14 16:46:51

On 1/14/2010 8:19 AM, Mike McCormack wrote:
quoted hunk ↗ jump to hunk
Jarek Poplawski wrote:
   
quoted
On Thu, Jan 14, 2010 at 03:20:09AM -0800, David Miller wrote:
     
quoted
From: Jarek Poplawski<redacted>
Date: Thu, 14 Jan 2010 11:16:36 +0000

       
quoted
So, now I really ;-) agree with David: this needs a proper fix.
         
Now Jarek, do you now see my dirty little secret?

When people disagree with me, I just silently sit around waiting for
them to eventually change their mind.

Isn't it brilliant? -)
       
If it were that easy... (it never works for me :-()

Probably, there is something more... ;-)
     
Here's what was sitting in my tree...


Subject: [PATCH] sky2: Don't detach device when restarting

Block the tx queue from transmitting when restarting
  rather than trying to take the device offline.

Signed-off-by: Mike McCormack<redacted>
---
  drivers/net/sky2.c |   13 +++++++++----
  1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index d76d907..061f6f2 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1580,6 +1580,8 @@ static int sky2_up(struct net_device *dev)
  	if (netif_msg_ifup(sky2))
  		printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);

+	netif_start_queue(dev);
+
  	return 0;

  err_out:
@@ -1596,6 +1598,8 @@ static inline int tx_inuse(const struct sky2_port *sky2)
  /* Number of list elements available for next tx */
  static inline int tx_avail(const struct sky2_port *sky2)
  {
+	if (unlikely(!sky2->tx_ring))
+		return 0;
  	return sky2->tx_pending - tx_inuse(sky2);
  }
@@ -1925,7 +1929,9 @@ static int sky2_down(struct net_device *dev)
  	sky2_read32(hw, B0_IMSK);

  	synchronize_irq(hw->pdev->irq);
-	napi_synchronize(&hw->napi);
+	netif_tx_lock_bh(dev);
+	napi_disable(&hw->napi);
+	netif_stop_queue(dev);

  	spin_lock_bh(&sky2->phy_lock);
  	sky2_phy_power_down(hw, port);
@@ -1939,6 +1945,8 @@ static int sky2_down(struct net_device *dev)
  	sky2_rx_clean(sky2);

  	sky2_free_buffers(sky2);
+	napi_enable(&hw->napi);
+	netif_tx_unlock_bh(dev);

  	return 0;
  }
@@ -3177,9 +3185,6 @@ static void sky2_reset(struct sky2_hw *hw)
  static void sky2_detach(struct net_device *dev)
  {
  	if (netif_running(dev)) {
-		netif_tx_lock_bh(dev);
-		netif_device_detach(dev);	/* stop txq */
-		netif_tx_unlock_bh(dev);
  		sky2_down(dev);
  	}
  }
-- 1.5.6.5
   
Ok - no obvious difference with this patch + Stephen's.

I still see:

No reported errors; decent throughput; earlier issues with IRQ resolved.
But... still seeing DHCP DISCOVER/OFFER (no REQUEST/ACK) while under 
load. I can try to sniff this... but given that it's under load, I'd 
probably have to filter out non DHCP stuff and might miss whatever is 
really going on. Again - main point here is that absent these patches I 
don't see this issue. It's also possible that these two patches allow 
the load to be heavier thus actually causing a different problem.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help