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

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

[Mike McCormack]

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(-)

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);
-- 
1.5.6.5

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

[Stephen Hemminger]

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.

-- 

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

[Stephen Hemminger]

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.

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

[Mike McCormack]

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