* [PATCH] net/fec: call netif_carrier_off when not having link @ 2013-07-25 13:27 Uwe Kleine-König 2013-07-25 16:03 ` Stephen Hemminger 2013-07-30 9:29 ` [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link Uwe Kleine-König 0 siblings, 2 replies; 15+ messages in thread From: Uwe Kleine-König @ 2013-07-25 13:27 UTC (permalink / raw) To: netdev Cc: David S. Miller, Fabio Estevam, Frank Li, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner Without this patch I see a machine running an -rt patched Linux being stuck in sch_direct_xmit when it looses link while there is still a packet to be send. In this case the fec_enet_start_xmit routine returns NETDEV_TX_BUSY which makes the network stack reschedule the packet and so sch_direct_xmit calls fec_enet_start_xmit again. The right fix is to tell the network stack that the link was lost (using netif_carrier_off) so that it doesn't even try to get rid of the pending packets. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, this resembles a problem described on the linux-rt mailing list some time ago: http://thread.gmane.org/gmane.linux.rt.user/7815 So I Cc:d the people that participated in that thread. IMHO this is 3.11 material (assuming I did it right) and is even worth backporting as I think the problem exists in mainline, too, just harder to trigger. Best regards Uwe drivers/net/ethernet/freescale/fec_main.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 0642006..631bd5a 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) unsigned short status; unsigned int index; - if (!fep->link) { - /* Link is down or auto-negotiation is in progress. */ - return NETDEV_TX_BUSY; - } - /* Fill in a Tx ring entry */ bdp = fep->cur_tx; @@ -459,6 +454,11 @@ fec_restart(struct net_device *ndev, int duplex) netif_tx_lock_bh(ndev); } + if (!fep->link) + netif_carrier_off(ndev); + else + netif_carrier_on(ndev); + /* Whack a reset. We should wait for this. */ writel(1, fep->hwp + FEC_ECNTRL); udelay(10); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-25 13:27 [PATCH] net/fec: call netif_carrier_off when not having link Uwe Kleine-König @ 2013-07-25 16:03 ` Stephen Hemminger 2013-07-26 9:35 ` Duan Fugang-B38611 2013-07-30 9:29 ` [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link Uwe Kleine-König 1 sibling, 1 reply; 15+ messages in thread From: Stephen Hemminger @ 2013-07-25 16:03 UTC (permalink / raw) To: Uwe Kleine-König Cc: netdev, David S. Miller, Fabio Estevam, Frank Li, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner On Thu, 25 Jul 2013 15:27:55 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 0642006..631bd5a 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) > unsigned short status; > unsigned int index; > > - if (!fep->link) { > - /* Link is down or auto-negotiation is in progress. */ > - return NETDEV_TX_BUSY; > - } > - That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt). If the driver cared to test it (most drivers just let hardware deal with this situation), then it should free packet and return TX_OK. ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-25 16:03 ` Stephen Hemminger @ 2013-07-26 9:35 ` Duan Fugang-B38611 2013-07-26 10:06 ` Florian Fainelli 2013-07-26 15:31 ` Ben Hutchings 0 siblings, 2 replies; 15+ messages in thread From: Duan Fugang-B38611 @ 2013-07-26 9:35 UTC (permalink / raw) To: Stephen Hemminger, Uwe Kleine-König Cc: netdev, David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote: >> diff --git a/drivers/net/ethernet/freescale/fec_main.c >> b/drivers/net/ethernet/freescale/fec_main.c >> index 0642006..631bd5a 100644 >> --- a/drivers/net/ethernet/freescale/fec_main.c >> +++ b/drivers/net/ethernet/freescale/fec_main.c >> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) >> unsigned short status; >> unsigned int index; >> >> - if (!fep->link) { >> - /* Link is down or auto-negotiation is in progress. */ >> - return NETDEV_TX_BUSY; >> - } >> - > >That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt). >If the driver cared to test it (most drivers just let hardware deal with this situation), >then it should free packet and return TX_OK. When link is down, the logic is - call netif_stop_queue() to stop queue - and then notify there have no link using netif_carrier_off(). But the flow must be handled in fec_enet_adjust_link() function, not in xmit(). Thanks, Andy ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-26 9:35 ` Duan Fugang-B38611 @ 2013-07-26 10:06 ` Florian Fainelli 2013-07-26 10:52 ` Sascha Hauer 2013-07-26 15:31 ` Ben Hutchings 1 sibling, 1 reply; 15+ messages in thread From: Florian Fainelli @ 2013-07-26 10:06 UTC (permalink / raw) To: Duan Fugang-B38611 Cc: Stephen Hemminger, Uwe Kleine-König, netdev, David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner Hello, 2013/7/26 Duan Fugang-B38611 <B38611@freescale.com>: > On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote: >>> diff --git a/drivers/net/ethernet/freescale/fec_main.c >>> b/drivers/net/ethernet/freescale/fec_main.c >>> index 0642006..631bd5a 100644 >>> --- a/drivers/net/ethernet/freescale/fec_main.c >>> +++ b/drivers/net/ethernet/freescale/fec_main.c >>> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) >>> unsigned short status; >>> unsigned int index; >>> >>> - if (!fep->link) { >>> - /* Link is down or auto-negotiation is in progress. */ >>> - return NETDEV_TX_BUSY; >>> - } >>> - >> >>That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt). >>If the driver cared to test it (most drivers just let hardware deal with this situation), >>then it should free packet and return TX_OK. > > When link is down, the logic is > - call netif_stop_queue() to stop queue > - and then notify there have no link using netif_carrier_off(). netif_carrier_off() is handled by the PHY library before calling the adjust_link callback in FEC. > > But the flow must be handled in fec_enet_adjust_link() function, not in xmit(). What is special about this hardware that fec_restart() needs to do all that work? Can't you just update the duplex/speed/pause settings directly without doing a full hardware re-init? Also the comment above it is wrong, fec_restart() is not just called when the duplex changes, it is called when the link/speed/duplex settings changed and when the workqueue is scheduled from fec_timeout(). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-26 10:06 ` Florian Fainelli @ 2013-07-26 10:52 ` Sascha Hauer 0 siblings, 0 replies; 15+ messages in thread From: Sascha Hauer @ 2013-07-26 10:52 UTC (permalink / raw) To: Florian Fainelli Cc: Duan Fugang-B38611, Stephen Hemminger, Uwe Kleine-König, netdev, David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner On Fri, Jul 26, 2013 at 11:06:06AM +0100, Florian Fainelli wrote: > Hello, > > 2013/7/26 Duan Fugang-B38611 <B38611@freescale.com>: > > On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote: > >>> diff --git a/drivers/net/ethernet/freescale/fec_main.c > >>> b/drivers/net/ethernet/freescale/fec_main.c > >>> index 0642006..631bd5a 100644 > >>> --- a/drivers/net/ethernet/freescale/fec_main.c > >>> +++ b/drivers/net/ethernet/freescale/fec_main.c > >>> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) > >>> unsigned short status; > >>> unsigned int index; > >>> > >>> - if (!fep->link) { > >>> - /* Link is down or auto-negotiation is in progress. */ > >>> - return NETDEV_TX_BUSY; > >>> - } > >>> - > >> > >>That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt). > >>If the driver cared to test it (most drivers just let hardware deal with this situation), > >>then it should free packet and return TX_OK. > > > > When link is down, the logic is > > - call netif_stop_queue() to stop queue > > - and then notify there have no link using netif_carrier_off(). > > netif_carrier_off() is handled by the PHY library before calling the > adjust_link callback in FEC. > > > > > But the flow must be handled in fec_enet_adjust_link() function, not in xmit(). > > What is special about this hardware that fec_restart() needs to do all > that work? The special thing is that the driver is written in a braindamaged way. fec_restart is kind of a suicide-and-resurrect function which is called whenever something changes. > Can't you just update the duplex/speed/pause settings > directly without doing a full hardware re-init? Yes, that's possible. I made some initial patches for this which we'll probably send soon. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-26 9:35 ` Duan Fugang-B38611 2013-07-26 10:06 ` Florian Fainelli @ 2013-07-26 15:31 ` Ben Hutchings 2013-07-29 2:10 ` Duan Fugang-B38611 2013-07-29 2:14 ` Duan Fugang-B38611 1 sibling, 2 replies; 15+ messages in thread From: Ben Hutchings @ 2013-07-26 15:31 UTC (permalink / raw) To: Duan Fugang-B38611 Cc: Stephen Hemminger, Uwe Kleine-König, netdev, David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner On Fri, 2013-07-26 at 09:35 +0000, Duan Fugang-B38611 wrote: > On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote: > >> diff --git a/drivers/net/ethernet/freescale/fec_main.c > >> b/drivers/net/ethernet/freescale/fec_main.c > >> index 0642006..631bd5a 100644 > >> --- a/drivers/net/ethernet/freescale/fec_main.c > >> +++ b/drivers/net/ethernet/freescale/fec_main.c > >> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) > >> unsigned short status; > >> unsigned int index; > >> > >> - if (!fep->link) { > >> - /* Link is down or auto-negotiation is in progress. */ > >> - return NETDEV_TX_BUSY; > >> - } > >> - > > > >That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt). > >If the driver cared to test it (most drivers just let hardware deal with this situation), > >then it should free packet and return TX_OK. > > When link is down, the logic is > - call netif_stop_queue() to stop queue > - and then notify there have no link using netif_carrier_off(). netif_stop_queue() *must not* be called before netif_carrier_off(), otherwise the TX watchdog can fire immediately. The TX watchdog only knows when the last packet was passed to the driver, not when the queue was stopped. The last packet could have been added an arbitrarily long time before the link went down, therefore it may appear that the timeout has already expired.. Although it is safe to call netif_stop_queue() after netif_carrier_off(), it is not useful. netif_stop_queue() should only be called from your ndo_start_xmit operation and only because the queue is full. Any other reason to stop should be communicated to the kernel using netif_carrier_off() or netif_device_detach(). Ben. > But the flow must be handled in fec_enet_adjust_link() function, not in xmit(). -- Ben Hutchings, Staff Engineer, Solarflare 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 [flat|nested] 15+ messages in thread
* RE: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-26 15:31 ` Ben Hutchings @ 2013-07-29 2:10 ` Duan Fugang-B38611 2013-07-29 2:14 ` Duan Fugang-B38611 1 sibling, 0 replies; 15+ messages in thread From: Duan Fugang-B38611 @ 2013-07-29 2:10 UTC (permalink / raw) To: Ben Hutchings Cc: Stephen Hemminger, Uwe Kleine-König, netdev, David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner >netif_stop_queue() *must not* be called before netif_carrier_off(), otherwise the TX watchdog can fire immediately. >The TX watchdog only knows when the last packet was passed to the driver, not when the queue was stopped. >The last packet could have been added an arbitrarily long time before the link went down, therefore it may appear that the timeout has already expired.. > >Although it is safe to call netif_stop_queue() after netif_carrier_off(), it is not useful. >netif_stop_queue() should only be called from your ndo_start_xmit operation and only because the queue is full. >Any other reason to stop should be communicated to the kernel using netif_carrier_off() or netif_device_detach(). > >Ben. Agree. I remember you said: The watchdog fires when the software queue has been stopped *and* the link has been reported as up for over dev->watchdog_timeo ticks. The software queue should be stopped if the hardware queue is full or nearly full. If the software queue remains stopped and the link is still reported up, then one of these things is happening: 1. The link went down but the driver didn't notice, or sent a transmit packet which never completes 2. TX completions are not being indicated or handled correctly 3. The hardware TX path has locked up 4. The link is stalled by excessive pause frames or collisions 5. Timeout is too low and/or low watermark is too high (there may be other explanations) The watchdog is primarily meant to deal with case 3, though all of cases 1-3 may be worked around by resetting the hardware. Thanks, Andy -----Original Message----- From: Ben Hutchings [mailto:bhutchings@solarflare.com] Sent: Friday, July 26, 2013 11:32 PM To: Duan Fugang-B38611 Cc: Stephen Hemminger; Uwe Kleine-König; netdev@vger.kernel.org; David S. Miller; Estevam Fabio-R49496; Li Frank-B20596; Shawn Guo; kernel@pengutronix.de; Hector Palacios; Tim Sander; Steven Rostedt; Thomas Gleixner Subject: Re: [PATCH] net/fec: call netif_carrier_off when not having link On Fri, 2013-07-26 at 09:35 +0000, Duan Fugang-B38611 wrote: > On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote: > >> diff --git a/drivers/net/ethernet/freescale/fec_main.c > >> b/drivers/net/ethernet/freescale/fec_main.c > >> index 0642006..631bd5a 100644 > >> --- a/drivers/net/ethernet/freescale/fec_main.c > >> +++ b/drivers/net/ethernet/freescale/fec_main.c > >> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) > >> unsigned short status; > >> unsigned int index; > >> > >> - if (!fep->link) { > >> - /* Link is down or auto-negotiation is in progress. */ > >> - return NETDEV_TX_BUSY; > >> - } > >> - > > > >That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt). > >If the driver cared to test it (most drivers just let hardware deal > >with this situation), then it should free packet and return TX_OK. > > When link is down, the logic is > - call netif_stop_queue() to stop queue > - and then notify there have no link using netif_carrier_off(). netif_stop_queue() *must not* be called before netif_carrier_off(), otherwise the TX watchdog can fire immediately. The TX watchdog only knows when the last packet was passed to the driver, not when the queue was stopped. The last packet could have been added an arbitrarily long time before the link went down, therefore it may appear that the timeout has already expired.. Although it is safe to call netif_stop_queue() after netif_carrier_off(), it is not useful. netif_stop_queue() should only be called from your ndo_start_xmit operation and only because the queue is full. Any other reason to stop should be communicated to the kernel using netif_carrier_off() or netif_device_detach(). Ben. > But the flow must be handled in fec_enet_adjust_link() function, not in xmit(). -- Ben Hutchings, Staff Engineer, Solarflare 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 [flat|nested] 15+ messages in thread
* RE: [PATCH] net/fec: call netif_carrier_off when not having link 2013-07-26 15:31 ` Ben Hutchings 2013-07-29 2:10 ` Duan Fugang-B38611 @ 2013-07-29 2:14 ` Duan Fugang-B38611 1 sibling, 0 replies; 15+ messages in thread From: Duan Fugang-B38611 @ 2013-07-29 2:14 UTC (permalink / raw) To: Ben Hutchings Cc: Stephen Hemminger, Uwe Kleine-König, netdev, David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner On Fri, 2013-07-26 at 11:32 PM, Ben Hutchings wrote: >netif_stop_queue() *must not* be called before netif_carrier_off(), otherwise the TX watchdog can fire immediately. >The TX watchdog only knows when the last packet was passed to the driver, not when the queue was stopped. >The last packet could have been added an arbitrarily long time before the link went down, therefore it may appear that the timeout has already expired.. > >Although it is safe to call netif_stop_queue() after netif_carrier_off(), it is not useful. >netif_stop_queue() should only be called from your ndo_start_xmit operation and only because the queue is full. >Any other reason to stop should be communicated to the kernel using netif_carrier_off() or netif_device_detach(). > >Ben. Agree. I remember you said: The watchdog fires when the software queue has been stopped *and* the link has been reported as up for over dev->watchdog_timeo ticks. The software queue should be stopped if the hardware queue is full or nearly full. If the software queue remains stopped and the link is still reported up, then one of these things is happening: 1. The link went down but the driver didn't notice, or sent a transmit packet which never completes 2. TX completions are not being indicated or handled correctly 3. The hardware TX path has locked up 4. The link is stalled by excessive pause frames or collisions 5. Timeout is too low and/or low watermark is too high (there may be other explanations) The watchdog is primarily meant to deal with case 3, though all of cases 1-3 may be worked around by resetting the hardware. Thanks, Andy ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-07-25 13:27 [PATCH] net/fec: call netif_carrier_off when not having link Uwe Kleine-König 2013-07-25 16:03 ` Stephen Hemminger @ 2013-07-30 9:29 ` Uwe Kleine-König 2013-07-30 9:59 ` Duan Fugang-B38611 2013-07-30 23:05 ` David Miller 1 sibling, 2 replies; 15+ messages in thread From: Uwe Kleine-König @ 2013-07-30 9:29 UTC (permalink / raw) To: netdev Cc: David S. Miller, Fabio Estevam, Frank Li, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner, Stephen Hemminger, Duan Fugang-B38611, Florian Fainelli, Ben Hutchings Don't test for having link and let hardware deal with this situation. Without this patch I see a machine running an -rt patched Linux being stuck in sch_direct_xmit when it looses link while there is still a packet to be sent. In this case the fec_enet_start_xmit routine returned NETDEV_TX_BUSY which makes the network stack reschedule the packet and so sch_direct_xmit calls fec_enet_start_xmit again. I failed to reproduce a complete hang without -rt, but I think the problem exists there, too. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, this is a new variant of "[PATCH] net/fec: call netif_carrier_off when not having link" that drops calling netif_carrier_{on,off} in fec_restart because the phy library already handles that. I think this is 3.11 material assuming you are ok with the patch now. Best regards Uwe drivers/net/ethernet/freescale/fec_main.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index ed61160..792d729 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev) unsigned short status; unsigned int index; - if (!fep->link) { - /* Link is down or auto-negotiation is in progress. */ - return NETDEV_TX_BUSY; - } - /* Fill in a Tx ring entry */ bdp = fep->cur_tx; -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-07-30 9:29 ` [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link Uwe Kleine-König @ 2013-07-30 9:59 ` Duan Fugang-B38611 2013-07-30 23:05 ` David Miller 1 sibling, 0 replies; 15+ messages in thread From: Duan Fugang-B38611 @ 2013-07-30 9:59 UTC (permalink / raw) To: Uwe Kleine-König, netdev Cc: David S. Miller, Estevam Fabio-R49496, Li Frank-B20596, Shawn Guo, kernel, Hector Palacios, Tim Sander, Steven Rostedt, Thomas Gleixner, Stephen Hemminger, Florian Fainelli, Ben Hutchings On Tue, Jul 30, 2013 at 05:30 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: >Don't test for having link and let hardware deal with this situation. > >Without this patch I see a machine running an -rt patched Linux being stuck in sch_direct_xmit when it looses link while there is still a packet to be sent. >In this case the fec_enet_start_xmit routine returned NETDEV_TX_BUSY which makes the network stack reschedule the packet and so sch_direct_xmit calls >fec_enet_start_xmit again. >I failed to reproduce a complete hang without -rt, but I think the problem exists there, too. > >Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> Acked-and-tested-by: Fugang Duan <B38611@freescale.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-07-30 9:29 ` [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link Uwe Kleine-König 2013-07-30 9:59 ` Duan Fugang-B38611 @ 2013-07-30 23:05 ` David Miller 2013-07-31 8:16 ` Uwe Kleine-König 1 sibling, 1 reply; 15+ messages in thread From: David Miller @ 2013-07-30 23:05 UTC (permalink / raw) To: u.kleine-koenig Cc: netdev, fabio.estevam, Frank.Li, shawn.guo, kernel, hector.palacios, tim.sander, rostedt, tglx, stephen, B38611, florian, bhutchings From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Date: Tue, 30 Jul 2013 11:29:40 +0200 > Don't test for having link and let hardware deal with this situation. > > Without this patch I see a machine running an -rt patched Linux being > stuck in sch_direct_xmit when it looses link while there is still a > packet to be sent. In this case the fec_enet_start_xmit routine returned > NETDEV_TX_BUSY which makes the network stack reschedule the packet and > so sch_direct_xmit calls fec_enet_start_xmit again. > I failed to reproduce a complete hang without -rt, but I think the > problem exists there, too. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Applied, thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-07-30 23:05 ` David Miller @ 2013-07-31 8:16 ` Uwe Kleine-König 2013-07-31 18:51 ` David Miller 0 siblings, 1 reply; 15+ messages in thread From: Uwe Kleine-König @ 2013-07-31 8:16 UTC (permalink / raw) To: David Miller Cc: netdev, fabio.estevam, Frank.Li, shawn.guo, kernel, hector.palacios, tim.sander, rostedt, tglx, stephen, B38611, florian, bhutchings On Tue, Jul 30, 2013 at 04:05:30PM -0700, David Miller wrote: > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Date: Tue, 30 Jul 2013 11:29:40 +0200 > > > Don't test for having link and let hardware deal with this situation. > > > > Without this patch I see a machine running an -rt patched Linux being > > stuck in sch_direct_xmit when it looses link while there is still a > > packet to be sent. In this case the fec_enet_start_xmit routine returned > > NETDEV_TX_BUSY which makes the network stack reschedule the packet and > > so sch_direct_xmit calls fec_enet_start_xmit again. > > I failed to reproduce a complete hang without -rt, but I think the > > problem exists there, too. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Applied, thanks. Thanks, I see it as a264b981f2c76e281ef27e7232774bf6c54ec865 in net.git (i.e. https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=a264b981f2c76e281ef27e7232774bf6c54ec865) You missed to take the Acked-and-tested-by: Fugang Duan <B38611@freescale.com> . If you want to fix that, you can do: git filter-branch --msg-filter 'if test $GIT_COMMIT = a264b981f2c76e281ef27e7232774bf6c54ec865; then sed "/David/ i Acked-and-tested-by: Fugang Duan <B38611@freescale.com>"; else cat; fi' a264b981f2c76e281^.. on your master branch to add it to that commit. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-07-31 8:16 ` Uwe Kleine-König @ 2013-07-31 18:51 ` David Miller 2013-08-01 2:01 ` Duan Fugang-B38611 0 siblings, 1 reply; 15+ messages in thread From: David Miller @ 2013-07-31 18:51 UTC (permalink / raw) To: u.kleine-koenig Cc: netdev, fabio.estevam, Frank.Li, shawn.guo, kernel, hector.palacios, tim.sander, rostedt, tglx, stephen, B38611, florian, bhutchings From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Date: Wed, 31 Jul 2013 10:16:49 +0200 > On Tue, Jul 30, 2013 at 04:05:30PM -0700, David Miller wrote: >> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> Date: Tue, 30 Jul 2013 11:29:40 +0200 >> >> > Don't test for having link and let hardware deal with this situation. >> > >> > Without this patch I see a machine running an -rt patched Linux being >> > stuck in sch_direct_xmit when it looses link while there is still a >> > packet to be sent. In this case the fec_enet_start_xmit routine returned >> > NETDEV_TX_BUSY which makes the network stack reschedule the packet and >> > so sch_direct_xmit calls fec_enet_start_xmit again. >> > I failed to reproduce a complete hang without -rt, but I think the >> > problem exists there, too. >> > >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> >> Applied, thanks. > Thanks, I see it as a264b981f2c76e281ef27e7232774bf6c54ec865 in net.git > (i.e. > https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=a264b981f2c76e281ef27e7232774bf6c54ec865) > > You missed to take the > > Acked-and-tested-by: Fugang Duan <B38611@freescale.com> This is not an official tag recognized by patchwork, he should say: Acked-by: Fugang Duan <B38611@freescale.com> Tested-by: Fugang Duan <B38611@freescale.com> > . If you want to fix that, you can do: > > git filter-branch --msg-filter 'if test $GIT_COMMIT = a264b981f2c76e281ef27e7232774bf6c54ec865; then sed "/David/ i Acked-and-tested-by: Fugang Duan <B38611@freescale.com>"; else cat; fi' a264b981f2c76e281^.. > > on your master branch to add it to that commit. My GIT history is permanent once pushed out, I cannot change history or else it would break everyone cloning my tree. ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-07-31 18:51 ` David Miller @ 2013-08-01 2:01 ` Duan Fugang-B38611 2013-08-01 2:25 ` Steven Rostedt 0 siblings, 1 reply; 15+ messages in thread From: Duan Fugang-B38611 @ 2013-08-01 2:01 UTC (permalink / raw) To: David Miller, u.kleine-koenig Cc: netdev, Estevam Fabio-R49496, Li Frank-B20596, shawn.guo, kernel, hector.palacios, tim.sander, rostedt, tglx, stephen, florian, bhutchings On Thu, Aug 01, 2013 at 02:51 AM, David Miller wrote: >> On Tue, Jul 30, 2013 at 04:05:30PM -0700, David Miller wrote: >>> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> Date: Tue, 30 Jul 2013 11:29:40 +0200 >>> >>> > Don't test for having link and let hardware deal with this situation. >>> > >>> > Without this patch I see a machine running an -rt patched Linux >>> > being stuck in sch_direct_xmit when it looses link while there is >>> > still a packet to be sent. In this case the fec_enet_start_xmit >>> > routine returned NETDEV_TX_BUSY which makes the network stack >>> > reschedule the packet and so sch_direct_xmit calls fec_enet_start_xmit again. >>> > I failed to reproduce a complete hang without -rt, but I think the >>> > problem exists there, too. >>> > >>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> >>> Applied, thanks. >> Thanks, I see it as a264b981f2c76e281ef27e7232774bf6c54ec865 in >> net.git (i.e. >> https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id= >> a264b981f2c76e281ef27e7232774bf6c54ec865) >> >> You missed to take the >> >> Acked-and-tested-by: Fugang Duan <B38611@freescale.com> > > This is not an official tag recognized by patchwork, he should say: > > Acked-by: Fugang Duan <B38611@freescale.com> > Tested-by: Fugang Duan <B38611@freescale.com> I see Linux kernel trees have many non-official tag such as: Acked-and-tested-by. Anyway, we must follow the official rules. Thanks, David. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link 2013-08-01 2:01 ` Duan Fugang-B38611 @ 2013-08-01 2:25 ` Steven Rostedt 0 siblings, 0 replies; 15+ messages in thread From: Steven Rostedt @ 2013-08-01 2:25 UTC (permalink / raw) To: Duan Fugang-B38611 Cc: David Miller, u.kleine-koenig, netdev, Estevam Fabio-R49496, Li Frank-B20596, shawn.guo, kernel, hector.palacios, tim.sander, tglx, stephen, florian, bhutchings On Thu, 2013-08-01 at 02:01 +0000, Duan Fugang-B38611 wrote: > > This is not an official tag recognized by patchwork, he should say: > > > > Acked-by: Fugang Duan <B38611@freescale.com> > > Tested-by: Fugang Duan <B38611@freescale.com> > > I see Linux kernel trees have many non-official tag such as: Acked-and-tested-by. Yeah, that was done before, but it's been stated several times that we want to conform to one tag per line. Makes parsing with tools much easier. -- Steve ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-08-01 2:25 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-25 13:27 [PATCH] net/fec: call netif_carrier_off when not having link Uwe Kleine-König 2013-07-25 16:03 ` Stephen Hemminger 2013-07-26 9:35 ` Duan Fugang-B38611 2013-07-26 10:06 ` Florian Fainelli 2013-07-26 10:52 ` Sascha Hauer 2013-07-26 15:31 ` Ben Hutchings 2013-07-29 2:10 ` Duan Fugang-B38611 2013-07-29 2:14 ` Duan Fugang-B38611 2013-07-30 9:29 ` [PATCH] net/fec: Don't let ndo_start_xmit return NETDEV_TX_BUSY without link Uwe Kleine-König 2013-07-30 9:59 ` Duan Fugang-B38611 2013-07-30 23:05 ` David Miller 2013-07-31 8:16 ` Uwe Kleine-König 2013-07-31 18:51 ` David Miller 2013-08-01 2:01 ` Duan Fugang-B38611 2013-08-01 2:25 ` Steven Rostedt
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).