* [PATCH net-2.6] tg3: Fix phylib locking strategy
@ 2009-10-05 21:09 Matt Carlson
2009-10-05 21:29 ` Michael Chan
0 siblings, 1 reply; 3+ messages in thread
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 [flat|nested] 3+ messages in thread* Re: [PATCH net-2.6] tg3: Fix phylib locking strategy
2009-10-05 21:09 [PATCH net-2.6] tg3: Fix phylib locking strategy Matt Carlson
@ 2009-10-05 21:29 ` Michael Chan
2009-10-06 0:03 ` Matt Carlson
0 siblings, 1 reply; 3+ messages in thread
From: Michael Chan @ 2009-10-05 21:29 UTC (permalink / raw)
To: Matt Carlson; +Cc: Felix Radensky, netdev@vger.kernel.org, andy@greyhouse.net
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 [flat|nested] 3+ messages in thread* Re: [PATCH net-2.6] tg3: Fix phylib locking strategy
2009-10-05 21:29 ` Michael Chan
@ 2009-10-06 0:03 ` Matt Carlson
0 siblings, 0 replies; 3+ messages in thread
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
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 [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-10-06 0:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-05 21:09 [PATCH net-2.6] tg3: Fix phylib locking strategy Matt Carlson
2009-10-05 21:29 ` Michael Chan
2009-10-06 0:03 ` Matt Carlson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).