[PATCH net-next] bnx2x: avoid TX timeout when stopping device

[PATCH net-next] bnx2x: avoid TX timeout when stopping device

[Stanislaw Gruszka]

When stop device call netif_carrier_off() just after disabling TX queue to
avoid possibility of netdev watchdog warning and ->ndo_tx_timeout() invocation.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/bnx2x_main.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 2bc35c7..57ff5b3 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -8499,6 +8499,7 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
 
 	/* Disable HW interrupts, NAPI and Tx */
 	bnx2x_netif_stop(bp, 1);
+	netif_carrier_off(bp->dev);
 
 	del_timer_sync(&bp->timer);
 	SHMEM_WR(bp, func_mb[BP_FUNC(bp)].drv_pulse_mb,
@@ -8524,8 +8525,6 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
 
 	bp->state = BNX2X_STATE_CLOSED;
 
-	netif_carrier_off(bp->dev);
-
 	/* The last driver must disable a "close the gate" if there is no
 	 * parity attention or "process kill" pending.
 	 */
@@ -13431,6 +13430,7 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
 	bp->rx_mode = BNX2X_RX_MODE_NONE;
 
 	bnx2x_netif_stop(bp, 0);
+	netif_carrier_off(bp->dev);
 
 	del_timer_sync(&bp->timer);
 	bp->stats_state = STATS_STATE_DISABLED;
@@ -13457,8 +13457,6 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
 
 	bp->state = BNX2X_STATE_CLOSED;
 
-	netif_carrier_off(bp->dev);
-
 	return 0;
 }
 
-- 
1.5.5.6

Re: [PATCH net-next] bnx2x: avoid TX timeout when stopping device

[Eric Dumazet]

Le mercredi 12 mai 2010 à 11:09 +0200, Stanislaw Gruszka a écrit :
> When stop device call netif_carrier_off() just after disabling TX queue to
> avoid possibility of netdev watchdog warning and ->ndo_tx_timeout() invocation.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---

This reminds me I saw some strange things in bnx2.c for a similar
symptom.

Commit e6bf95ffa8d6f8f4b7ee33ea01490d95b0bbeb6e

Would you take a look at this too ?

Or if this kind of trans_start refresh on all queues is really needed,
it should be a core network provided function, not implemented on every
driver...

Thanks :)

>  drivers/net/bnx2x_main.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 2bc35c7..57ff5b3 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -8499,6 +8499,7 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
>  
>  	/* Disable HW interrupts, NAPI and Tx */
>  	bnx2x_netif_stop(bp, 1);
> +	netif_carrier_off(bp->dev);
>  
>  	del_timer_sync(&bp->timer);
>  	SHMEM_WR(bp, func_mb[BP_FUNC(bp)].drv_pulse_mb,
> @@ -8524,8 +8525,6 @@ static int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode)
>  
>  	bp->state = BNX2X_STATE_CLOSED;
>  
> -	netif_carrier_off(bp->dev);
> -
>  	/* The last driver must disable a "close the gate" if there is no
>  	 * parity attention or "process kill" pending.
>  	 */
> @@ -13431,6 +13430,7 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
>  	bp->rx_mode = BNX2X_RX_MODE_NONE;
>  
>  	bnx2x_netif_stop(bp, 0);
> +	netif_carrier_off(bp->dev);
>  
>  	del_timer_sync(&bp->timer);
>  	bp->stats_state = STATS_STATE_DISABLED;
> @@ -13457,8 +13457,6 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
>  
>  	bp->state = BNX2X_STATE_CLOSED;
>  
> -	netif_carrier_off(bp->dev);
> -
>  	return 0;
>  }
>  

Re: [PATCH net-next] bnx2x: avoid TX timeout when stopping device

[Stanislaw Gruszka]

On Wed, 12 May 2010 11:27:38 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le mercredi 12 mai 2010 à 11:09 +0200, Stanislaw Gruszka a écrit :
> > When stop device call netif_carrier_off() just after disabling TX queue to
> > avoid possibility of netdev watchdog warning and ->ndo_tx_timeout() invocation.
> > 
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> 
> This reminds me I saw some strange things in bnx2.c for a similar
> symptom.
> 
> Commit e6bf95ffa8d6f8f4b7ee33ea01490d95b0bbeb6e
> 
> Would you take a look at this too ?

I can send RFC patch for bnx2, and tg3 as I think it needs similar fix.
 
> Or if this kind of trans_start refresh on all queues is really needed,
> it should be a core network provided function, not implemented on every
> driver...

I think netif_carrier_off() should be used, since touching trans_start make
timeout only less probable, but not prevent it. 

Stanislaw

[RFC PATCH] bnx2: use netif_carrier_off to prevent tx timeout

[Stanislaw Gruszka]

Touching ->trans_start make netdev watchdog timeouts only less probable.
Use netif_carrier_off to prevent timeout, lately we take care of tuning
carrier on.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
Patch was not tested!

 drivers/net/bnx2.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 667f419..44fc392 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -656,17 +656,9 @@ bnx2_netif_stop(struct bnx2 *bp, bool stop_cnic)
 	if (stop_cnic)
 		bnx2_cnic_stop(bp);
 	if (netif_running(bp->dev)) {
-		int i;
-
 		bnx2_napi_disable(bp);
 		netif_tx_disable(bp->dev);
-		/* prevent tx timeout */
-		for (i = 0; i <  bp->dev->num_tx_queues; i++) {
-			struct netdev_queue *txq;
-
-			txq = netdev_get_tx_queue(bp->dev, i);
-			txq->trans_start = jiffies;
-		}
+		netif_carrier_off(bp->dev);
 	}
 	bnx2_disable_int_sync(bp);
 }
@@ -6346,6 +6338,8 @@ bnx2_vlan_rx_register(struct net_device *dev, struct vlan_group *vlgrp)
 	if (bp->flags & BNX2_FLAG_CAN_KEEP_VLAN)
 		bnx2_fw_sync(bp, BNX2_DRV_MSG_CODE_KEEP_VLAN_UPDATE, 0, 1);
 
+	if (bp->link_up)
+		netif_carrier_on(bp->dev);
 	bnx2_netif_start(bp, false);
 }
 #endif
-- 
1.5.5.6

[RFC PATCH] tg3: use netif_carrier_off to prevent tx timeout

[Stanislaw Gruszka]

Touching ->trans_start make netdev watchdog timeouts only less probable.
Use netif_carrier_off to prevent timeout, lately we take care of turning
carrier on.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
Patch was not tested!

 drivers/net/tg3.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 573054a..d745038 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -753,7 +753,7 @@ static void tg3_napi_enable(struct tg3 *tp)
 
 static inline void tg3_netif_stop(struct tg3 *tp)
 {
-	tp->dev->trans_start = jiffies;	/* prevent tx timeout */
+	netif_carrier_off(tp->dev);	/* prevent tx timeout */
 	tg3_napi_disable(tp);
 	netif_tx_disable(tp->dev);
 }
@@ -10964,12 +10964,14 @@ static int tg3_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 static void tg3_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
 {
 	struct tg3 *tp = netdev_priv(dev);
+	int link_up;
 
 	if (!netif_running(dev)) {
 		tp->vlgrp = grp;
 		return;
 	}
 
+	link_up = netif_carrier_ok(dev);
 	tg3_netif_stop(tp);
 
 	tg3_full_lock(tp, 0);
@@ -10979,6 +10981,8 @@ static void tg3_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
 	/* Update RX_MODE_KEEP_VLAN_TAG bit in RX_MODE register. */
 	__tg3_set_rx_mode(dev);
 
+	if (link_up)
+		netif_carrier_on(dev);
 	tg3_netif_start(tp);
 
 	tg3_full_unlock(tp);
-- 
1.5.5.6

Re: [PATCH net-next] bnx2x: avoid TX timeout when stopping device

[Eilon Greenstein]

On Wed, 2010-05-12 at 03:58 -0700, Stanislaw Gruszka wrote:
> On Wed, 12 May 2010 11:27:38 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Le mercredi 12 mai 2010 à 11:09 +0200, Stanislaw Gruszka a écrit :
> > > When stop device call netif_carrier_off() just after disabling TX queue to
> > > avoid possibility of netdev watchdog warning and ->ndo_tx_timeout() invocation.
> > > 
> > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Stanislaw,

I like this patch - it does make a lot of sense. Thanks!

Acked-by: Eilon Greenstein <eilong@broadcom.com>

> > > ---
> > 
> > This reminds me I saw some strange things in bnx2.c for a similar
> > symptom.
> > 
> > Commit e6bf95ffa8d6f8f4b7ee33ea01490d95b0bbeb6e
> > 
> > Would you take a look at this too ?
> 
> I can send RFC patch for bnx2, and tg3 as I think it needs similar fix.
>  
> > Or if this kind of trans_start refresh on all queues is really needed,
> > it should be a core network provided function, not implemented on every
> > driver...
> 
> I think netif_carrier_off() should be used, since touching trans_start make
> timeout only less probable, but not prevent it. 

Eric,

I thought that your 1ae5dc342ac78d7a42965fd1f323815f6f5ef2c1 commit took
care of trans_start, right? Well, actually the commit that made it
possible to remove the trans_start manipulation.

Eilon

Re: [PATCH net-next] bnx2x: avoid TX timeout when stopping device

[Eric Dumazet]

Le mercredi 12 mai 2010 à 14:19 +0300, Eilon Greenstein a écrit :

> Eric,
> 
> I thought that your 1ae5dc342ac78d7a42965fd1f323815f6f5ef2c1 commit took
> care of trans_start, right? Well, actually the commit that made it
> possible to remove the trans_start manipulation.

Its only part of the (huge) job, started last year with 10GB and GB
nics.

My intent was to take care of not touching dev->trans_start in
start_xmit() method, then take care of other cases later.

Re: [RFC PATCH] bnx2: use netif_carrier_off to prevent tx timeout

[Michael Chan]

Stanislaw Gruszka wrote:

> Touching ->trans_start make netdev watchdog timeouts only
> less probable.
> Use netif_carrier_off to prevent timeout, lately we take care
> of tuning
> carrier on.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> Patch was not tested!
>
>  drivers/net/bnx2.c |   12 +++---------
>  1 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 667f419..44fc392 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -656,17 +656,9 @@ bnx2_netif_stop(struct bnx2 *bp, bool stop_cnic)
>       if (stop_cnic)
>               bnx2_cnic_stop(bp);
>       if (netif_running(bp->dev)) {
> -             int i;
> -
>               bnx2_napi_disable(bp);
>               netif_tx_disable(bp->dev);
> -             /* prevent tx timeout */
> -             for (i = 0; i <  bp->dev->num_tx_queues; i++) {
> -                     struct netdev_queue *txq;
> -
> -                     txq = netdev_get_tx_queue(bp->dev, i);
> -                     txq->trans_start = jiffies;
> -             }
> +             netif_carrier_off(bp->dev);
>       }
>       bnx2_disable_int_sync(bp);
>  }
> @@ -6346,6 +6338,8 @@ bnx2_vlan_rx_register(struct net_device
> *dev, struct vlan_group *vlgrp)
>       if (bp->flags & BNX2_FLAG_CAN_KEEP_VLAN)
>               bnx2_fw_sync(bp,
> BNX2_DRV_MSG_CODE_KEEP_VLAN_UPDATE, 0, 1);
>
> +     if (bp->link_up)
> +             netif_carrier_on(bp->dev);

Thanks Stanislaw, I think it is better to turn carrier on in
bnx2_netif_start().  We can use the start_cnic parameter to
decide if we need to call carrier_on().  I'll test this out.
Thanks.

>       bnx2_netif_start(bp, false);
>  }
>  #endif
> --
> 1.5.5.6
>
>
>
>

Re: [RFC PATCH] bnx2: use netif_carrier_off to prevent tx timeout

[Stanislaw Gruszka]

On Wed, 12 May 2010 06:31:52 -0700
"Michael Chan" <mchan@broadcom.com> wrote:

> > @@ -6346,6 +6338,8 @@ bnx2_vlan_rx_register(struct net_device
> > *dev, struct vlan_group *vlgrp)
> >       if (bp->flags & BNX2_FLAG_CAN_KEEP_VLAN)
> >               bnx2_fw_sync(bp,
> > BNX2_DRV_MSG_CODE_KEEP_VLAN_UPDATE, 0, 1);
> >
> > +     if (bp->link_up)
> > +             netif_carrier_on(bp->dev);
> 
> Thanks Stanislaw, I think it is better to turn carrier on in
> bnx2_netif_start().  We can use the start_cnic parameter to
> decide if we need to call carrier_on().

IIRC in most cases we set carrier status in bnx2_init_nic()
(based on what we get from PHY) called before bnx2_netif_start().
One exception is bnx2_vlan_rx_register() function.

Thanks
Stanislaw

Re: [RFC PATCH] bnx2: use netif_carrier_off to prevent tx timeout

[Michael Chan]

Stanislaw Gruszka wrote:

> On Wed, 12 May 2010 06:31:52 -0700
> "Michael Chan" <mchan@broadcom.com> wrote:
>
> > > @@ -6346,6 +6338,8 @@ bnx2_vlan_rx_register(struct net_device
> > > *dev, struct vlan_group *vlgrp)
> > >       if (bp->flags & BNX2_FLAG_CAN_KEEP_VLAN)
> > >               bnx2_fw_sync(bp,
> > > BNX2_DRV_MSG_CODE_KEEP_VLAN_UPDATE, 0, 1);
> > >
> > > +     if (bp->link_up)
> > > +             netif_carrier_on(bp->dev);
> >
> > Thanks Stanislaw, I think it is better to turn carrier on in
> > bnx2_netif_start().  We can use the start_cnic parameter to
> > decide if we need to call carrier_on().
>
> IIRC in most cases we set carrier status in bnx2_init_nic()
> (based on what we get from PHY) called before bnx2_netif_start().
> One exception is bnx2_vlan_rx_register() function.
>

Yes, if we reset the chip and reset the PHY, we'll always get a
link up event and we'll set carrier.  We need to take care of
the cases where the chip is not reset or the phy is not reset.
I think it is better if the caller of bnx2_netif_start() does
not have to worry about setting the carrier.