sky2 patches for net-next

PATCH In review Stephen Hemminger 4 participants 22 messages Nov 16, 2011 – Nov 18, 2011

Subsystems (3)

Networking Network drivers Marvell gigabit Ethernet drivers (skge/sky2)

lore

Share & tools

lore

Stephen Hemminger

author

2011-11-16 23:52 UTC #0 lore raw

cover sky2 patches for net-next

This includes fix for recent regression and other bug fixes (in order of decreasing importance). Only the first one is critical to get into 3.2.

Stephen Hemminger

author

2011-11-16 23:52 UTC #1 lore raw

PATCH 1/6 sky2: fix hang on shutdown (and other irq issues)

There are several problems with recent change to how IRQ's are setup.
* synchronize_irq in sky2_shutdown would hang because there was no IRQ setup.
* when device was set to down, some IRQ bits left enabled so a hardware error would produce IRQ with no handler
* quick link on Optima chip set was enabled without handler
* suspend/resume would leave IRQ on with no handler if device was down

Signed-off-by Stephen Hemminger <[email protected]>

---
This needs to be applied to net-next and -net

drivers/net/ethernet/marvell/sky2.c +27 ~~-18~~

@@ -1747,6 +1747,11 @@ static int sky2_up(struct net_device *de
 
 	sky2_hw_up(sky2);
 
+	if (hw->chip_id == CHIP_ID_YUKON_OPT ||
+	    hw->chip_id == CHIP_ID_YUKON_PRM ||
+	    hw->chip_id == CHIP_ID_YUKON_OP_2)
+		imask |= Y2_IS_PHY_QLNK;	/* enable PHY Quick Link */
+
 	/* Enable interrupts from phy/mac for port */
 	imask = sky2_read32(hw, B0_IMSK);
 	imask |= portirq_msk[port];

@@ -2101,15 +2106,21 @@ static int sky2_down(struct net_device *
 
 	netif_info(sky2, ifdown, dev, "disabling interface\n");
 
-	/* Disable port IRQ */
-	sky2_write32(hw, B0_IMSK,
-		     sky2_read32(hw, B0_IMSK) & ~portirq_msk[sky2->port]);
-	sky2_read32(hw, B0_IMSK);
-
 	if (hw->ports == 1) {
+		sky2_write32(hw, B0_IMSK, 0);
+		sky2_read32(hw, B0_IMSK);
+
 		napi_disable(&hw->napi);
 		free_irq(hw->pdev->irq, hw);
 	} else {
+		u32 imask;
+
+		/* Disable port IRQ */
+		imask  = sky2_read32(hw, B0_IMSK);
+		imask &= ~portirq_msk[sky2->port];
+		sky2_write32(hw, B0_IMSK, imask);
+		sky2_read32(hw, B0_IMSK);
+
 		synchronize_irq(hw->pdev->irq);
 		napi_synchronize(&hw->napi);
 	}

@@ -3258,7 +3269,6 @@ static void sky2_reset(struct sky2_hw *h
 	    hw->chip_id == CHIP_ID_YUKON_PRM ||
 	    hw->chip_id == CHIP_ID_YUKON_OP_2) {
 		u16 reg;
-		u32 msk;
 
 		if (hw->chip_id == CHIP_ID_YUKON_OPT && hw->chip_rev == 0) {
 			/* disable PCI-E PHY power down (set PHY reg 0x80, bit 7 */

@@ -3281,11 +3291,6 @@ static void sky2_reset(struct sky2_hw *h
 		sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
 		sky2_pci_write16(hw, PSM_CONFIG_REG4, reg);
 
-		/* enable PHY Quick Link */
-		msk = sky2_read32(hw, B0_IMSK);
-		msk |= Y2_IS_PHY_QLNK;
-		sky2_write32(hw, B0_IMSK, msk);
-
 		/* check if PSMv2 was running before */
 		reg = sky2_pci_read16(hw, PSM_CONFIG_REG3);
 		if (reg & PCI_EXP_LNKCTL_ASPMC)

@@ -3412,7 +3417,9 @@ static void sky2_all_down(struct sky2_hw
 
 	sky2_read32(hw, B0_IMSK);
 	sky2_write32(hw, B0_IMSK, 0);
-	synchronize_irq(hw->pdev->irq);
+
+	if (hw->ports > 1 || netif_running(hw->dev[0]))
+		synchronize_irq(hw->pdev->irq);
 	napi_disable(&hw->napi);
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3430,7 +3437,7 @@ static void sky2_all_down(struct sky2_hw
 
 static void sky2_all_up(struct sky2_hw *hw)
 {
-	u32 imask = Y2_IS_BASE;
+	u32 imask = 0;
 	int i;
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3446,11 +3453,13 @@ static void sky2_all_up(struct sky2_hw *
 		netif_wake_queue(dev);
 	}
 
-	sky2_write32(hw, B0_IMSK, imask);
-	sky2_read32(hw, B0_IMSK);
-
-	sky2_read32(hw, B0_Y2_SP_LISR);
-	napi_enable(&hw->napi);
+	if (imask || hw->ports > 1) {
+		imask |= Y2_IS_BASE;
+		sky2_write32(hw, B0_IMSK, imask);
+		sky2_read32(hw, B0_IMSK);
+		sky2_read32(hw, B0_Y2_SP_LISR);
+		napi_enable(&hw->napi);
+	}
 }
 
 static void sky2_restart(struct work_struct *work)

Sven Joachim

2011-11-17 08:46 UTC #2 lore raw

Re: [PATCH 1/6] sky2: fix hang on shutdown (and other irq issues)

Quoted 8 lines · Stephen Hemminger

There are several problems with recent change to how IRQ's are setup.
* synchronize_irq in sky2_shutdown would hang because there was no IRQ setup.
* when device was set to down, some IRQ bits left enabled so a hardware error would produce IRQ with no handler
* quick link on Optima chip set was enabled without handler
* suspend/resume would leave IRQ on with no handler if device was down

Unfortunately, this patch does not fix the hang at shutdown for me. :-(

Quoted 105 lines

Signed-off-by Stephen Hemminger <[email protected]>

---
This needs to be applied to net-next and -net

drivers/net/ethernet/marvell/sky2.c +27 ~~-18~~

@@ -1747,6 +1747,11 @@ static int sky2_up(struct net_device *de
 
 	sky2_hw_up(sky2);
 
+	if (hw->chip_id == CHIP_ID_YUKON_OPT ||
+	    hw->chip_id == CHIP_ID_YUKON_PRM ||
+	    hw->chip_id == CHIP_ID_YUKON_OP_2)
+		imask |= Y2_IS_PHY_QLNK;	/* enable PHY Quick Link */
+
 	/* Enable interrupts from phy/mac for port */
 	imask = sky2_read32(hw, B0_IMSK);
 	imask |= portirq_msk[port];

@@ -2101,15 +2106,21 @@ static int sky2_down(struct net_device *
 
 	netif_info(sky2, ifdown, dev, "disabling interface\n");
 
-	/* Disable port IRQ */
-	sky2_write32(hw, B0_IMSK,
-		     sky2_read32(hw, B0_IMSK) & ~portirq_msk[sky2->port]);
-	sky2_read32(hw, B0_IMSK);
-
 	if (hw->ports == 1) {
+		sky2_write32(hw, B0_IMSK, 0);
+		sky2_read32(hw, B0_IMSK);
+
 		napi_disable(&hw->napi);
 		free_irq(hw->pdev->irq, hw);
 	} else {
+		u32 imask;
+
+		/* Disable port IRQ */
+		imask  = sky2_read32(hw, B0_IMSK);
+		imask &= ~portirq_msk[sky2->port];
+		sky2_write32(hw, B0_IMSK, imask);
+		sky2_read32(hw, B0_IMSK);
+
 		synchronize_irq(hw->pdev->irq);
 		napi_synchronize(&hw->napi);
 	}

@@ -3258,7 +3269,6 @@ static void sky2_reset(struct sky2_hw *h
 	    hw->chip_id == CHIP_ID_YUKON_PRM ||
 	    hw->chip_id == CHIP_ID_YUKON_OP_2) {
 		u16 reg;
-		u32 msk;
 
 		if (hw->chip_id == CHIP_ID_YUKON_OPT && hw->chip_rev == 0) {
 			/* disable PCI-E PHY power down (set PHY reg 0x80, bit 7 */

@@ -3281,11 +3291,6 @@ static void sky2_reset(struct sky2_hw *h
 		sky2_write8(hw, B2_TST_CTRL1, TST_CFG_WRITE_ON);
 		sky2_pci_write16(hw, PSM_CONFIG_REG4, reg);
 
-		/* enable PHY Quick Link */
-		msk = sky2_read32(hw, B0_IMSK);
-		msk |= Y2_IS_PHY_QLNK;
-		sky2_write32(hw, B0_IMSK, msk);
-
 		/* check if PSMv2 was running before */
 		reg = sky2_pci_read16(hw, PSM_CONFIG_REG3);
 		if (reg & PCI_EXP_LNKCTL_ASPMC)

@@ -3412,7 +3417,9 @@ static void sky2_all_down(struct sky2_hw
 
 	sky2_read32(hw, B0_IMSK);
 	sky2_write32(hw, B0_IMSK, 0);
-	synchronize_irq(hw->pdev->irq);
+
+	if (hw->ports > 1 || netif_running(hw->dev[0]))
+		synchronize_irq(hw->pdev->irq);
 	napi_disable(&hw->napi);
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3430,7 +3437,7 @@ static void sky2_all_down(struct sky2_hw
 
 static void sky2_all_up(struct sky2_hw *hw)
 {
-	u32 imask = Y2_IS_BASE;
+	u32 imask = 0;
 	int i;
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3446,11 +3453,13 @@ static void sky2_all_up(struct sky2_hw *
 		netif_wake_queue(dev);
 	}
 
-	sky2_write32(hw, B0_IMSK, imask);
-	sky2_read32(hw, B0_IMSK);
-
-	sky2_read32(hw, B0_Y2_SP_LISR);
-	napi_enable(&hw->napi);
+	if (imask || hw->ports > 1) {
+		imask |= Y2_IS_BASE;
+		sky2_write32(hw, B0_IMSK, imask);
+		sky2_read32(hw, B0_IMSK);
+		sky2_read32(hw, B0_Y2_SP_LISR);
+		napi_enable(&hw->napi);
+	}
 }
 
 static void sky2_restart(struct work_struct *work)

Stephen Hemminger

author

2011-11-17 17:01 UTC #3 lore raw

Re: [PATCH 1/6] sky2: fix hang on shutdown (and other irq issues)

Quoted 11 lines · On Thu, 17 Nov 2011 09:46:43 +0100 Sven Joachim <[email protected]> wrote:

Quoted 8 lines · Stephen Hemminger
There are several problems with recent change to how IRQ's are setup.
* synchronize_irq in sky2_shutdown would hang because there was no IRQ setup.
* when device was set to down, some IRQ bits left enabled so a hardware error would produce IRQ with no handler
* quick link on Optima chip set was enabled without handler
* suspend/resume would leave IRQ on with no handler if device was down

Quoted 1 line

Unfortunately, this patch does not fix the hang at shutdown for me. :-(

You are probably stuck in some part of the code path in sky2_shutdown. The best debug for this is sprinkling the code printk's. That is how I narrowed the problem down to synchronize_irq.

Sven Joachim

2011-11-17 18:38 UTC #4 lore raw

Re: [PATCH 1/6] sky2: fix hang on shutdown (and other irq issues)

Quoted 14 lines · Stephen Hemminger

Quoted 11 lines · On Thu, 17 Nov 2011 09:46:43 +0100 Sven Joachim <[email protected]> wrote:
Quoted 8 lines · Stephen Hemminger
There are several problems with recent change to how IRQ's are setup.
* synchronize_irq in sky2_shutdown would hang because there was no IRQ setup.
* when device was set to down, some IRQ bits left enabled so a hardware error would produce IRQ with no handler
* quick link on Optima chip set was enabled without handler
* suspend/resume would leave IRQ on with no handler if device was down

Quoted 3 lines

Quoted 1 line
Unfortunately, this patch does not fix the hang at shutdown for me. :-(
You are probably stuck in some part of the code path in sky2_shutdown.

Indeed.

Quoted 2 lines

The best debug for this is sprinkling the code printk's. That is how I narrowed the problem down to synchronize_irq.

So I found out it's hanging in napi_disable() in sky2_all_down(). Not being a kernel hacker, I don't know where to go from there.

Cheers,
Sven

Stephen Hemminger

author

2011-11-17 19:18 UTC #5 lore raw

Re: [PATCH 1/6] sky2: fix hang on shutdown (and other irq issues)

Does this fix it?

drivers/net/ethernet/marvell/sky2.c +10 -7

@@ -1723,6 +1723,8 @@ static int sky2_setup_irq(struct sky2_hw
 	if (err)
 		dev_err(&pdev->dev, "cannot assign irq %d\n", pdev->irq);
 	else {
+		hw->flags |= SKY2_HW_IRQ_SETUP;
+
 		napi_enable(&hw->napi);
 		sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 		sky2_read32(hw, B0_IMSK);

@@ -2120,6 +2122,7 @@ static int sky2_close(struct net_device
 
 		napi_disable(&hw->napi);
 		free_irq(hw->pdev->irq, hw);
+		hw->flags &= ~SKY2_HW_IRQ_SETUP;
 	} else {
 		u32 imask;

@@ -3423,12 +3426,13 @@ static void sky2_all_down(struct sky2_hw
 {
 	int i;
 
-	sky2_read32(hw, B0_IMSK);
-	sky2_write32(hw, B0_IMSK, 0);
+	if (hw->flags & SKY2_HW_IRQ_SETUP) {
+		sky2_read32(hw, B0_IMSK);
+		sky2_write32(hw, B0_IMSK, 0);
 
-	if (hw->ports > 1 || netif_running(hw->dev[0]))
 		synchronize_irq(hw->pdev->irq);
-	napi_disable(&hw->napi);
+		napi_disable(&hw->napi);
+	}
 
 	for (i = 0; i < hw->ports; i++) {
 		struct net_device *dev = hw->dev[i];

@@ -3445,7 +3449,7 @@ static void sky2_all_down(struct sky2_hw
 
 static void sky2_all_up(struct sky2_hw *hw)
 {
-	u32 imask = 0;
+	u32 imask = Y2_IS_BASE;
 	int i;
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3461,8 +3465,7 @@ static void sky2_all_up(struct sky2_hw *
 		netif_wake_queue(dev);
 	}
 
-	if (imask || hw->ports > 1) {
-		imask |= Y2_IS_BASE;
+	if (hw->flags & SKY2_HW_IRQ_SETUP) {
 		sky2_write32(hw, B0_IMSK, imask);
 		sky2_read32(hw, B0_IMSK);
 		sky2_read32(hw, B0_Y2_SP_LISR);

drivers/net/ethernet/marvell/sky2.h +1 -0

@@ -2287,6 +2287,7 @@ struct sky2_hw {
 #define SKY2_HW_RSS_BROKEN	0x00000100
 #define SKY2_HW_VLAN_BROKEN     0x00000200
 #define SKY2_HW_RSS_CHKSUM	0x00000400	/* RSS requires chksum */
+#define SKY2_HW_IRQ_SETUP	0x00000800
 
 	u8	     	     chip_id;
 	u8		     chip_rev;

Sven Joachim

2011-11-17 20:57 UTC #6 lore raw

Re: [PATCH 1/6] sky2: fix hang on shutdown (and other irq issues)

Quoted 1 line · Stephen Hemminger

Does this fix it?

Yes, thank you. However, I had a more serious problem unrelated to that patch; more on that in another mail.

Quoted 66 lines

drivers/net/ethernet/marvell/sky2.c +10 -7

@@ -1723,6 +1723,8 @@ static int sky2_setup_irq(struct sky2_hw
 	if (err)
 		dev_err(&pdev->dev, "cannot assign irq %d\n", pdev->irq);
 	else {
+		hw->flags |= SKY2_HW_IRQ_SETUP;
+
 		napi_enable(&hw->napi);
 		sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 		sky2_read32(hw, B0_IMSK);

@@ -2120,6 +2122,7 @@ static int sky2_close(struct net_device
 
 		napi_disable(&hw->napi);
 		free_irq(hw->pdev->irq, hw);
+		hw->flags &= ~SKY2_HW_IRQ_SETUP;
 	} else {
 		u32 imask;

@@ -3423,12 +3426,13 @@ static void sky2_all_down(struct sky2_hw
 {
 	int i;
 
-	sky2_read32(hw, B0_IMSK);
-	sky2_write32(hw, B0_IMSK, 0);
+	if (hw->flags & SKY2_HW_IRQ_SETUP) {
+		sky2_read32(hw, B0_IMSK);
+		sky2_write32(hw, B0_IMSK, 0);
 
-	if (hw->ports > 1 || netif_running(hw->dev[0]))
 		synchronize_irq(hw->pdev->irq);
-	napi_disable(&hw->napi);
+		napi_disable(&hw->napi);
+	}
 
 	for (i = 0; i < hw->ports; i++) {
 		struct net_device *dev = hw->dev[i];

@@ -3445,7 +3449,7 @@ static void sky2_all_down(struct sky2_hw
 
 static void sky2_all_up(struct sky2_hw *hw)
 {
-	u32 imask = 0;
+	u32 imask = Y2_IS_BASE;
 	int i;
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3461,8 +3465,7 @@ static void sky2_all_up(struct sky2_hw *
 		netif_wake_queue(dev);
 	}
 
-	if (imask || hw->ports > 1) {
-		imask |= Y2_IS_BASE;
+	if (hw->flags & SKY2_HW_IRQ_SETUP) {
 		sky2_write32(hw, B0_IMSK, imask);
 		sky2_read32(hw, B0_IMSK);
 		sky2_read32(hw, B0_Y2_SP_LISR);

drivers/net/ethernet/marvell/sky2.h +1 -0

@@ -2287,6 +2287,7 @@ struct sky2_hw {
 #define SKY2_HW_RSS_BROKEN	0x00000100
 #define SKY2_HW_VLAN_BROKEN     0x00000200
 #define SKY2_HW_RSS_CHKSUM	0x00000400	/* RSS requires chksum */
+#define SKY2_HW_IRQ_SETUP	0x00000800
 
 	u8	     	     chip_id;
 	u8		     chip_rev;

Stephen Hemminger

author

2011-11-18 00:37 UTC #7 lore raw

PATCH 1/1 sky2: fix hang in napi_disable

In reply to Sven Joachim

If IRQ was never initialized, then calling napi_disable() would hang. Add more bookkeeping to track whether IRQ was ever initialized.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c +10 -7

@@ -1723,6 +1723,8 @@ static int sky2_setup_irq(struct sky2_hw
 	if (err)
 		dev_err(&pdev->dev, "cannot assign irq %d\n", pdev->irq);
 	else {
+		hw->flags |= SKY2_HW_IRQ_SETUP;
+
 		napi_enable(&hw->napi);
 		sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
 		sky2_read32(hw, B0_IMSK);

@@ -2120,6 +2122,7 @@ static int sky2_close(struct net_device
 
 		napi_disable(&hw->napi);
 		free_irq(hw->pdev->irq, hw);
+		hw->flags &= ~SKY2_HW_IRQ_SETUP;
 	} else {
 		u32 imask;

@@ -3423,12 +3426,13 @@ static void sky2_all_down(struct sky2_hw
 {
 	int i;
 
-	sky2_read32(hw, B0_IMSK);
-	sky2_write32(hw, B0_IMSK, 0);
+	if (hw->flags & SKY2_HW_IRQ_SETUP) {
+		sky2_read32(hw, B0_IMSK);
+		sky2_write32(hw, B0_IMSK, 0);
 
-	if (hw->ports > 1 || netif_running(hw->dev[0]))
 		synchronize_irq(hw->pdev->irq);
-	napi_disable(&hw->napi);
+		napi_disable(&hw->napi);
+	}
 
 	for (i = 0; i < hw->ports; i++) {
 		struct net_device *dev = hw->dev[i];

@@ -3445,7 +3449,7 @@ static void sky2_all_down(struct sky2_hw
 
 static void sky2_all_up(struct sky2_hw *hw)
 {
-	u32 imask = 0;
+	u32 imask = Y2_IS_BASE;
 	int i;
 
 	for (i = 0; i < hw->ports; i++) {

@@ -3461,8 +3465,7 @@ static void sky2_all_up(struct sky2_hw *
 		netif_wake_queue(dev);
 	}
 
-	if (imask || hw->ports > 1) {
-		imask |= Y2_IS_BASE;
+	if (hw->flags & SKY2_HW_IRQ_SETUP) {
 		sky2_write32(hw, B0_IMSK, imask);
 		sky2_read32(hw, B0_IMSK);
 		sky2_read32(hw, B0_Y2_SP_LISR);

drivers/net/ethernet/marvell/sky2.h +1 -0

@@ -2287,6 +2287,7 @@ struct sky2_hw {
 #define SKY2_HW_RSS_BROKEN	0x00000100
 #define SKY2_HW_VLAN_BROKEN     0x00000200
 #define SKY2_HW_RSS_CHKSUM	0x00000400	/* RSS requires chksum */
+#define SKY2_HW_IRQ_SETUP	0x00000800
 
 	u8	     	     chip_id;
 	u8		     chip_rev;

David Miller

maintainer

2011-11-18 01:52 UTC #8 lore raw

Re: [PATCH net-next] sky2: fix hang in napi_disable

From: Stephen Hemminger <[email protected]>
Date: Thu, 17 Nov 2011 16:37:35 -0800

Quoted 4 lines

If IRQ was never initialized, then calling napi_disable() would hang. Add more bookkeeping to track whether IRQ was ever initialized.
Signed-off-by Stephen Hemminger <[email protected]>

Stephen, your other 6 patches went into 'net' so you have to target these fixes there too.

Stephen Hemminger

2011-11-18 02:10 UTC #9 lore raw

Re: [PATCH net-next] sky2: fix hang in napi_disable

Quoted 4 lines

From: Stephen Hemminger <[email protected]>
Date: Thu, 17 Nov 2011 16:37:35 -0800

Quoted 9 lines

Quoted 5 lines
If IRQ was never initialized, then calling napi_disable() would hang.
Add more bookkeeping to track whether IRQ was ever initialized.
Signed-off-by Stephen Hemminger <[email protected]>
Stephen, your other 6 patches went into 'net' so you have to target these fixes there too.

Do you need me to respin them?

David Miller

maintainer

2011-11-18 02:15 UTC #10 lore raw

Re: [PATCH net-next] sky2: fix hang in napi_disable

From: Stephen Hemminger <[email protected]>
Date: Thu, 17 Nov 2011 18:10:49 -0800 (PST)

Quoted 8 lines

Quoted 8 lines
From: Stephen Hemminger <[email protected]>
Date: Thu, 17 Nov 2011 16:37:35 -0800
Quoted 4 lines
If IRQ was never initialized, then calling napi_disable() would hang.
Add more bookkeeping to track whether IRQ was ever initialized.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Stephen Hemminger <[email protected]>
Stephen, your other 6 patches went into 'net' so you have to target these fixes there too.
Do you need me to respin them?

If they apply cleanly to 'net', no. Do they?

David Miller

maintainer Applied

2011-11-18 02:44 UTC #11 lore raw

Re: [PATCH net-next] sky2: fix hang in napi_disable

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Thu, 17 Nov 2011 16:37:35 -0800

Quoted 4 lines

If IRQ was never initialized, then calling napi_disable() would hang. Add more bookkeeping to track whether IRQ was ever initialized.
Signed-off-by Stephen Hemminger <[email protected]>

Applied to 'net'.

Stephen Hemminger

author

2011-11-16 23:52 UTC #12 lore raw

PATCH 2/6 sky2: pci posting issues

In reply to Stephen Hemminger

A couple of the reset and setup paths have possible PCI posting issues. When setting registers, a read is necessary to force the writes to complete.

Signed-off-by Stephen Hemminger <[email protected]>

---
This needs to be applied to net-next and -net

drivers/net/ethernet/marvell/sky2.c +3 -0

@@ -869,6 +869,7 @@ static void sky2_wol_init(struct sky2_po
 
 	/* block receiver */
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
+	sky2_read32(hw, B0_CTST);
 }
 
 static void sky2_set_tx_stfwd(struct sky2_hw *hw, unsigned port)

@@ -2045,6 +2046,8 @@ static void sky2_tx_reset(struct sky2_hw
 
 	sky2_write32(hw, RB_ADDR(txqaddr[port], RB_CTRL), RB_RST_SET);
 	sky2_write8(hw, SK_REG(port, TX_GMF_CTRL_T), GMF_RST_SET);
+
+	sky2_read32(hw, B0_CTST);
 }
 
 static void sky2_hw_down(struct sky2_port *sky2)

Stephen Hemminger

author

2011-11-16 23:52 UTC #13 lore raw

PATCH 3/6 sky2: rename up/down functions

In reply to Stephen Hemminger

The code is clearer if the up/down functions are renamed to open/close like other drivers. Purely syntax change.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c +10 ~~-10~~

@@ -1728,7 +1728,7 @@ static int sky2_setup_irq(struct sky2_hw
 
 
 /* Bring up network interface. */
-static int sky2_up(struct net_device *dev)
+static int sky2_open(struct net_device *dev)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 	struct sky2_hw *hw = sky2->hw;

@@ -2098,7 +2098,7 @@ static void sky2_hw_down(struct sky2_por
 }
 
 /* Network shutdown */
-static int sky2_down(struct net_device *dev)
+static int sky2_close(struct net_device *dev)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
 	struct sky2_hw *hw = sky2->hw;

@@ -2601,7 +2601,7 @@ static inline void sky2_tx_done(struct n
 	if (netif_running(dev)) {
 		sky2_tx_complete(sky2, last);
 
-		/* Wake unless it's detached, and called e.g. from sky2_down() */
+		/* Wake unless it's detached, and called e.g. from sky2_close() */
 		if (tx_avail(sky2) > MAX_SKB_TX_LE + 4)
 			netif_wake_queue(dev);
 	}

@@ -3391,7 +3391,7 @@ static void sky2_detach(struct net_devic
 		netif_tx_lock(dev);
 		netif_device_detach(dev);	/* stop txq */
 		netif_tx_unlock(dev);
-		sky2_down(dev);
+		sky2_close(dev);
 	}
 }

@@ -3401,7 +3401,7 @@ static int sky2_reattach(struct net_devi
 	int err = 0;
 
 	if (netif_running(dev)) {
-		err = sky2_up(dev);
+		err = sky2_open(dev);
 		if (err) {
 			netdev_info(dev, "could not restart %d\n", err);
 			dev_close(dev);

@@ -4570,7 +4570,7 @@ static int sky2_device_event(struct noti
 	struct net_device *dev = ptr;
 	struct sky2_port *sky2 = netdev_priv(dev);
 
-	if (dev->netdev_ops->ndo_open != sky2_up || !sky2_debug)
+	if (dev->netdev_ops->ndo_open != sky2_open || !sky2_debug)
 		return NOTIFY_DONE;
 
 	switch (event) {

@@ -4635,8 +4635,8 @@ static __exit void sky2_debug_cleanup(vo
    not allowing netpoll on second port */
 static const struct net_device_ops sky2_netdev_ops[2] = {
   {
-	.ndo_open		= sky2_up,
-	.ndo_stop		= sky2_down,
+	.ndo_open		= sky2_open,
+	.ndo_stop		= sky2_close,
 	.ndo_start_xmit		= sky2_xmit_frame,
 	.ndo_do_ioctl		= sky2_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,

@@ -4652,8 +4652,8 @@ static const struct net_device_ops sky2_
 #endif
   },
   {
-	.ndo_open		= sky2_up,
-	.ndo_stop		= sky2_down,
+	.ndo_open		= sky2_open,
+	.ndo_stop		= sky2_close,
 	.ndo_start_xmit		= sky2_xmit_frame,
 	.ndo_do_ioctl		= sky2_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,

Stephen Hemminger

author

2011-11-16 23:52 UTC #14 lore raw

PATCH 4/6 sky2: reduce default Tx ring size

In reply to Stephen Hemminger

The default Tx ring size for the sky2 driver is quite large and could cause excess buffer bloat for many users. The minimum ring size possible and still allow handling the worst case packet on 64bit platforms is 38 which gets rounded up to a power of 2. But most packets only require a couple of ring elements.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c +1 -1

@@ -68,7 +68,7 @@
 #define MAX_SKB_TX_LE	(2 + (sizeof(dma_addr_t)/sizeof(u32))*(MAX_SKB_FRAGS+1))
 #define TX_MIN_PENDING		(MAX_SKB_TX_LE+1)
 #define TX_MAX_PENDING		1024
-#define TX_DEF_PENDING		127
+#define TX_DEF_PENDING		63
 
 #define TX_WATCHDOG		(5 * HZ)
 #define NAPI_WEIGHT		64

Sven Joachim

2011-11-17 21:07 UTC #15 lore raw

Re: [PATCH 4/6] sky2: reduce default Tx ring size

Quoted 11 lines · Stephen Hemminger

The default Tx ring size for the sky2 driver is quite large and could cause excess buffer bloat for many users. The minimum ring size possible and still allow handling the worst case packet on 64bit platforms is 38 which gets rounded up to a power of 2. But most packets only require a couple of ring elements.
Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c line 74

Quoted 8 lines

drivers/net/ethernet/marvell/sky2.c +1 -1

@@ -68,7 +68,7 @@
 #define MAX_SKB_TX_LE	(2 + (sizeof(dma_addr_t)/sizeof(u32))*(MAX_SKB_FRAGS+1))
 #define TX_MIN_PENDING		(MAX_SKB_TX_LE+1)
 #define TX_MAX_PENDING		1024
-#define TX_DEF_PENDING		127
+#define TX_DEF_PENDING		63
 
 #define TX_WATCHDOG		(5 * HZ)
 #define NAPI_WEIGHT		64

It's hard to believe, but this innocuous-looking patch caused my system to crash and burn as soon as I actually used the network. Unfortunately, I have neither a digital camera nor a serial console at hand, but from my recollections and having read sky2.c I conclude that

	BUG_ON(done >= sky2->tx_ring_size);

in sky2_tx_complete() seems to have been triggered. Does this make any sense?

Cheers,
Sven

Stephen Hemminger

author

2011-11-17 22:41 UTC #16 lore raw

Re: [PATCH 4/6] sky2: reduce default Tx ring size

Quoted 27 lines · On Thu, 17 Nov 2011 22:07:25 +0100 Sven Joachim <[email protected]> wrote:

Quoted 11 lines · Stephen Hemminger
The default Tx ring size for the sky2 driver is quite large and could cause excess buffer bloat for many users. The minimum ring size possible and still allow handling the worst case packet on 64bit platforms is 38 which gets rounded up to a power of 2. But most packets only require a couple of ring elements.
Signed-off-by Stephen Hemminger <[email protected]>
drivers/net/ethernet/marvell/sky2.c line 74
Quoted 8 lines
drivers/net/ethernet/marvell/sky2.c +1 -1
@@ -68,7 +68,7 @@
 #define MAX_SKB_TX_LE	(2 + (sizeof(dma_addr_t)/sizeof(u32))*(MAX_SKB_FRAGS+1))
 #define TX_MIN_PENDING		(MAX_SKB_TX_LE+1)
 #define TX_MAX_PENDING		1024
-#define TX_DEF_PENDING		127
+#define TX_DEF_PENDING		63
 
 #define TX_WATCHDOG		(5 * HZ)
 #define NAPI_WEIGHT		64
It's hard to believe, but this innocuous-looking patch caused my system to crash and burn as soon as I actually used the network. Unfortunately, I have neither a digital camera nor a serial console at hand, but from my recollections and having read sky2.c I conclude that

Quoted 5 lines

	BUG_ON(done >= sky2->tx_ring_size);
in sky2_tx_complete() seems to have been triggered. Does this make any sense?

If that is triggering then something is really odd because that means the ring index went past the end of the ring and wasn't wrapped correctly.

Stephen Hemminger

author

2011-11-18 00:37 UTC #17 lore raw

PATCH 1/1 sky2: enforce minimum ring size

In reply to Sven Joachim

The hardware has a restriction that the minimum ring size possible is 128. The number of elements used is controlled by tx_pending and the overall number of elements in the ring tx_ring_size, therefore it is okay to limit the number of elements in use to a small value (63) but still provide a bigger ring.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c +12 -2

@@ -4092,6 +4092,16 @@ static int sky2_set_coalesce(struct net_
 	return 0;
 }
 
+/*
+ * Hardware is limited to min of 128 and max of 2048 for ring size
+ * and  rounded up to next power of two
+ * to avoid division in modulus calclation
+ */
+static unsigned long roundup_ring_size(unsigned long pending)
+{
+	return max(128ul, roundup_pow_of_two(pending+1));
+}
+
 static void sky2_get_ringparam(struct net_device *dev,
 			       struct ethtool_ringparam *ering)
 {

@@ -4119,7 +4129,7 @@ static int sky2_set_ringparam(struct net
 
 	sky2->rx_pending = ering->rx_pending;
 	sky2->tx_pending = ering->tx_pending;
-	sky2->tx_ring_size = roundup_pow_of_two(sky2->tx_pending+1);
+	sky2->tx_ring_size = roundup_ring_size(sky2->tx_pending);
 
 	return sky2_reattach(dev);
 }

@@ -4714,7 +4724,7 @@ static __devinit struct net_device *sky2
 	spin_lock_init(&sky2->phy_lock);
 
 	sky2->tx_pending = TX_DEF_PENDING;
-	sky2->tx_ring_size = roundup_pow_of_two(TX_DEF_PENDING+1);
+	sky2->tx_ring_size = roundup_ring_size(TX_DEF_PENDING);
 	sky2->rx_pending = RX_DEF_PENDING;
 
 	hw->dev[port] = dev;

David Miller

maintainer Applied

2011-11-18 02:44 UTC #18 lore raw

Re: [PATCH net-next] sky2: enforce minimum ring size

From: Stephen Hemminger <[email protected]>
Date: Thu, 17 Nov 2011 16:37:23 -0800

Quoted 7 lines

The hardware has a restriction that the minimum ring size possible is 128. The number of elements used is controlled by tx_pending and the overall number of elements in the ring tx_ring_size, therefore it is okay to limit the number of elements in use to a small value (63) but still provide a bigger ring.
Signed-off-by Stephen Hemminger <[email protected]>

Applied to 'net'.

Stephen Hemminger

author

2011-11-16 23:57 UTC #19 lore raw

PATCH 5/6 sky2: used fixed RSS key

In reply to Stephen Hemminger

Rather than generating a different RSS key on each boot, just use a predetermined value that will map same flow to same value on every device.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c +9 -4

@@ -1275,6 +1275,14 @@ static void rx_set_checksum(struct sky2_
 		     ? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
 }
 
+/*
+ * Fixed initial key as seed to RSS.
+ */
+static const uint32_t rss_init_key[10] = {
+	0x7c3351da, 0x51c5cf4e,	0x44adbdd1, 0xe8d38d18,	0x48897c43,
+	0xb1d60e7e, 0x6a3dd760, 0x01a2e453, 0x16f46f13, 0x1a0e7b30
+};
+
 /* Enable/disable receive hash calculation (RSS) */
 static void rx_set_rss(struct net_device *dev, netdev_features_t features)
 {

@@ -1290,12 +1298,9 @@ static void rx_set_rss(struct net_device
 
 	/* Program RSS initial values */
 	if (features & NETIF_F_RXHASH) {
-		u32 key[nkeys];
-
-		get_random_bytes(key, nkeys * sizeof(u32));
 		for (i = 0; i < nkeys; i++)
 			sky2_write32(hw, SK_REG(sky2->port, RSS_KEY + i * 4),
-				     key[i]);
+				     rss_init_key[i]);
 
 		/* Need to turn on (undocumented) flag to make hashing work  */
 		sky2_write32(hw, SK_REG(sky2->port, RX_GMF_CTRL_T),

Stephen Hemminger

author

2011-11-16 23:57 UTC #20 lore raw

PATCH 6/6 sky2: version 1.30

In reply to Stephen Hemminger

Update version number.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/net/ethernet/marvell/sky2.c +1 -1

@@ -50,7 +50,7 @@
 #include "sky2.h"
 
 #define DRV_NAME		"sky2"
-#define DRV_VERSION		"1.29"
+#define DRV_VERSION		"1.30"
 
 /*
  * The Yukon II chipset takes 64 bit command blocks (called list elements)

David Miller

maintainer

2011-11-17 01:36 UTC #21 lore raw

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Wed, 16 Nov 2011 15:42:54 -0800

Quoted 3 lines

This includes fix for recent regression and other bug fixes (in order of decreasing importance). Only the first one is critical to get into 3.2.

All applied to 'net', thanks.