Netdev List
 help / color / mirror / Atom feed
* [PATCH] ethtool: Add reset operation
From: Ben Hutchings @ 2009-10-05 20:59 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, linux-net-drivers

After updating firmware stored in flash, users may wish to reset the
relevant hardware and start the new firmware immediately.  This should
not be completely automatic as it may be disruptive.

A selective reset may also be useful for debugging or diagnostics.

This adds a separate reset operation which takes flags indicating the
components to be reset.  Drivers are allowed to reset only a subset of
those requested, and must indicate the actual subset.  This allows the
use of generic component masks and some future expansion.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
This differs from the previous (RFC) version only in the semantics of
the output value of the reset flags: they should indicate the components
which were *not* reset.  This should be slightly less error-prone as it
means implementations do not need to maintain the input and output flags
separately.

Ben.

 include/linux/ethtool.h |   32 ++++++++++++++++++++++++++++++++
 net/core/ethtool.c      |   23 +++++++++++++++++++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 15e4eb7..0ef1306 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -502,6 +502,7 @@ struct ethtool_ops {
 	int	(*get_rxnfc)(struct net_device *, struct ethtool_rxnfc *, void *);
 	int	(*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
 	int     (*flash_device)(struct net_device *, struct ethtool_flash *);
+	int	(*reset)(struct net_device *, u32 *);
 };
 #endif /* __KERNEL__ */
 
@@ -559,6 +560,7 @@ struct ethtool_ops {
 #define	ETHTOOL_SRXCLSRLDEL	0x00000031 /* Delete RX classification rule */
 #define	ETHTOOL_SRXCLSRLINS	0x00000032 /* Insert RX classification rule */
 #define	ETHTOOL_FLASHDEV	0x00000033 /* Flash firmware to device */
+#define	ETHTOOL_RESET		0x00000034 /* Reset hardware */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -689,4 +691,34 @@ struct ethtool_ops {
 
 #define	RX_CLS_FLOW_DISC	0xffffffffffffffffULL
 
+/* Reset flags */
+/* The reset() operation must clear the flags for the components which
+ * were actually reset.  On successful return, the flags indicate the
+ * components which were not reset, either because they do not exist
+ * in the hardware or because they cannot be reset independently.  The
+ * driver must never reset any components that were not requested.
+ */
+enum ethtool_reset_flags {
+	/* These flags represent components dedicated to the interface
+	 * the command is addressed to.  Shift any flag left by
+	 * ETH_RESET_SHARED_SHIFT to reset a shared component of the
+	 * same type.
+	 */
+  	ETH_RESET_MGMT		= 1 << 0,	/* Management processor */
+	ETH_RESET_IRQ		= 1 << 1,	/* Interrupt requester */
+	ETH_RESET_DMA		= 1 << 2,	/* DMA engine */
+	ETH_RESET_FILTER	= 1 << 3,	/* Filtering/flow direction */
+	ETH_RESET_OFFLOAD	= 1 << 4,	/* Protocol offload */
+	ETH_RESET_MAC		= 1 << 5,	/* Media access controller */
+	ETH_RESET_PHY		= 1 << 6,	/* Transceiver/PHY */
+	ETH_RESET_RAM		= 1 << 7,	/* RAM shared between
+						 * multiple components */
+
+	ETH_RESET_DEDICATED	= 0x0000ffff,	/* All components dedicated to
+						 * this interface */
+	ETH_RESET_ALL		= 0xffffffff,	/* All components used by this
+						 * interface, even if shared */
+};
+#define ETH_RESET_SHARED_SHIFT	16
+
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 4c12ddb..6c7429c 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -309,6 +309,26 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
 	return ret;
 }
 
+static int ethtool_reset(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value reset;
+	int ret;
+
+	if (!dev->ethtool_ops->reset)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&reset, useraddr, sizeof(reset)))
+		return -EFAULT;
+
+	ret = dev->ethtool_ops->reset(dev, &reset.data);
+	if (ret)
+		return ret;
+
+	if (copy_to_user(useraddr, &reset, sizeof(reset)))
+		return -EFAULT;
+	return 0;
+}
+
 static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_wolinfo wol = { ETHTOOL_GWOL };
@@ -1127,6 +1147,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_FLASHDEV:
 		rc = ethtool_flash_device(dev, useraddr);
 		break;
+	case ETHTOOL_RESET:
+		rc = ethtool_reset(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply related

* [PATCH net-2.6] tg3: Fix phylib locking strategy
From: Matt Carlson @ 2009-10-05 21:09 UTC (permalink / raw)
  To: Felix Radensky; +Cc: netdev, Michael Chan, andy

Felix, can you verify this solves your problem?

---

Felix Radensky noted that chip resets were generating stack trace dumps.
This is because the driver is attempting to acquire the mdio bus mutex
while holding the tp->lock spinlock.  The fix is to change the code such
that every phy access takes the tp->lock spinlock instead.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
---
 drivers/net/tg3.c |   37 ++++++++++++-------------------------
 drivers/net/tg3.h |    1 -
 2 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index f09bc5d..8afc60e 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -902,11 +902,12 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
 	struct tg3 *tp = bp->priv;
 	u32 val;
 
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
-		return -EAGAIN;
+	spin_lock(&tp->lock);
 
 	if (tg3_readphy(tp, reg, &val))
-		return -EIO;
+		val = -EIO;
+
+	spin_unlock(&tp->lock);
 
 	return val;
 }
@@ -914,14 +915,16 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
 static int tg3_mdio_write(struct mii_bus *bp, int mii_id, int reg, u16 val)
 {
 	struct tg3 *tp = bp->priv;
+	u32 ret = 0;
 
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
-		return -EAGAIN;
+	spin_lock(&tp->lock);
 
 	if (tg3_writephy(tp, reg, val))
-		return -EIO;
+		ret = -EIO;
 
-	return 0;
+	spin_unlock(&tp->lock);
+
+	return ret;
 }
 
 static int tg3_mdio_reset(struct mii_bus *bp)
@@ -1011,12 +1014,6 @@ static void tg3_mdio_config_5785(struct tg3 *tp)
 
 static void tg3_mdio_start(struct tg3 *tp)
 {
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_INITED) {
-		mutex_lock(&tp->mdio_bus->mdio_lock);
-		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_PAUSED;
-		mutex_unlock(&tp->mdio_bus->mdio_lock);
-	}
-
 	tp->mi_mode &= ~MAC_MI_MODE_AUTO_POLL;
 	tw32_f(MAC_MI_MODE, tp->mi_mode);
 	udelay(80);
@@ -1041,15 +1038,6 @@ static void tg3_mdio_start(struct tg3 *tp)
 		tg3_mdio_config_5785(tp);
 }
 
-static void tg3_mdio_stop(struct tg3 *tp)
-{
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_INITED) {
-		mutex_lock(&tp->mdio_bus->mdio_lock);
-		tp->tg3_flags3 |= TG3_FLG3_MDIOBUS_PAUSED;
-		mutex_unlock(&tp->mdio_bus->mdio_lock);
-	}
-}
-
 static int tg3_mdio_init(struct tg3 *tp)
 {
 	int i;
@@ -1141,7 +1129,6 @@ static void tg3_mdio_fini(struct tg3 *tp)
 		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_INITED;
 		mdiobus_unregister(tp->mdio_bus);
 		mdiobus_free(tp->mdio_bus);
-		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_PAUSED;
 	}
 }
 
@@ -6392,8 +6379,6 @@ static int tg3_chip_reset(struct tg3 *tp)
 
 	tg3_nvram_lock(tp);
 
-	tg3_mdio_stop(tp);
-
 	tg3_ape_lock(tp, TG3_APE_LOCK_GRC);
 
 	/* No matching tg3_nvram_unlock() after this because
@@ -8698,6 +8683,8 @@ static int tg3_close(struct net_device *dev)
 
 	del_timer_sync(&tp->timer);
 
+	tg3_phy_stop(tp);
+
 	tg3_full_lock(tp, 1);
 #if 0
 	tg3_dump_state(tp);
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 524691c..bab7940 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2748,7 +2748,6 @@ struct tg3 {
 #define TG3_FLG3_5701_DMA_BUG		0x00000008
 #define TG3_FLG3_USE_PHYLIB		0x00000010
 #define TG3_FLG3_MDIOBUS_INITED		0x00000020
-#define TG3_FLG3_MDIOBUS_PAUSED		0x00000040
 #define TG3_FLG3_PHY_CONNECTED		0x00000080
 #define TG3_FLG3_RGMII_STD_IBND_DISABLE	0x00000100
 #define TG3_FLG3_RGMII_EXT_IBND_RX_EN	0x00000200
-- 
1.6.4.4



^ permalink raw reply related

* Re: [PATCH 21/21] drivers/net/tlan.h: Convert printk(KERN_DEBUG to pr_dbg(
From: Rafael J. Wysocki @ 2009-10-05 21:15 UTC (permalink / raw)
  To: Joe Perches; +Cc: David Miller, linux-kernel, chessman, netdev
In-Reply-To: <1254726974.1799.315.camel@Joe-Laptop.home>

On Monday 05 October 2009, Joe Perches wrote:
> On Mon, 2009-10-05 at 00:12 -0700, David Miller wrote:
> > From: Joe Perches <joe@perches.com>
> > Date: Sun,  4 Oct 2009 17:53:48 -0700
> > > Removed "TLAN: " prefix from debug printks, it's added by pr_fmt
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > Applied to net-next-2.6
> 
> Patches 20 and 21 depend on patch 1, which introduces pr_dbg
> to kernel.h.  Compile failure otherwise.

Why don't you push the first patch first, then, and wait with the others until
it gets merged?  This way individual subsystem maintainers will be able to
merge them cleanly.

Rafael

^ permalink raw reply

* Re: [PATCH RFC] isdn/capi: fix up CAPI subsystem workaround locking a bit
From: Michael Buesch @ 2009-10-05 21:24 UTC (permalink / raw)
  To: Tilman Schmidt
  Cc: i4ldeveloper, Carsten Paeth, Karsten Keil, Karsten Keil,
	Armin Schindler, isdn4linux, netdev, linux-kernel
In-Reply-To: <4AC9DBA5.1090701@imap.cc>

On Monday 05 October 2009 13:42:29 Tilman Schmidt wrote:
> On Sat, 2009-10-03 20:35:19 +0200, Michael Buesch wrote:
> 
> >> I remember that handle_minor_send() and/or handle_minor_recv() showed up
> >> in the crash backtraces. So if you move them out of the critical
> >> section, you can as well remove the lock completely.
> >
> > here's my original mail:
> > http://lkml.indiana.edu/hypermail/linux/kernel/0605.0/0455.html
> >
> > Note the patch in that mail does _not_ fix the issue, as it turned out later.
> > Then I did the workaround-lock patch, which _did_ fix it.
> 
> Thanks for the info. So do I understand correctly that after:
> 
> commit 6aa65472d18703064898eefb5eb58f7ecd0d8912
> Author: Michael Buesch <mb@bu3sch.de>
> Date:   Mon Jun 26 00:25:30 2006 -0700
> 
>     [PATCH] CAPI crash / race condition
> 
> you were actually still seeing LIST_POISON2 Oopses in
> capiminor_del_ack(), but after:

Yeah well. The oops with LIST_POISON was with a patch that converted the
datahandle_queue to struct list_head, but without the spinlock_t ackqlock added.
Then I added the spinlock_t ackqlock and it first seemed to fix the problem. (That
is the patch from the mail).
But it did only shrink the race window, so the crash did still happen, but less often.
The crash was only "fixed" with the workaround_lock patch (but _without_ any of the
ackqueue patches applied.)

> commit 053b47ff249b9e0a634dae807f81465205e7c228
> Author: Michael Buesch <mb@bu3sch.de>
> Date:   Mon Feb 12 00:53:26 2007 -0800
> 
>     [PATCH] Workaround CAPI subsystem locking issue
> 
> they were gone? That's interesting. I'll try to wrap my mind around
> this.

Yeah, this sledgehammer lock did fix the crash while leaving the old non-list-head
queue in place (it should still be there today).

> It's unfortunate that these crashes only seem to occur with one specific
> device (FritzCard DSL) which I don't have.

I still have the device somewhere. If you want to have it, I can blow off the
dust and send it to you. If you don't want it, I'll throw it away soon.
I'd really like to send it to you to get rid of it. ;)

> Can anyone shed some light on 
> what that device is doing differently from other ISDN cards?

Well, it's a combined ISDN/DSL card, but I never used the ISDN part. So the crash
happened while transferring data over the DSL link.
The vendor driver is closed source with an open wrapper (like nvidia). It's a pretty
crappy unmaintained piece of software, but it ran stable with some patches applied
to the driver and the workaround-lock patch to the capi stack.

-- 
Greetings, Michael.

^ permalink raw reply

* Re: [PATCH 21/21] drivers/net/tlan.h: Convert printk(KERN_DEBUG to pr_dbg(
From: Joe Perches @ 2009-10-05 21:28 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: David Miller, linux-kernel, chessman, netdev
In-Reply-To: <200910052315.01357.rjw@sisk.pl>

On Mon, 2009-10-05 at 23:15 +0200, Rafael J. Wysocki wrote:
> Why don't you push the first patch first, then, and wait with the others until
> it gets merged?  This way individual subsystem maintainers will be able to
> merge them cleanly.

Sometimes patch series get a bit more feedback than individual patches.

Original rfc suggestion: (1 comment)
http://lkml.org/lkml/2009/10/1/399

Patch removing pr_fmt from driver (acked by Jiri):
http://lkml.org/lkml/2009/10/1/418

cheers, Joe

^ permalink raw reply

* Re: [PATCH net-2.6] tg3: Fix phylib locking strategy
From: Michael Chan @ 2009-10-05 21:29 UTC (permalink / raw)
  To: Matt Carlson; +Cc: Felix Radensky, netdev@vger.kernel.org, andy@greyhouse.net
In-Reply-To: <1254777182.18287@xw6200>


On Mon, 2009-10-05 at 14:09 -0700, Matt Carlson wrote:
> Felix, can you verify this solves your problem?
> 
> ---
> 
> Felix Radensky noted that chip resets were generating stack trace dumps.
> This is because the driver is attempting to acquire the mdio bus mutex
> while holding the tp->lock spinlock.  The fix is to change the code such
> that every phy access takes the tp->lock spinlock instead.
> 
> Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> ---
>  drivers/net/tg3.c |   37 ++++++++++++-------------------------
>  drivers/net/tg3.h |    1 -
>  2 files changed, 12 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index f09bc5d..8afc60e 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -902,11 +902,12 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
>  	struct tg3 *tp = bp->priv;
>  	u32 val;
>  
> -	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
> -		return -EAGAIN;
> +	spin_lock(&tp->lock);
>  

Matt, do we need spin_lock_bh() here?  The tp->lock can be taken in NAPI
poll BH context.

>  	if (tg3_readphy(tp, reg, &val))
> -		return -EIO;
> +		val = -EIO;
> +
> +	spin_unlock(&tp->lock);
>  
>  	return val;
>  }
> @@ -914,14 +915,16 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
>  static int tg3_mdio_write(struct mii_bus *bp, int mii_id, int reg, u16 val)
>  {
>  	struct tg3 *tp = bp->priv;
> +	u32 ret = 0;
>  
> -	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
> -		return -EAGAIN;
> +	spin_lock(&tp->lock);
>  
>  	if (tg3_writephy(tp, reg, val))
> -		return -EIO;
> +		ret = -EIO;
>  
> -	return 0;
> +	spin_unlock(&tp->lock);
> +
> +	return ret;
>  }
>  
>  static int tg3_mdio_reset(struct mii_bus *bp)
> @@ -1011,12 +1014,6 @@ static void tg3_mdio_config_5785(struct tg3 *tp)
>  
>  static void tg3_mdio_start(struct tg3 *tp)
>  {
> -	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_INITED) {
> -		mutex_lock(&tp->mdio_bus->mdio_lock);
> -		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_PAUSED;
> -		mutex_unlock(&tp->mdio_bus->mdio_lock);
> -	}
> -
>  	tp->mi_mode &= ~MAC_MI_MODE_AUTO_POLL;
>  	tw32_f(MAC_MI_MODE, tp->mi_mode);
>  	udelay(80);
> @@ -1041,15 +1038,6 @@ static void tg3_mdio_start(struct tg3 *tp)
>  		tg3_mdio_config_5785(tp);
>  }
>  
> -static void tg3_mdio_stop(struct tg3 *tp)
> -{
> -	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_INITED) {
> -		mutex_lock(&tp->mdio_bus->mdio_lock);
> -		tp->tg3_flags3 |= TG3_FLG3_MDIOBUS_PAUSED;
> -		mutex_unlock(&tp->mdio_bus->mdio_lock);
> -	}
> -}
> -
>  static int tg3_mdio_init(struct tg3 *tp)
>  {
>  	int i;
> @@ -1141,7 +1129,6 @@ static void tg3_mdio_fini(struct tg3 *tp)
>  		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_INITED;
>  		mdiobus_unregister(tp->mdio_bus);
>  		mdiobus_free(tp->mdio_bus);
> -		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_PAUSED;
>  	}
>  }
>  
> @@ -6392,8 +6379,6 @@ static int tg3_chip_reset(struct tg3 *tp)
>  
>  	tg3_nvram_lock(tp);
>  
> -	tg3_mdio_stop(tp);
> -
>  	tg3_ape_lock(tp, TG3_APE_LOCK_GRC);
>  
>  	/* No matching tg3_nvram_unlock() after this because
> @@ -8698,6 +8683,8 @@ static int tg3_close(struct net_device *dev)
>  
>  	del_timer_sync(&tp->timer);
>  
> +	tg3_phy_stop(tp);
> +
>  	tg3_full_lock(tp, 1);
>  #if 0
>  	tg3_dump_state(tp);
> diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
> index 524691c..bab7940 100644
> --- a/drivers/net/tg3.h
> +++ b/drivers/net/tg3.h
> @@ -2748,7 +2748,6 @@ struct tg3 {
>  #define TG3_FLG3_5701_DMA_BUG		0x00000008
>  #define TG3_FLG3_USE_PHYLIB		0x00000010
>  #define TG3_FLG3_MDIOBUS_INITED		0x00000020
> -#define TG3_FLG3_MDIOBUS_PAUSED		0x00000040
>  #define TG3_FLG3_PHY_CONNECTED		0x00000080
>  #define TG3_FLG3_RGMII_STD_IBND_DISABLE	0x00000100
>  #define TG3_FLG3_RGMII_EXT_IBND_RX_EN	0x00000200



^ permalink raw reply

* [net-2.6 PATCH 0/3] qlge: Fixes for qlge.
From: Ron Mercer @ 2009-10-05 21:46 UTC (permalink / raw)
  To: davem; +Cc: netdev, ron.mercer



^ permalink raw reply

* [net-2.6 PATCH 1/3] qlge: Fix some bit definitions for reset register.
From: Ron Mercer @ 2009-10-05 21:46 UTC (permalink / raw)
  To: davem; +Cc: netdev, ron.mercer
In-Reply-To: <1254779209-15174-1-git-send-email-ron.mercer@qlogic.com>

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
index 30d5585..f470fb9 100644
--- a/drivers/net/qlge/qlge.h
+++ b/drivers/net/qlge/qlge.h
@@ -135,9 +135,9 @@ enum {
 	RST_FO_TFO = (1 << 0),
 	RST_FO_RR_MASK = 0x00060000,
 	RST_FO_RR_CQ_CAM = 0x00000000,
-	RST_FO_RR_DROP = 0x00000001,
-	RST_FO_RR_DQ = 0x00000002,
-	RST_FO_RR_RCV_FUNC_CQ = 0x00000003,
+	RST_FO_RR_DROP = 0x00000002,
+	RST_FO_RR_DQ = 0x00000004,
+	RST_FO_RR_RCV_FUNC_CQ = 0x00000006,
 	RST_FO_FRB = (1 << 12),
 	RST_FO_MOP = (1 << 13),
 	RST_FO_REG = (1 << 14),
-- 
1.6.0.2


^ permalink raw reply related

* [net-2.6 PATCH 2/3] qlge: Fix queueing of firmware handler in ISR.
From: Ron Mercer @ 2009-10-05 21:46 UTC (permalink / raw)
  To: davem; +Cc: netdev, ron.mercer
In-Reply-To: <1254779209-15174-1-git-send-email-ron.mercer@qlogic.com>

Check that we are not already polling firmware events before we queue the
firmware event worker, then disable firmware interrupts.
Otherwise we can queue the same event multiple times.

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge_main.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index 3d0efea..c21eda0 100644
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -2001,15 +2001,17 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
 	/*
 	 * Check MPI processor activity.
 	 */
-	if (var & STS_PI) {
+	if ((var & STS_PI) &&
+		(ql_read32(qdev, INTR_MASK) & INTR_MASK_PI)) {
 		/*
 		 * We've got an async event or mailbox completion.
 		 * Handle it and clear the source of the interrupt.
 		 */
 		QPRINTK(qdev, INTR, ERR, "Got MPI processor interrupt.\n");
 		ql_disable_completion_interrupt(qdev, intr_context->intr);
-		queue_delayed_work_on(smp_processor_id(), qdev->workqueue,
-				      &qdev->mpi_work, 0);
+		ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
+		queue_delayed_work_on(smp_processor_id(),
+				qdev->workqueue, &qdev->mpi_work, 0);
 		work_done++;
 	}
 
-- 
1.6.0.2


^ permalink raw reply related

* [net-2.6 PATCH 3/3] qlge: Fix lock/mutex warnings.
From: Ron Mercer @ 2009-10-05 21:46 UTC (permalink / raw)
  To: davem; +Cc: netdev, ron.mercer
In-Reply-To: <1254779209-15174-1-git-send-email-ron.mercer@qlogic.com>

Get rid of spinlock and private mutex usage for exclusive access to the
HW semaphore register.  rtnl_lock already creates exclusive access to
this register in all driver API.
Add rtnl to firmware worker threads that also use the HW semaphore register.

Signed-off-by: Ron Mercer <ron.mercer@qlogic.com>
---
 drivers/net/qlge/qlge.h         |    2 +-
 drivers/net/qlge/qlge_ethtool.c |    2 --
 drivers/net/qlge/qlge_main.c    |   10 ----------
 drivers/net/qlge/qlge_mpi.c     |   12 ++++++++----
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/net/qlge/qlge.h b/drivers/net/qlge/qlge.h
index f470fb9..3ec6e85 100644
--- a/drivers/net/qlge/qlge.h
+++ b/drivers/net/qlge/qlge.h
@@ -9,6 +9,7 @@
 
 #include <linux/pci.h>
 #include <linux/netdevice.h>
+#include <linux/rtnetlink.h>
 
 /*
  * General definitions...
@@ -1477,7 +1478,6 @@ struct ql_adapter {
 	u32 mailbox_in;
 	u32 mailbox_out;
 	struct mbox_params idc_mbc;
-	struct mutex	mpi_mutex;
 
 	int tx_ring_size;
 	int rx_ring_size;
diff --git a/drivers/net/qlge/qlge_ethtool.c b/drivers/net/qlge/qlge_ethtool.c
index 68f9bd2..5207394 100644
--- a/drivers/net/qlge/qlge_ethtool.c
+++ b/drivers/net/qlge/qlge_ethtool.c
@@ -45,7 +45,6 @@ static int ql_update_ring_coalescing(struct ql_adapter *qdev)
 	if (!netif_running(qdev->ndev))
 		return status;
 
-	spin_lock(&qdev->hw_lock);
 	/* Skip the default queue, and update the outbound handler
 	 * queues if they changed.
 	 */
@@ -92,7 +91,6 @@ static int ql_update_ring_coalescing(struct ql_adapter *qdev)
 		}
 	}
 exit:
-	spin_unlock(&qdev->hw_lock);
 	return status;
 }
 
diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
index c21eda0..6168071 100644
--- a/drivers/net/qlge/qlge_main.c
+++ b/drivers/net/qlge/qlge_main.c
@@ -34,7 +34,6 @@
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <linux/skbuff.h>
-#include <linux/rtnetlink.h>
 #include <linux/if_vlan.h>
 #include <linux/delay.h>
 #include <linux/mm.h>
@@ -1926,12 +1925,10 @@ static void ql_vlan_rx_add_vid(struct net_device *ndev, u16 vid)
 	status = ql_sem_spinlock(qdev, SEM_MAC_ADDR_MASK);
 	if (status)
 		return;
-	spin_lock(&qdev->hw_lock);
 	if (ql_set_mac_addr_reg
 	    (qdev, (u8 *) &enable_bit, MAC_ADDR_TYPE_VLAN, vid)) {
 		QPRINTK(qdev, IFUP, ERR, "Failed to init vlan address.\n");
 	}
-	spin_unlock(&qdev->hw_lock);
 	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
 }
 
@@ -1945,12 +1942,10 @@ static void ql_vlan_rx_kill_vid(struct net_device *ndev, u16 vid)
 	if (status)
 		return;
 
-	spin_lock(&qdev->hw_lock);
 	if (ql_set_mac_addr_reg
 	    (qdev, (u8 *) &enable_bit, MAC_ADDR_TYPE_VLAN, vid)) {
 		QPRINTK(qdev, IFUP, ERR, "Failed to clear vlan address.\n");
 	}
-	spin_unlock(&qdev->hw_lock);
 	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
 
 }
@@ -3587,7 +3582,6 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 	status = ql_sem_spinlock(qdev, SEM_RT_IDX_MASK);
 	if (status)
 		return;
-	spin_lock(&qdev->hw_lock);
 	/*
 	 * Set or clear promiscuous mode if a
 	 * transition is taking place.
@@ -3664,7 +3658,6 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 		}
 	}
 exit:
-	spin_unlock(&qdev->hw_lock);
 	ql_sem_unlock(qdev, SEM_RT_IDX_MASK);
 }
 
@@ -3684,10 +3677,8 @@ static int qlge_set_mac_address(struct net_device *ndev, void *p)
 	status = ql_sem_spinlock(qdev, SEM_MAC_ADDR_MASK);
 	if (status)
 		return status;
-	spin_lock(&qdev->hw_lock);
 	status = ql_set_mac_addr_reg(qdev, (u8 *) ndev->dev_addr,
 			MAC_ADDR_TYPE_CAM_MAC, qdev->func * MAX_CQ);
-	spin_unlock(&qdev->hw_lock);
 	if (status)
 		QPRINTK(qdev, HW, ERR, "Failed to load MAC address.\n");
 	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
@@ -3930,7 +3921,6 @@ static int __devinit ql_init_device(struct pci_dev *pdev,
 	INIT_DELAYED_WORK(&qdev->mpi_work, ql_mpi_work);
 	INIT_DELAYED_WORK(&qdev->mpi_port_cfg_work, ql_mpi_port_cfg_work);
 	INIT_DELAYED_WORK(&qdev->mpi_idc_work, ql_mpi_idc_work);
-	mutex_init(&qdev->mpi_mutex);
 	init_completion(&qdev->ide_completion);
 
 	if (!cards_found) {
diff --git a/drivers/net/qlge/qlge_mpi.c b/drivers/net/qlge/qlge_mpi.c
index 6685bd9..c2e4307 100644
--- a/drivers/net/qlge/qlge_mpi.c
+++ b/drivers/net/qlge/qlge_mpi.c
@@ -472,7 +472,6 @@ static int ql_mailbox_command(struct ql_adapter *qdev, struct mbox_params *mbcp)
 {
 	int status, count;
 
-	mutex_lock(&qdev->mpi_mutex);
 
 	/* Begin polled mode for MPI */
 	ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
@@ -541,7 +540,6 @@ static int ql_mailbox_command(struct ql_adapter *qdev, struct mbox_params *mbcp)
 		status = -EIO;
 	}
 end:
-	mutex_unlock(&qdev->mpi_mutex);
 	/* End polled mode for MPI */
 	ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16) | INTR_MASK_PI);
 	return status;
@@ -776,7 +774,9 @@ static int ql_idc_wait(struct ql_adapter *qdev)
 static int ql_set_port_cfg(struct ql_adapter *qdev)
 {
 	int status;
+	rtnl_lock();
 	status = ql_mb_set_port_cfg(qdev);
+	rtnl_unlock();
 	if (status)
 		return status;
 	status = ql_idc_wait(qdev);
@@ -797,7 +797,9 @@ void ql_mpi_port_cfg_work(struct work_struct *work)
 	    container_of(work, struct ql_adapter, mpi_port_cfg_work.work);
 	int status;
 
+	rtnl_lock();
 	status = ql_mb_get_port_cfg(qdev);
+	rtnl_unlock();
 	if (status) {
 		QPRINTK(qdev, DRV, ERR,
 			"Bug: Failed to get port config data.\n");
@@ -855,7 +857,9 @@ void ql_mpi_idc_work(struct work_struct *work)
 		 * needs to be set.
 		 * */
 		set_bit(QL_CAM_RT_SET, &qdev->flags);
+		rtnl_lock();
 		status = ql_mb_idc_ack(qdev);
+		rtnl_unlock();
 		if (status) {
 			QPRINTK(qdev, DRV, ERR,
 			"Bug: No pending IDC!\n");
@@ -871,7 +875,7 @@ void ql_mpi_work(struct work_struct *work)
 	struct mbox_params *mbcp = &mbc;
 	int err = 0;
 
-	mutex_lock(&qdev->mpi_mutex);
+	rtnl_lock();
 
 	while (ql_read32(qdev, STS) & STS_PI) {
 		memset(mbcp, 0, sizeof(struct mbox_params));
@@ -884,7 +888,7 @@ void ql_mpi_work(struct work_struct *work)
 			break;
 	}
 
-	mutex_unlock(&qdev->mpi_mutex);
+	rtnl_unlock();
 	ql_enable_completion_interrupt(qdev, 0);
 }
 
-- 
1.6.0.2


^ permalink raw reply related

* Re: [PATCH net-2.6] tg3: Fix phylib locking strategy
From: Matt Carlson @ 2009-10-06  0:03 UTC (permalink / raw)
  To: Michael Chan
  Cc: Matthew Carlson, Felix Radensky, netdev@vger.kernel.org,
	andy@greyhouse.net
In-Reply-To: <1254778197.8559.2.camel@HP1>

On Mon, Oct 05, 2009 at 02:29:57PM -0700, Michael Chan wrote:
> 
> On Mon, 2009-10-05 at 14:09 -0700, Matt Carlson wrote:
> > Felix, can you verify this solves your problem?
> > 
> > ---
> > 
> > Felix Radensky noted that chip resets were generating stack trace dumps.
> > This is because the driver is attempting to acquire the mdio bus mutex
> > while holding the tp->lock spinlock.  The fix is to change the code such
> > that every phy access takes the tp->lock spinlock instead.
> > 
> > Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
> > ---
> >  drivers/net/tg3.c |   37 ++++++++++++-------------------------
> >  drivers/net/tg3.h |    1 -
> >  2 files changed, 12 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > index f09bc5d..8afc60e 100644
> > --- a/drivers/net/tg3.c
> > +++ b/drivers/net/tg3.c
> > @@ -902,11 +902,12 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
> >  	struct tg3 *tp = bp->priv;
> >  	u32 val;
> >  
> > -	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
> > -		return -EAGAIN;
> > +	spin_lock(&tp->lock);
> >  
> 
> Matt, do we need spin_lock_bh() here?  The tp->lock can be taken in NAPI
> poll BH context.

Yes.  You are right.  And the same change needs to be made in
tg3_adjust_link().  V2 of the patch, coming up.


^ permalink raw reply

* [PATCH] ipv4: arp_notify address list bug
From: Stephen Hemminger @ 2009-10-06  2:15 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

This fixes a bug with arp_notify and also adds a small enhancement.

If arp_notify is enabled, kernel will crash if address is changed
and no IP address is assigned.
  http://bugzilla.kernel.org/show_bug.cgi?id=14330

The fix is to walk the (possibly empty) list when sending
the gratuitous ARP's.

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

---
This should go to stable kernel fixes as well.

--- a/net/ipv4/devinet.c	2009-10-05 17:31:35.759514625 -0700
+++ b/net/ipv4/devinet.c	2009-10-05 17:44:04.204494945 -0700
@@ -1077,12 +1077,15 @@ static int inetdev_event(struct notifier
 		ip_mc_up(in_dev);
 		/* fall through */
 	case NETDEV_CHANGEADDR:
-		if (IN_DEV_ARP_NOTIFY(in_dev))
-			arp_send(ARPOP_REQUEST, ETH_P_ARP,
-				 in_dev->ifa_list->ifa_address,
-				 dev,
-				 in_dev->ifa_list->ifa_address,
-				 NULL, dev->dev_addr, NULL);
+		/* Send gratitious ARP to notify of link change */
+		if (IN_DEV_ARP_NOTIFY(in_dev)) {
+			struct in_ifaddr *ifa;
+			for (ifa = in_dev->ifa_list; ifa; ifa = ifa->ifa_next)
+				arp_send(ARPOP_REQUEST, ETH_P_ARP,
+					 ifa->ifa_address, dev,
+					 ifa->ifa_address, NULL,
+					 dev->dev_addr, NULL);
+		}
 		break;
 	case NETDEV_DOWN:
 		ip_mc_down(in_dev);

^ permalink raw reply

* RE: [net-next-2.6 PATCH 8/9] vxge: Acquire correct lock based on interrupt context.
From: Ramkrishna Vepa @ 2009-10-06  2:36 UTC (permalink / raw)
  To: David Miller, Sreenivasa Honnur; +Cc: netdev
In-Reply-To: <20091005.053759.74765905.davem@davemloft.net>

> > - Added macros that check if the thread is in interrupt context or
not
> to
> >   acquire or release locks
> >
> > Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>
> 
> There is nothing at all wrong with using irqsave/irqrestore spin lock
> calls when you're already in an interrupt.
> 
> I don't see any reason for this change.
[Ram] Right, but why have the additional step of saving and restoring
the flags while in the interrupt context?

Ram

^ permalink raw reply

* Re: [PATCH] ipv4: arp_notify address list bug
From: Eric Dumazet @ 2009-10-06  3:13 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Miller, netdev
In-Reply-To: <20091005191505.75c929a6@nehalam>

Stephen Hemminger a écrit :
> This fixes a bug with arp_notify and also adds a small enhancement.
> 
> If arp_notify is enabled, kernel will crash if address is changed
> and no IP address is assigned.
>   http://bugzilla.kernel.org/show_bug.cgi?id=14330
> 
> The fix is to walk the (possibly empty) list when sending
> the gratuitous ARP's.

> 
> -				 NULL, dev->dev_addr, NULL);
> +		/* Send gratitious ARP to notify of link change */

	/* gratuitous */

> +		if (IN_DEV_ARP_NOTIFY(in_dev)) {
> +			struct in_ifaddr *ifa;
> +			for (ifa = in_dev->ifa_list; ifa; ifa = ifa->ifa_next)
> +				arp_send(ARPOP_REQUEST, ETH_P_ARP,
> +					 ifa->ifa_address, dev,
> +					 ifa->ifa_address, NULL,
> +					 dev->dev_addr, NULL);
> +		}

This sends a broadcast storm if device has a long address list.

Maybe we should change arp_notify to an INTEGER to be able to give a limit.

If people used to set arp_notify to 1, they wont be surprised too much.

I suggest splitting patch in two parts, one to fix the bug for linux-2.6 and stable,
and another one for net-next-2.6 for the enhancement ?

Thanks


^ permalink raw reply

* Re: [net-next-2.6 PATCH 8/9] vxge: Acquire correct lock based on interrupt context.
From: Eric Dumazet @ 2009-10-06  3:19 UTC (permalink / raw)
  To: Ramkrishna Vepa; +Cc: David Miller, Sreenivasa Honnur, netdev
In-Reply-To: <78C9135A3D2ECE4B8162EBDCE82CAD7705C16322@nekter>

Ramkrishna Vepa a écrit :
> [Ram] Right, but why have the additional step of saving and restoring
> the flags while in the interrupt context?
> 

It costs about the same thing than testing two times in_interrupt()

pushf
pop register
test iflag,register
je ...


In the end, original code is shorter and faster.

^ permalink raw reply

* RE: [net-next-2.6 PATCH 8/9] vxge: Acquire correct lock based on interrupt context.
From: Ramkrishna Vepa @ 2009-10-06  3:23 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, Sreenivasa Honnur, netdev
In-Reply-To: <4ACAB72A.3080802@gmail.com>

> Ramkrishna Vepa a écrit :
> > [Ram] Right, but why have the additional step of saving and restoring
> > the flags while in the interrupt context?
> >
> 
> It costs about the same thing than testing two times in_interrupt()
> 
> pushf
> pop register
> test iflag,register
> je ...
> 
> 
> In the end, original code is shorter and faster.
[Ram] Got it. We'll remove this patch from the patch set.

Ram

^ permalink raw reply

* [PATCH net-2.6 V2] tg3: Fix phylib locking strategy
From: Matt Carlson @ 2009-10-06  3:55 UTC (permalink / raw)
  To: Felix Radensky; +Cc: netdev, Michael Chan, andy

O.K.  Here is the latest version.  Felix, can you verify your problem
is solved with this patch?

---

Felix Radensky noted that chip resets were generating stack trace dumps.
This is because the driver is attempting to acquire the mdio bus mutex
while holding the tp->lock spinlock.  The fix is to change the code such
that every phy access takes the tp->lock spinlock instead.

Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
---
 drivers/net/tg3.c |   41 ++++++++++++++---------------------------
 drivers/net/tg3.h |    1 -
 2 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index f09bc5d..ba5d3fe 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -902,11 +902,12 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
 	struct tg3 *tp = bp->priv;
 	u32 val;
 
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
-		return -EAGAIN;
+	spin_lock_bh(&tp->lock);
 
 	if (tg3_readphy(tp, reg, &val))
-		return -EIO;
+		val = -EIO;
+
+	spin_unlock_bh(&tp->lock);
 
 	return val;
 }
@@ -914,14 +915,16 @@ static int tg3_mdio_read(struct mii_bus *bp, int mii_id, int reg)
 static int tg3_mdio_write(struct mii_bus *bp, int mii_id, int reg, u16 val)
 {
 	struct tg3 *tp = bp->priv;
+	u32 ret = 0;
 
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_PAUSED)
-		return -EAGAIN;
+	spin_lock_bh(&tp->lock);
 
 	if (tg3_writephy(tp, reg, val))
-		return -EIO;
+		ret = -EIO;
 
-	return 0;
+	spin_unlock_bh(&tp->lock);
+
+	return ret;
 }
 
 static int tg3_mdio_reset(struct mii_bus *bp)
@@ -1011,12 +1014,6 @@ static void tg3_mdio_config_5785(struct tg3 *tp)
 
 static void tg3_mdio_start(struct tg3 *tp)
 {
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_INITED) {
-		mutex_lock(&tp->mdio_bus->mdio_lock);
-		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_PAUSED;
-		mutex_unlock(&tp->mdio_bus->mdio_lock);
-	}
-
 	tp->mi_mode &= ~MAC_MI_MODE_AUTO_POLL;
 	tw32_f(MAC_MI_MODE, tp->mi_mode);
 	udelay(80);
@@ -1041,15 +1038,6 @@ static void tg3_mdio_start(struct tg3 *tp)
 		tg3_mdio_config_5785(tp);
 }
 
-static void tg3_mdio_stop(struct tg3 *tp)
-{
-	if (tp->tg3_flags3 & TG3_FLG3_MDIOBUS_INITED) {
-		mutex_lock(&tp->mdio_bus->mdio_lock);
-		tp->tg3_flags3 |= TG3_FLG3_MDIOBUS_PAUSED;
-		mutex_unlock(&tp->mdio_bus->mdio_lock);
-	}
-}
-
 static int tg3_mdio_init(struct tg3 *tp)
 {
 	int i;
@@ -1141,7 +1129,6 @@ static void tg3_mdio_fini(struct tg3 *tp)
 		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_INITED;
 		mdiobus_unregister(tp->mdio_bus);
 		mdiobus_free(tp->mdio_bus);
-		tp->tg3_flags3 &= ~TG3_FLG3_MDIOBUS_PAUSED;
 	}
 }
 
@@ -1363,7 +1350,7 @@ static void tg3_adjust_link(struct net_device *dev)
 	struct tg3 *tp = netdev_priv(dev);
 	struct phy_device *phydev = tp->mdio_bus->phy_map[PHY_ADDR];
 
-	spin_lock(&tp->lock);
+	spin_lock_bh(&tp->lock);
 
 	mac_mode = tp->mac_mode & ~(MAC_MODE_PORT_MODE_MASK |
 				    MAC_MODE_HALF_DUPLEX);
@@ -1431,7 +1418,7 @@ static void tg3_adjust_link(struct net_device *dev)
 	tp->link_config.active_speed = phydev->speed;
 	tp->link_config.active_duplex = phydev->duplex;
 
-	spin_unlock(&tp->lock);
+	spin_unlock_bh(&tp->lock);
 
 	if (linkmesg)
 		tg3_link_report(tp);
@@ -6392,8 +6379,6 @@ static int tg3_chip_reset(struct tg3 *tp)
 
 	tg3_nvram_lock(tp);
 
-	tg3_mdio_stop(tp);
-
 	tg3_ape_lock(tp, TG3_APE_LOCK_GRC);
 
 	/* No matching tg3_nvram_unlock() after this because
@@ -8698,6 +8683,8 @@ static int tg3_close(struct net_device *dev)
 
 	del_timer_sync(&tp->timer);
 
+	tg3_phy_stop(tp);
+
 	tg3_full_lock(tp, 1);
 #if 0
 	tg3_dump_state(tp);
diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index 524691c..bab7940 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h
@@ -2748,7 +2748,6 @@ struct tg3 {
 #define TG3_FLG3_5701_DMA_BUG		0x00000008
 #define TG3_FLG3_USE_PHYLIB		0x00000010
 #define TG3_FLG3_MDIOBUS_INITED		0x00000020
-#define TG3_FLG3_MDIOBUS_PAUSED		0x00000040
 #define TG3_FLG3_PHY_CONNECTED		0x00000080
 #define TG3_FLG3_RGMII_STD_IBND_DISABLE	0x00000100
 #define TG3_FLG3_RGMII_EXT_IBND_RX_EN	0x00000200
-- 
1.6.4.4



^ permalink raw reply related

* Re: [net-next-2.6 PATCH 8/9] vxge: Acquire correct lock based on interrupt context.
From: David Miller @ 2009-10-06  4:18 UTC (permalink / raw)
  To: Ramkrishna.Vepa; +Cc: Sreenivasa.Honnur, netdev
In-Reply-To: <78C9135A3D2ECE4B8162EBDCE82CAD7705C16322@nekter>

From: "Ramkrishna Vepa" <Ramkrishna.Vepa@neterion.com>
Date: Mon, 5 Oct 2009 22:36:22 -0400

>> > - Added macros that check if the thread is in interrupt context or
> not
>> to
>> >   acquire or release locks
>> >
>> > Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>
>> 
>> There is nothing at all wrong with using irqsave/irqrestore spin lock
>> calls when you're already in an interrupt.
>> 
>> I don't see any reason for this change.
> [Ram] Right, but why have the additional step of saving and restoring
> the flags while in the interrupt context?

Do you know that in the interrupt handler, cpu interrupts are
actually enabled?

And the cost is (relatively speaking) next to nothing.  You're
in the process of taking cache misses and toucing PIO registers
(on the order of thousands of cycles).

Not doing a IRQ save/restore is going to save you a hand full
of cycles.

The new branch mispredict you might get there is probably more
expensive or of equal expense to the IRQ save/restore itself.

This change makes really no sense, NO OTHER DRIVER does crap
like this.  Because there is no reason to.

^ permalink raw reply

* Re: tg3: bug report, driver freeze (transmit timed out), ifdown+ifup makes interface work again
From: Matt Carlson @ 2009-10-06  4:21 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Michael Chan, Matthew Carlson, netdev@vger.kernel.org,
	sander.contrib@gmail.com, David S. Miller
In-Reply-To: <1254386068.5551.25.camel@localhost.localdomain>

Thanks for the detailed bug report.  There are lots of things to think
about.

While I think about the next step, can you tell me whether or not jumbo
frames is enabled?

On Thu, Oct 01, 2009 at 01:34:28AM -0700, Jesper Dangaard Brouer wrote:
> 
> A friend of mine is experiencing problems with his tg3 based NIC.  He is
> experiencing the net stops working (transmit timed out), and he hade to
> access the console to get it working again.
> 
> Kernel: 2.6.26-2-686 (standard Debian package)
> OS: Debian Lenny 5.0 (all upgrades)
> 
> Ethernet controller: Broadcom Corporation NetXtreme BCM5700 Gigabit Ethernet (rev 12)
>  Subsystem: Dell Broadcom BCM5700
>  eth1: Tigon3 [partno(none) rev 7102 PHY(5401)]
> 
> Is this a known issue? (If so what kernel is it fixed in... that I can
> make him test...)
> 
> Cite:
> According to the kernel log the tg3 driver tries to reset it self.
> However, even though it looks like the interface is up, it is not!
> 
> A manuel ifdown eth1 && ifup eth1 does the trick.
> 
> According to my rtorrent I had used about 4GB of traffic (combined
> down/up)..  so a qualified guess could be a 32-bit limitation in the
> tg3-driver?
> 
> 
> Server specs:
>  DELL PowerEdge 2550
>  2 GB Ram
>  2x1 Ghz Pentium III (Coppermine)
> 
> 
> Sep 30 11:45:46 samurai kernel: [1145615.063992] NETDEV WATCHDOG: eth1: transmit timed out
> Sep 30 11:45:46 samurai kernel: [1145615.064028] tg3: eth1: transmit timed out, resetting
> Sep 30 11:45:46 samurai kernel: [1145615.064052] tg3: DEBUG: MAC_TX_STATUS[00000008] MAC_RX_STATUS[00000008]
> Sep 30 11:45:46 samurai kernel: [1145615.064078] tg3: DEBUG: RDMAC_STATUS[00000000] WDMAC_STATUS[00000000]
> Sep 30 11:45:46 samurai kernel: [1145615.064119] ------------[ cut here]------------
> Sep 30 11:45:46 samurai kernel: [1145615.064141] WARNING: at net/sched/sch_generic.c:222 dev_watchdog+0x8f/0xdc()
> Sep 30 11:45:46 samurai kernel: [1145615.064174] Modules linked in: iptable_mangle iptable_nat nf_nat ipt_LOG nf_conntrack_ip
> v4 xt_state nf_conntrack xt_tcpudp iptable_filter ip_tables x_tables ipv6 dm_snapshot dm_mirror dm_log dm_mod loop parport_pc
>  parport evdev psmouse snd_pcm snd_timer snd soundcore snd_page_alloc serio_raw pcspkr shpchp pci_hotplug i2c_piix4 i2c_core
> button sworks_agp agpgart dcdbas ext3 jbd mbcache sg sd_mod ide_cd_mod cdrom ide_pci_generic serverworks ide_core floppy aacr
> aid aic7xxx scsi_transport_spi ata_generic e100 ohci_hcd libata scsi_mod dock tg3 usbcore 8139cp 8139too mii thermal processo
> r fan thermal_sys [last unloaded: scsi_wait_scan]
> Sep 30 11:45:46 samurai kernel: [1145615.064517] Pid: 0, comm: swapper Not tainted 2.6.26-2-686 #1
> Sep 30 11:45:46 samurai kernel: [1145615.064549]  [<c01225f3>] warn_on_slowpath+0x40/0x66
> Sep 30 11:45:46 samurai kernel: [1145615.064594]  [<c0119160>] hrtick_start_fair+0xeb/0x12c
> Sep 30 11:45:46 samurai kernel: [1145615.064635]  [<c0118926>] enqueue_task+0x52/0x5d
> Sep 30 11:45:46 samurai kernel: [1145615.064663]  [<c011894c>] activate_task+0x1b/0x26
> Sep 30 11:45:46 samurai kernel: [1145615.064690]  [<c011b6f3>] try_to_wake_up+0xe8/0xf1
> Sep 30 11:45:46 samurai kernel: [1145615.064723]  [<c01319a9>] autoremove_wake_function+0xd/0x2d
> Sep 30 11:45:46 samurai kernel: [1145615.064760]  [<c01184d1>] __wake_up_common+0x2e/0x58
> Sep 30 11:45:46 samurai kernel: [1145615.064792]  [<c011a6bb>] __wake_up+0x29/0x39
> Sep 30 11:45:46 samurai kernel: [1145615.064822]  [<c012f11f>] insert_work+0x58/0x5c
> Sep 30 11:45:46 samurai kernel: [1145615.064849]  [<c012f40d>] __queue_work+0x1c/0x28
> Sep 30 11:45:46 samurai kernel: [1145615.064876]  [<c012f468>] queue_work+0x33/0x3c
> Sep 30 11:45:46 samurai kernel: [1145615.064903]  [<c0267035>] dev_watchdog+0x8f/0xdc
> Sep 30 11:45:46 samurai kernel: [1145615.064930]  [<c01296d4>] run_timer_softirq+0x11a/0x17c
> Sep 30 11:45:46 samurai kernel: [1145615.064960]  [<c0266fa6>] dev_watchdog+0x0/0xdc
> Sep 30 11:45:46 samurai kernel: [1145615.064993]  [<c01265f5>] __do_softirq+0x66/0xd3
> Sep 30 11:45:46 samurai kernel: [1145615.065022]  [<c01266a7>] do_softirq+0x45/0x53
> Sep 30 11:45:46 samurai kernel: [1145615.065047]  [<c012695e>] irq_exit+0x35/0x67
> Sep 30 11:45:46 samurai kernel: [1145615.065070]  [<c01101c9>] smp_apic_timer_interrupt+0x6b/0x76
> Sep 30 11:45:46 samurai kernel: [1145615.065098]  [<c0102656>] default_idle+0x0/0x53
> Sep 30 11:45:46 samurai kernel: [1145615.065127]  [<c0104364>] apic_timer_interrupt+0x28/0x30
> Sep 30 11:45:46 samurai kernel: [1145615.065156]  [<c0102656>] default_idle+0x0/0x53
> Sep 30 11:45:46 samurai kernel: [1145615.065189]  [<c0114d78>] native_safe_halt+0x2/0x3
> Sep 30 11:45:46 samurai kernel: [1145615.065225]  [<c0102683>] default_idle+0x2d/0x53
> Sep 30 11:45:46 samurai kernel: [1145615.065250]  [<c01025ce>] cpu_idle+0xab/0xcb
> Sep 30 11:45:46 samurai kernel: [1145615.065291]  =======================
> Sep 30 11:45:46 samurai kernel: [1145615.065311] ---[ end trace 0dbb94f68d53053b ]---
> Sep 30 11:45:46 samurai kernel: [1145615.457820] tg3: tg3_stop_block timed out, ofs=2c00 enable_bit=2
> Sep 30 11:45:46 samurai kernel: [1145615.557909] tg3: tg3_stop_block timed out, ofs=3400 enable_bit=2
> Sep 30 11:45:46 samurai kernel: [1145615.657903] tg3: tg3_stop_block timed out, ofs=2400 enable_bit=2
> Sep 30 11:45:46 samurai kernel: [1145615.758203] tg3: tg3_stop_block timed out, ofs=1800 enable_bit=2
> Sep 30 11:45:47 samurai kernel: [1145615.858203] tg3: tg3_stop_block timed out, ofs=c00 enable_bit=2
> Sep 30 11:45:47 samurai kernel: [1145615.958203] tg3: tg3_stop_block timed out, ofs=4800 enable_bit=2
> Sep 30 11:45:47 samurai kernel: [1145616.089213] tg3: eth1: Link is down.
> Sep 30 11:45:49 samurai kernel: [1145618.565251] tg3: eth1: Link is up at 100 Mbps, full duplex.
> Sep 30 11:45:49 samurai kernel: [1145618.565288] tg3: eth1: Flow control is off for TX and off for RX.
> 
> Sep 30 14:02:09 samurai kernel: [1154721.802641] NETDEV WATCHDOG: eth1: transmit timed out
> Sep 30 14:02:09 samurai kernel: [1154721.802679] tg3: eth1: transmit timed out, resetting
> Sep 30 14:02:09 samurai kernel: [1154721.802702] tg3: DEBUG: MAC_TX_STATUS[00000008] MAC_RX_STATUS[00000008]
> Sep 30 14:02:09 samurai kernel: [1154721.802729] tg3: DEBUG: RDMAC_STATUS[00000000] WDMAC_STATUS[00000000]
> Sep 30 14:02:09 samurai kernel: [1154721.974663] tg3: tg3_stop_block timed out, ofs=1800 enable_bit=2
> Sep 30 14:02:09 samurai kernel: [1154722.078613] tg3: tg3_stop_block timed out, ofs=4800 enable_bit=2
> Sep 30 14:02:09 samurai kernel: [1154722.206614] tg3: eth1: Link is down.
> Sep 30 14:02:11 samurai kernel: [1154724.209290] tg3: eth1: Link is up at 100 Mbps, full duplex.
> Sep 30 14:02:11 samurai kernel: [1154724.209328] tg3: eth1: Flow control is off for TX and off for RX.
> 
> -- 
> Med venlig hilsen / Best regards
>   Jesper Brouer
>   ComX Networks A/S
>   Linux Network developer
>   Cand. Scient Datalog / MSc.
>   Author of http://adsl-optimizer.dk
>   LinkedIn: http://www.linkedin.com/in/brouer
> 
> lspci -vvv
> 01:08.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5700 Gigabit Ethernet (rev 12)
>         Subsystem: Dell Broadcom BCM5700
>         Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx-
>         Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-<TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 32 (16000ns min), Cache Line Size: 32 bytes
>         Interrupt: pin A routed to IRQ 17
>         Region 0: Memory at feb00000 (64-bit, non-prefetchable) [size=64K]
>         Capabilities: [40] PCI-X non-bridge device
>                 Command: DPERE- ERO- RBC=512 OST=1
>                 Status: Dev=ff:1f.1 64bit+ 133MHz+ SCD- USC- DC=simple DMMRBC=512 DMOST=1 DMCRS=8 RSCEM- 266MHz- 533MHz-
>         Capabilities: [48] Power Management version 2
>                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot+,D3cold-)
>                 Status: D0 PME-Enable- DSel=0 DScale=1 PME-
>         Capabilities: [50] Vital Product Data <?>
>         Capabilities: [58] Message Signalled Interrupts: Mask- 64bit+ Queue=0/3 Enable-
>                 Address: da6771daee5b44a4  Data: 889a
>         Kernel driver in use: tg3
>         Kernel modules: tg3
> 
> 
> ethtool -i eth1:
> driver: tg3
> version: 3.92.1
> firmware-version:
> bus-info: 0000:01:08.0
> 
> Sep 18 22:34:19 samurai kernel: [ 4.707217] eth1: Tigon3 [partno(none) rev 7102 PHY(5401)] (PCI:66MHz:64-bit) 10/100/1000B
> ase-T Ethernet 00:06:5b:39:d3:4a
> Sep 18 22:34:19 samurai kernel: [ 4.707217] eth1: RXcsums[1] LinkChgREG[1] MIirq[1] ASF[0] WireSpeed[0] TSOcap[0]
> Sep 18 22:34:19 samurai kernel: [ 4.707217] eth1: dma_rwctrl[76ff000f] dma_mask[64-bit]
> 


^ permalink raw reply

* Re: [PATCH] ipv4: arp_notify address list bug
From: David Miller @ 2009-10-06  4:31 UTC (permalink / raw)
  To: eric.dumazet; +Cc: shemminger, netdev
In-Reply-To: <4ACAB5BC.1020307@gmail.com>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 06 Oct 2009 05:13:00 +0200

> I suggest splitting patch in two parts, one to fix the bug for linux-2.6 and stable,
> and another one for net-next-2.6 for the enhancement ?
> 

Good idea.

^ permalink raw reply

* [PATCH] ipv4: arp_notify address list bug
From: Eric Dumazet @ 2009-10-06  5:20 UTC (permalink / raw)
  To: David Miller; +Cc: Hannes Frederic Sowa, shemminger, netdev
In-Reply-To: <20091005.213103.237708475.davem@davemloft.net>

From: Stephen Hemminger <shemminger@vyatta.com>

This fixes a bug with arp_notify.

If arp_notify is enabled, kernel will crash if address is changed
and no IP address is assigned.
  http://bugzilla.kernel.org/show_bug.cgi?id=14330

Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv4/devinet.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e92f1fd..5df2f6a 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1077,12 +1077,16 @@ static int inetdev_event(struct notifier_block *this, unsigned long event,
 		ip_mc_up(in_dev);
 		/* fall through */
 	case NETDEV_CHANGEADDR:
-		if (IN_DEV_ARP_NOTIFY(in_dev))
-			arp_send(ARPOP_REQUEST, ETH_P_ARP,
-				 in_dev->ifa_list->ifa_address,
-				 dev,
-				 in_dev->ifa_list->ifa_address,
-				 NULL, dev->dev_addr, NULL);
+		/* Send gratuitous ARP to notify of link change */
+		if (IN_DEV_ARP_NOTIFY(in_dev)) {
+			struct in_ifaddr *ifa = in_dev->ifa_list;
+
+			if (ifa)
+				arp_send(ARPOP_REQUEST, ETH_P_ARP,
+					 ifa->ifa_address, dev,
+					 ifa->ifa_address, NULL,
+					 dev->dev_addr, NULL);
+		}
 		break;
 	case NETDEV_DOWN:
 		ip_mc_down(in_dev);

^ permalink raw reply related

* Re: [PATCH 0/4][RFC]: coding convention for CCID-struct prefixes
From: Gerrit Renker @ 2009-10-06  5:36 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: David Miller, dccp, netdev
In-Reply-To: <20091005123823.GE30535@ghostprotocols.net>

| > I am waiting for the feedback also in order to rebuild the test tree; and have
| > informed CCID-4 developers (CCID-4 subtree) about this.
| 
| On a first look I saw one inconsistency, while in ccid3 you do:
| 
| -	return scaled_div(w_init << 6, hctx->tx_rtt);
| +	return scaled_div(w_init << 6, hc->tx_rtt);
| 
| in ccid2 you do:
| 
| -	struct ccid2_seq *seqp = hctx->ccid2hctx_seqh;
| +	struct ccid2_seq *seqp = hctx->tx_seqh;
| 
| Since this change is about reducing the names by removing redundancy, I
| think the ccid3 variant is better, i.e.: hc->tx_foo.
| 
I fully agree with your comment, but could I ask you to take a second look please?

(My fine-grained separation of patches may not have been as good an idea
 as I had initially thought.)

The first change (scaled_div/ccid3) is from patch 1/4, whereas the second (seqp/ccid2)
is from patch 4/4. In the end the changes complement one another, and both ccids have
the same naming scheme:
 * patch 1/4 replaces hc{tx,rx}->ccid2hc{tx,rx}_ with hc{tx,rx}->{tx,rx}_ (ccid2.{c,h})
 * patch 2/4 replaces hc{tx,rx}->ccid3hc{tx,rx}_ with hc{tx,rx}->{tx_rx}_ (ccid3.{c,h})
 * patch 3/4 replaces hc{tx,rx}->{tx,rx}_ with hc->{tx,rx}_ (ccid2.{c,h})
 * patch 4/4 replaces hc{tx,rx}->{tx,rx}_ with hc->{tx,rx}_ (ccid3.{c,h})

I checked again and re-applied the submitted patches and did the following:

gerrit@virtual_carrot > grep -REhC2 'hc(tx|rx)' net/dccp/
static inline struct ccid3_hc_tx_sock *ccid3_hc_tx_sk(const struct sock *sk)
{
        struct ccid3_hc_tx_sock *hctx = ccid_priv(dccp_sk(sk)->dccps_hc_tx_ccid);
        BUG_ON(hctx == NULL);
        return hctx;
}

--
static inline struct ccid3_hc_rx_sock *ccid3_hc_rx_sk(const struct sock *sk)
{
        struct ccid3_hc_rx_sock *hcrx = ccid_priv(dccp_sk(sk)->dccps_hc_rx_ccid);
        BUG_ON(hcrx == NULL);
        return hcrx;
}

These are the only two exceptions, I left the hc{tx,rx} in since they don't appear
in a prefix. 

Can you please have a look and say whether you are ok with the naming scheme?

As per earlier email, I'd be ok to repackage or combine the patches into a single one,
or combine patch 1/4 with 3/4 and 2/4 with 4/4.

^ permalink raw reply

* Re: [PATCH net-2.6 V2] tg3: Fix phylib locking strategy
From: Felix Radensky @ 2009-10-06  5:45 UTC (permalink / raw)
  To: Matt Carlson; +Cc: netdev, Michael Chan, andy
In-Reply-To: <1254801692.18507@xw6200>

Hi, Matt

Matt Carlson wrote:
> O.K.  Here is the latest version.  Felix, can you verify your problem
> is solved with this patch?
>
>   
Thanks for the patch. I don't have the board with Broadcom NIC at the 
moment.
Hopefully I'll get it back in a couple of days and will test your patch.

Felix.

^ permalink raw reply

* Re: [r8169.c] support for 8168D/DP was Re: r8169 chips on some Intel D945GSEJT boards fail to work after
From: Francois Romieu @ 2009-10-06  6:05 UTC (permalink / raw)
  To: Xose Vazquez Perez; +Cc: netdev, Rainer.Koenig, Simon Farnsworth
In-Reply-To: <4AC8C5C2.7040502@gmail.com>

Xose Vazquez Perez <xose.vazquez@gmail.com> :
[...]
> Francois, is it ready for 2.6.32-rc2 ?

s/rc2/rc3/ otherwise yes.

-- 
Ueimor

^ permalink raw reply

* RE: [net-next-2.6 PATCH 6/9] vxge: Check if FCS stripping is disabled by the firmware.
From: Ramkrishna Vepa @ 2009-10-06  6:41 UTC (permalink / raw)
  To: Eric Dumazet, Sreenivasa Honnur; +Cc: davem, netdev
In-Reply-To: <4AC9EEE0.5000601@gmail.com>

> > - Added a function to check if FCS stripping is disabled by the
> firmware, if
> >   it is not disabled fail driver load.
> >
> > - By default FCS stripping is disabled by the firmware. With this
> assumption
> >   driver decrements the indicated packet length by 4 bytes(FCS
length).
> >
> > - This patch ensures that FCS stripping is disabled during driver
load
> time.
> >
> > Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>
> 
> What the big deal about FCS not being stripped ?
[Ram] The minimum frame size that can be received by the hardware is 57
Bytes. A 64 Byte Ethernet frame with the vlan tag and fcs stripped will
result in a 56 Byte frame which will lock up the receive engine. The
work around is to disable fcs stripping in the hardware which is a
privileged operation done with a firmware version that the driver checks
for currently.

Ram

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox