diff mbox

[2/2] sky2: Allocate initial skbs in sky2_alloc_buffers

Message ID 4B6CC473.4090008@ring3k.org
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Mike McCormack Feb. 6, 2010, 1:22 a.m. UTC 
Allocating everything in one place means there's a single point
of failure in sky2_up, and sky2_rx_start can no longer fail.

With this patch, sky2_restart can later be rewritten to shutdown
the hardware and restart it without freeing and reallocing memory.

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

Comments

stephen hemminger Feb. 6, 2010, 2:10 a.m. UTC | #1 
On Sat, 06 Feb 2010 10:22:59 +0900
Mike McCormack <mikem@ring3k.org> wrote:

> Allocating everything in one place means there's a single point
> of failure in sky2_up, and sky2_rx_start can no longer fail.

If ring is never allocated, how then it must fail in up.
Plus if the initial ring allocation is partial it should fail.
stephen hemminger Feb. 6, 2010, 6:12 a.m. UTC | #2 
On Fri, 5 Feb 2010 18:10:45 -0800
Stephen Hemminger <shemminger@vyatta.com> wrote:

> On Sat, 06 Feb 2010 10:22:59 +0900
> Mike McCormack <mikem@ring3k.org> wrote:
> 
> > Allocating everything in one place means there's a single point
> > of failure in sky2_up, and sky2_rx_start can no longer fail.
> 
> If ring is never allocated, how then it must fail in up.
> Plus if the initial ring allocation is partial it should fail.

Let me put that clearer...
When dev_open is called, the system might be very short of memory
and unable to allocate the number of receive buffers; in that case,
I would prefer that an error was returned to the application.
Yes, this is a corner case; but it is better to fail with a noisy
error than limp along with a dead device.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike McCormack Feb. 7, 2010, 9:56 a.m. UTC | #3 
On 6 February 2010 15:12, Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Fri, 5 Feb 2010 18:10:45 -0800
> Stephen Hemminger <shemminger@vyatta.com> wrote:
>
>> On Sat, 06 Feb 2010 10:22:59 +0900
>> Mike McCormack <mikem@ring3k.org> wrote:
>>
>> > Allocating everything in one place means there's a single point
>> > of failure in sky2_up, and sky2_rx_start can no longer fail.
>>
>> If ring is never allocated, how then it must fail in up.
>> Plus if the initial ring allocation is partial it should fail.
>
> Let me put that clearer...
> When dev_open is called, the system might be very short of memory
> and unable to allocate the number of receive buffers; in that case,
> I would prefer that an error was returned to the application.
> Yes, this is a corner case; but it is better to fail with a noisy
> error than limp along with a dead device.

Hi Stephen,

I think you've misread my patch.  I have not attempted to change the
way allocation failure is handled, just to move all allocations to the
same place.

The end goal is to refactor sky2_up into a piece that initializes
hardware following memory allocation (say sky2_start) so we can do
sky2_reset() without free'ing and allocating memory, or detaching the
device.

thanks,

Mike
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index 40c661b..ee091d1 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -1379,8 +1379,34 @@  static inline void sky2_rx_update(struct sky2_port *sky2, unsigned rxq)
 	sky2_put_idx(sky2->hw, rxq, sky2->rx_put);
 }
 
+static int sky2_alloc_rx_skbs(struct sky2_port *sky2)
+{
+	struct sky2_hw *hw = sky2->hw;
+	unsigned i;
+
+	sky2->rx_data_size = sky2_get_rx_data_size(sky2);
+
+	/* Fill Rx ring */
+	for (i = 0; i < sky2->rx_pending; i++) {
+		struct rx_ring_info *re = sky2->rx_ring + i;
+
+		re->skb = sky2_rx_alloc(sky2);
+		if (!re->skb)
+			goto nomem;
+
+		if (sky2_rx_map_skb(hw->pdev, re, sky2->rx_data_size)) {
+			dev_kfree_skb(re->skb);
+			re->skb = NULL;
+			goto nomem;
+		}
+	}
+	return 0;
+nomem:
+	return -ENOMEM;
+}
+
 /*
- * Allocate and setup receiver buffer pool.
+ * Setup receiver buffer pool.
  * Normal case this ends up creating one list element for skb
  * in the receive ring. Worst case if using large MTU and each
  * allocation falls on a different 64 bit region, that results
@@ -1388,7 +1414,7 @@  static inline void sky2_rx_update(struct sky2_port *sky2, unsigned rxq)
  * One element is used for checksum enable/disable, and one
  * extra to avoid wrap.
  */
-static int sky2_rx_start(struct sky2_port *sky2)
+static void sky2_rx_start(struct sky2_port *sky2)
 {
 	struct sky2_hw *hw = sky2->hw;
 	struct rx_ring_info *re;
@@ -1414,22 +1440,9 @@  static int sky2_rx_start(struct sky2_port *sky2)
 	if (!(hw->flags & SKY2_HW_NEW_LE))
 		rx_set_checksum(sky2);
 
-	sky2->rx_data_size = sky2_get_rx_data_size(sky2);
-
-	/* Fill Rx ring */
+	/* submit Rx ring */
 	for (i = 0; i < sky2->rx_pending; i++) {
 		re = sky2->rx_ring + i;
-
-		re->skb = sky2_rx_alloc(sky2);
-		if (!re->skb)
-			goto nomem;
-
-		if (sky2_rx_map_skb(hw->pdev, re, sky2->rx_data_size)) {
-			dev_kfree_skb(re->skb);
-			re->skb = NULL;
-			goto nomem;
-		}
-
 		sky2_rx_submit(sky2, re);
 	}
 
@@ -1471,13 +1484,6 @@  static int sky2_rx_start(struct sky2_port *sky2)
 		sky2_write32(hw, Q_ADDR(txqaddr[sky2->port], Q_TEST),
 			     TBMU_TEST_HOME_ADD_FIX_EN | TBMU_TEST_ROUTING_ADD_FIX_EN);
 	}
-
-
-
-	return 0;
-nomem:
-	sky2_rx_clean(sky2);
-	return -ENOMEM;
 }
 
 static int sky2_alloc_buffers(struct sky2_port *sky2)
@@ -1508,7 +1514,7 @@  static int sky2_alloc_buffers(struct sky2_port *sky2)
 	if (!sky2->rx_ring)
 		goto nomem;
 
-	return 0;
+	return sky2_alloc_rx_skbs(sky2);
 nomem:
 	return -ENOMEM;
 }
@@ -1517,6 +1523,8 @@  static void sky2_free_buffers(struct sky2_port *sky2)
 {
 	struct sky2_hw *hw = sky2->hw;
 
+	sky2_rx_clean(sky2);
+
 	if (sky2->rx_le) {
 		pci_free_consistent(hw->pdev, RX_LE_BYTES,
 				    sky2->rx_le, sky2->rx_le_map);
@@ -1606,9 +1614,7 @@  static int sky2_up(struct net_device *dev)
 	sky2_set_vlan_mode(hw, port, sky2->vlgrp != NULL);
 #endif
 
-	err = sky2_rx_start(sky2);
-	if (err)
-		goto err_out;
+	sky2_rx_start(sky2);
 
 	/* Enable interrupts from phy/mac for port */
 	imask = sky2_read32(hw, B0_IMSK);
@@ -1976,8 +1982,6 @@  static int sky2_down(struct net_device *dev)
 	/* Free any pending frames stuck in HW queue */
 	sky2_tx_complete(sky2, sky2->tx_prod);
 
-	sky2_rx_clean(sky2);
-
 	sky2_free_buffers(sky2);
 
 	return 0;
@@ -2267,7 +2271,11 @@  static int sky2_change_mtu(struct net_device *dev, int new_mtu)
 
 	sky2_write8(hw, RB_ADDR(rxqaddr[port], RB_CTRL), RB_ENA_OP_MD);
 
-	err = sky2_rx_start(sky2);
+	err = sky2_alloc_rx_skbs(sky2);
+	if (!err)
+		sky2_rx_start(sky2);
+	else
+		sky2_rx_clean(sky2);
 	sky2_write32(hw, B0_IMSK, imask);
 
 	sky2_read32(hw, B0_Y2_SP_LISR);