Message ID | 20100512110921.0e3f45fc@dhcp-lab-109.englab.brq.redhat.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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; > } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)