sky2: in free_buffers() do proper checks before call of rx_clean()

sky2: in free_buffers() do proper checks before call of rx_clean()

[Lino Sanfilippo]

In sky2_open() free_buffers() is called in case that alloc_buffers() failed.
Here we do the check for sky2->rx_le too late, since we already called
rx_clean() which accessed rx_le. Furthermore we lack a check for sky2->rx_ring
which is also accessed by rx_clean().

With this patch both sky2->rx_le and sky2->rx_ring are checked before
alloc_buffers() is called.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 This applies against 3.3

 drivers/net/ethernet/marvell/sky2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 760c2b1..3e3eda6 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -1628,9 +1628,10 @@ static void sky2_free_buffers(struct sky2_port *sky2)
 {
 	struct sky2_hw *hw = sky2->hw;
 
-	sky2_rx_clean(sky2);
-
 	if (sky2->rx_le) {
+		if (sky2->rx_ring)
+			sky2_rx_clean(sky2);
+
 		pci_free_consistent(hw->pdev, RX_LE_BYTES,
 				    sky2->rx_le, sky2->rx_le_map);
 		sky2->rx_le = NULL;
-- 
1.7.2.5

Re: sky2: in free_buffers() do proper checks before call of rx_clean()

[David Miller]

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: Tue, 3 Apr 2012 02:13:27 +0200

> In sky2_open() free_buffers() is called in case that alloc_buffers() failed.
> Here we do the check for sky2->rx_le too late, since we already called
> rx_clean() which accessed rx_le. Furthermore we lack a check for sky2->rx_ring
> which is also accessed by rx_clean().
> 
> With this patch both sky2->rx_le and sky2->rx_ring are checked before
> alloc_buffers() is called.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Ping?

Re: sky2: in free_buffers() do proper checks before call of rx_clean()

[David Miller]

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date: Tue, 3 Apr 2012 02:13:27 +0200

> In sky2_open() free_buffers() is called in case that alloc_buffers() failed.
> Here we do the check for sky2->rx_le too late, since we already called
> rx_clean() which accessed rx_le. Furthermore we lack a check for sky2->rx_ring
> which is also accessed by rx_clean().
> 
> With this patch both sky2->rx_le and sky2->rx_ring are checked before
> alloc_buffers() is called.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Stephen, I'm still waiting on your review of this patch.

Thanks.

Re: sky2: in free_buffers() do proper checks before call of rx_clean()

[Stephen Hemminger]

How about this instead? It moves the rx_clean out of free_buffers and
into the proper place in the unwind code.


--- a/drivers/net/ethernet/marvell/sky2.c	2012-04-12 08:07:11.572640522 -0700
+++ b/drivers/net/ethernet/marvell/sky2.c	2012-04-13 10:19:39.466839378 -0700
@@ -1630,7 +1630,8 @@ static int sky2_alloc_buffers(struct sky
 	if (!sky2->rx_ring)
 		goto nomem;
 
-	return sky2_alloc_rx_skbs(sky2);
+	return 0;
+
 nomem:
 	return -ENOMEM;
 }
@@ -1639,8 +1640,6 @@ static void sky2_free_buffers(struct sky
 {
 	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);
@@ -1761,9 +1760,13 @@ static int sky2_open(struct net_device *
 	if (err)
 		goto err_out;
 
+	err = sky2_alloc_rx_skbs(sky2);
+	if (err)
+		goto err_clean;
+
 	/* With single port, IRQ is setup when device is brought up */
 	if (hw->ports == 1 && (err = sky2_setup_irq(hw, dev->name)))
-		goto err_out;
+		goto err_clean;
 
 	sky2_hw_up(sky2);
 
@@ -1783,6 +1786,8 @@ static int sky2_open(struct net_device *
 
 	return 0;
 
+err_clean:
+	sky2_rx_clean(sky2);
 err_out:
 	sky2_free_buffers(sky2);
 	return err;
@@ -2158,6 +2163,7 @@ static int sky2_close(struct net_device
 
 	sky2_hw_down(sky2);
 
+	sky2_rx_clean(sky2);
 	sky2_free_buffers(sky2);
 
 	return 0;

Re: sky2: in free_buffers() do proper checks before call of rx_clean()

[Stephen Hemminger]

On Tue, 3 Apr 2012 02:13:27 +0200
Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote:

> In sky2_open() free_buffers() is called in case that alloc_buffers() failed.
> Here we do the check for sky2->rx_le too late, since we already called
> rx_clean() which accessed rx_le. Furthermore we lack a check for sky2->rx_ring
> which is also accessed by rx_clean().
> 
> With this patch both sky2->rx_le and sky2->rx_ring are checked before
> alloc_buffers() is called.
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>

Looking at sky2_alloc_buffers, it really needs to change to have
proper unwind instead. If some portion of the memory required isn't available
everything needs to be undone.