[PATCH 0/6] sky2 patches for net-next

[PATCH 0/6] sky2 patches for net-next

[Stephen Hemminger]

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.

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

[Stephen Hemminger]

[-- Attachment #1: sky2-sync-irq-shutdown.patch --]
[-- Type: text/plain, Size: 3460 bytes --]

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 <shemminger@vyatta.com>

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

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:15:48.212513321 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:32.898508932 -0800
@@ -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)

[PATCH 2/6] sky2: pci posting issues

[Stephen Hemminger]

[-- Attachment #1: sky2-pci-posting-wol.patch --]
[-- Type: text/plain, Size: 937 bytes --]

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 <shemminger@vyatta.com>

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

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:32.898508932 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:38.282631277 -0800
@@ -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)

[PATCH 3/6] sky2: rename up/down functions

[Stephen Hemminger]

[-- Attachment #1: sky2-open-stop-rename.patch --]
[-- Type: text/plain, Size: 2639 bytes --]

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 <shemminger@vyatta.com>

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:38.282631277 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:39.518659262 -0800
@@ -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,

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

[Stephen Hemminger]

[-- Attachment #1: sky2-tx-debloat.patch --]
[-- Type: text/plain, Size: 814 bytes --]

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 <shemminger@vyatta.com>

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:39.518659262 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:40.990692544 -0800
@@ -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

[PATCH 5/6] sky2: used fixed RSS key

[Stephen Hemminger]

[-- Attachment #1: sky2-fixed-key.patch --]
[-- Type: text/plain, Size: 1294 bytes --]

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 <shemminger@vyatta.com>


--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:40.990692544 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:42.254721076 -0800
@@ -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),

[PATCH 6/6] sky2: version 1.30

[Stephen Hemminger]

[-- Attachment #1: sky2-1.30-version.patch --]
[-- Type: text/plain, Size: 444 bytes --]

Update version number.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:08:36.031104340 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:11:24.728913165 -0800
@@ -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)

Re: [PATCH 0/6] sky2 patches for net-next

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 16 Nov 2011 15:42:54 -0800

> 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.

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

[Sven Joachim]

On 2011-11-17 00:42 +0100, Stephen Hemminger wrote:

> 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. :-(

> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> ---
> This needs to be applied to net-next and -net
>
> --- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:15:48.212513321 -0800
> +++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:32.898508932 -0800
> @@ -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)

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

[Stephen Hemminger]

On Thu, 17 Nov 2011 09:46:43 +0100
Sven Joachim <svenjoac@gmx.de> wrote:

> On 2011-11-17 00:42 +0100, Stephen Hemminger wrote:
> 
> > 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. :-(

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.

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

[Sven Joachim]

On 2011-11-17 18:01 +0100, Stephen Hemminger wrote:

> On Thu, 17 Nov 2011 09:46:43 +0100
> Sven Joachim <svenjoac@gmx.de> wrote:
>
>> On 2011-11-17 00:42 +0100, Stephen Hemminger wrote:
>> 
>> > 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. :-(
>
> You are probably stuck in some part of the code path in sky2_shutdown.

Indeed.

> 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

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

[Stephen Hemminger]

Does this fix it?

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-17 11:10:35.938076778 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-17 11:15:49.527882932 -0800
@@ -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);
--- a/drivers/net/ethernet/marvell/sky2.h	2011-11-16 15:15:40.964280527 -0800
+++ b/drivers/net/ethernet/marvell/sky2.h	2011-11-17 11:13:00.154858718 -0800
@@ -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;

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

[Sven Joachim]

On 2011-11-17 20:18 +0100, Stephen Hemminger wrote:

> Does this fix it?

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

> --- a/drivers/net/ethernet/marvell/sky2.c	2011-11-17 11:10:35.938076778 -0800
> +++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-17 11:15:49.527882932 -0800
> @@ -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);
> --- a/drivers/net/ethernet/marvell/sky2.h	2011-11-16 15:15:40.964280527 -0800
> +++ b/drivers/net/ethernet/marvell/sky2.h	2011-11-17 11:13:00.154858718 -0800
> @@ -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;

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

[Sven Joachim]

On 2011-11-17 00:42 +0100, Stephen Hemminger wrote:

> 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 <shemminger@vyatta.com>
>
> --- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:39.518659262 -0800
> +++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:40.990692544 -0800
> @@ -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

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

[Stephen Hemminger]

On Thu, 17 Nov 2011 22:07:25 +0100
Sven Joachim <svenjoac@gmx.de> wrote:

> On 2011-11-17 00:42 +0100, Stephen Hemminger wrote:
> 
> > 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 <shemminger@vyatta.com>
> >
> > --- a/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:39.518659262 -0800
> > +++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-16 15:19:40.990692544 -0800
> > @@ -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?
> 

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.

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

[Stephen Hemminger]

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 <shemminger@vyatta.com>

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-17 16:25:40.079898621 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-17 16:33:19.088438709 -0800
@@ -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;

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

[Stephen Hemminger]

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 <shemminger@vyatta.com>

--- a/drivers/net/ethernet/marvell/sky2.c	2011-11-17 11:10:35.938076778 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2011-11-17 11:15:49.527882932 -0800
@@ -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);
--- a/drivers/net/ethernet/marvell/sky2.h	2011-11-16 15:15:40.964280527 -0800
+++ b/drivers/net/ethernet/marvell/sky2.h	2011-11-17 11:13:00.154858718 -0800
@@ -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;

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

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 17 Nov 2011 16:37:35 -0800

> 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 <shemminger@vyatta.com>

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

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

[Stephen Hemminger]

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 17 Nov 2011 16:37:35 -0800
> 
> > 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 <shemminger@vyatta.com>
> 
> Stephen, your other 6 patches went into 'net' so you have to target
> these fixes there too.
> 

Do you need me to respin them?

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

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Thu, 17 Nov 2011 18:10:49 -0800 (PST)

> 
>> From: Stephen Hemminger <shemminger@vyatta.com>
>> Date: Thu, 17 Nov 2011 16:37:35 -0800
>> 
>> > 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 <shemminger@vyatta.com>
>> 
>> 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?

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

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 17 Nov 2011 16:37:35 -0800

> 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 <shemminger@vyatta.com>

Applied to 'net'.

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

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 17 Nov 2011 16:37:23 -0800

> 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 <shemminger@vyatta.com>

Applied to 'net'.