[2/2] e1000e: start network tx queue only when link is up
diff mbox series

Message ID 155548880068.3454.1621821881762819373.stgit@buzz
State Accepted
Delegated to: Jeff Kirsher
Headers show
Series
  • [1/2] Revert "e1000e: fix cyclic resets at link up with active tx"
Related show

Commit Message

Konstantin Khlebnikov April 17, 2019, 8:13 a.m. UTC
Driver does not want to keep packets in tx queue when link is lost.
But present code only reset NIC to flush them, but does not prevent
queuing new packets. Moreover reset sequence itself could generate
new packets via netconsole and NIC falls into endless reset loop.

This patch wakes tx queue only when NIC is ready to send packets.

This is proper fix for problem addressed by commit 0f9e980bf5ee
("e1000e: fix cyclic resets at link up with active tx").

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Joseph Yasi April 23, 2019, 12:49 a.m. UTC | #1
On Wed, Apr 17, 2019 at 4:13 AM Konstantin Khlebnikov <
khlebnikov@yandex-team.ru> wrote:

> Driver does not want to keep packets in tx queue when link is lost.
> But present code only reset NIC to flush them, but does not prevent
> queuing new packets. Moreover reset sequence itself could generate
> new packets via netconsole and NIC falls into endless reset loop.
>
> This patch wakes tx queue only when NIC is ready to send packets.
>
> This is proper fix for problem addressed by commit 0f9e980bf5ee
> ("e1000e: fix cyclic resets at link up with active tx").
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
>
Tested-by: Joseph Yasi <joe.yasi@gmail.com>

> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> b/drivers/net/ethernet/intel/e1000e/netdev.c
> index ba96e52aa8d1..fe643d66aa10 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4209,7 +4209,7 @@ void e1000e_up(struct e1000_adapter *adapter)
>                 e1000_configure_msix(adapter);
>         e1000_irq_enable(adapter);
>
> -       netif_start_queue(adapter->netdev);
> +       /* tx queue started by watchdog timer when link is up */
>
>         e1000e_trigger_lsc(adapter);
>  }
> @@ -4607,6 +4607,7 @@ int e1000e_open(struct net_device *netdev)
>         pm_runtime_get_sync(&pdev->dev);
>
>         netif_carrier_off(netdev);
> +       netif_stop_queue(netdev);
>
>         /* allocate transmit descriptors */
>         err = e1000e_setup_tx_resources(adapter->tx_ring);
> @@ -4667,7 +4668,6 @@ int e1000e_open(struct net_device *netdev)
>         e1000_irq_enable(adapter);
>
>         adapter->tx_hang_recheck = false;
> -       netif_start_queue(netdev);
>
>         hw->mac.get_link_status = true;
>         pm_runtime_put(&pdev->dev);
> @@ -5289,6 +5289,7 @@ static void e1000_watchdog_task(struct work_struct
> *work)
>                         if (phy->ops.cfg_on_link_up)
>                                 phy->ops.cfg_on_link_up(hw);
>
> +                       netif_wake_queue(netdev);
>                         netif_carrier_on(netdev);
>
>                         if (!test_bit(__E1000_DOWN, &adapter->state))
> @@ -5302,6 +5303,7 @@ static void e1000_watchdog_task(struct work_struct
> *work)
>                         /* Link status message must follow this format */
>                         pr_info("%s NIC Link is Down\n",
> adapter->netdev->name);
>                         netif_carrier_off(netdev);
> +                       netif_stop_queue(netdev);
>                         if (!test_bit(__E1000_DOWN, &adapter->state))
>                                 mod_timer(&adapter->phy_info_timer,
>                                           round_jiffies(jiffies + 2 * HZ));
>
>
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 17, 2019 at 4:13 AM Konstantin Khlebnikov &lt;<a href="mailto:khlebnikov@yandex-team.ru">khlebnikov@yandex-team.ru</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Driver does not want to keep packets in tx queue when link is lost.<br>
But present code only reset NIC to flush them, but does not prevent<br>
queuing new packets. Moreover reset sequence itself could generate<br>
new packets via netconsole and NIC falls into endless reset loop.<br>
<br>
This patch wakes tx queue only when NIC is ready to send packets.<br>
<br>
This is proper fix for problem addressed by commit 0f9e980bf5ee<br>
(&quot;e1000e: fix cyclic resets at link up with active tx&quot;).<br>
<br>
Signed-off-by: Konstantin Khlebnikov &lt;<a href="mailto:khlebnikov@yandex-team.ru" target="_blank">khlebnikov@yandex-team.ru</a>&gt;<br>
Suggested-by: Alexander Duyck &lt;<a href="mailto:alexander.duyck@gmail.com" target="_blank">alexander.duyck@gmail.com</a>&gt;<br></blockquote><div>Tested-by: Joseph Yasi &lt;<a href="mailto:joe.yasi@gmail.com" target="_blank">joe.yasi@gmail.com</a>&gt; </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
 drivers/net/ethernet/intel/e1000e/netdev.c |    6 ++++--<br>
 1 file changed, 4 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c<br>
index ba96e52aa8d1..fe643d66aa10 100644<br>
--- a/drivers/net/ethernet/intel/e1000e/netdev.c<br>
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c<br>
@@ -4209,7 +4209,7 @@ void e1000e_up(struct e1000_adapter *adapter)<br>
                e1000_configure_msix(adapter);<br>
        e1000_irq_enable(adapter);<br>
<br>
-       netif_start_queue(adapter-&gt;netdev);<br>
+       /* tx queue started by watchdog timer when link is up */<br>
<br>
        e1000e_trigger_lsc(adapter);<br>
 }<br>
@@ -4607,6 +4607,7 @@ int e1000e_open(struct net_device *netdev)<br>
        pm_runtime_get_sync(&amp;pdev-&gt;dev);<br>
<br>
        netif_carrier_off(netdev);<br>
+       netif_stop_queue(netdev);<br>
<br>
        /* allocate transmit descriptors */<br>
        err = e1000e_setup_tx_resources(adapter-&gt;tx_ring);<br>
@@ -4667,7 +4668,6 @@ int e1000e_open(struct net_device *netdev)<br>
        e1000_irq_enable(adapter);<br>
<br>
        adapter-&gt;tx_hang_recheck = false;<br>
-       netif_start_queue(netdev);<br>
<br>
        hw-&gt;mac.get_link_status = true;<br>
        pm_runtime_put(&amp;pdev-&gt;dev);<br>
@@ -5289,6 +5289,7 @@ static void e1000_watchdog_task(struct work_struct *work)<br>
                        if (phy-&gt;ops.cfg_on_link_up)<br>
                                phy-&gt;ops.cfg_on_link_up(hw);<br>
<br>
+                       netif_wake_queue(netdev);<br>
                        netif_carrier_on(netdev);<br>
<br>
                        if (!test_bit(__E1000_DOWN, &amp;adapter-&gt;state))<br>
@@ -5302,6 +5303,7 @@ static void e1000_watchdog_task(struct work_struct *work)<br>
                        /* Link status message must follow this format */<br>
                        pr_info(&quot;%s NIC Link is Down\n&quot;, adapter-&gt;netdev-&gt;name);<br>
                        netif_carrier_off(netdev);<br>
+                       netif_stop_queue(netdev);<br>
                        if (!test_bit(__E1000_DOWN, &amp;adapter-&gt;state))<br>
                                mod_timer(&amp;adapter-&gt;phy_info_timer,<br>
                                          round_jiffies(jiffies + 2 * HZ));<br>
<br>
</blockquote></div></div>
Joseph Yasi April 23, 2019, 12:50 a.m. UTC | #2
On Wed, Apr 17, 2019 at 4:13 AM Konstantin Khlebnikov
<khlebnikov@yandex-team.ru> wrote:
>
> Driver does not want to keep packets in tx queue when link is lost.
> But present code only reset NIC to flush them, but does not prevent
> queuing new packets. Moreover reset sequence itself could generate
> new packets via netconsole and NIC falls into endless reset loop.
>
> This patch wakes tx queue only when NIC is ready to send packets.
>
> This is proper fix for problem addressed by commit 0f9e980bf5ee
> ("e1000e: fix cyclic resets at link up with active tx").
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
Tested-by: Joseph Yasi <joe.yasi@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index ba96e52aa8d1..fe643d66aa10 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4209,7 +4209,7 @@ void e1000e_up(struct e1000_adapter *adapter)
>                 e1000_configure_msix(adapter);
>         e1000_irq_enable(adapter);
>
> -       netif_start_queue(adapter->netdev);
> +       /* tx queue started by watchdog timer when link is up */
>
>         e1000e_trigger_lsc(adapter);
>  }
> @@ -4607,6 +4607,7 @@ int e1000e_open(struct net_device *netdev)
>         pm_runtime_get_sync(&pdev->dev);
>
>         netif_carrier_off(netdev);
> +       netif_stop_queue(netdev);
>
>         /* allocate transmit descriptors */
>         err = e1000e_setup_tx_resources(adapter->tx_ring);
> @@ -4667,7 +4668,6 @@ int e1000e_open(struct net_device *netdev)
>         e1000_irq_enable(adapter);
>
>         adapter->tx_hang_recheck = false;
> -       netif_start_queue(netdev);
>
>         hw->mac.get_link_status = true;
>         pm_runtime_put(&pdev->dev);
> @@ -5289,6 +5289,7 @@ static void e1000_watchdog_task(struct work_struct *work)
>                         if (phy->ops.cfg_on_link_up)
>                                 phy->ops.cfg_on_link_up(hw);
>
> +                       netif_wake_queue(netdev);
>                         netif_carrier_on(netdev);
>
>                         if (!test_bit(__E1000_DOWN, &adapter->state))
> @@ -5302,6 +5303,7 @@ static void e1000_watchdog_task(struct work_struct *work)
>                         /* Link status message must follow this format */
>                         pr_info("%s NIC Link is Down\n", adapter->netdev->name);
>                         netif_carrier_off(netdev);
> +                       netif_stop_queue(netdev);
>                         if (!test_bit(__E1000_DOWN, &adapter->state))
>                                 mod_timer(&adapter->phy_info_timer,
>                                           round_jiffies(jiffies + 2 * HZ));
>
Brown, Aaron F April 26, 2019, 12:05 a.m. UTC | #3
> From: Konstantin Khlebnikov [mailto:khlebnikov@yandex-team.ru]
> Sent: Wednesday, April 17, 2019 1:13 AM
> To: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; linux-
> kernel@vger.kernel.org; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: Sasha Levin <sashal@kernel.org>; Joseph Yasi <joe.yasi@gmail.com>;
> Brown, Aaron F <aaron.f.brown@intel.com>; Alexander Duyck
> <alexander.duyck@gmail.com>; e1000-devel@lists.sourceforge.net
> Subject: [PATCH 2/2] e1000e: start network tx queue only when link is up
> 
> Driver does not want to keep packets in tx queue when link is lost.
> But present code only reset NIC to flush them, but does not prevent
> queuing new packets. Moreover reset sequence itself could generate
> new packets via netconsole and NIC falls into endless reset loop.
> 
> This patch wakes tx queue only when NIC is ready to send packets.
> 
> This is proper fix for problem addressed by commit 0f9e980bf5ee
> ("e1000e: fix cyclic resets at link up with active tx").
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Again, more of a regression check than a test that the patch solves the problem as I did not manage to trigger the hang.
Oleksandr Natalenko April 26, 2019, 2:12 p.m. UTC | #4
On Wed, Apr 17, 2019 at 11:13:20AM +0300, Konstantin Khlebnikov wrote:
> Driver does not want to keep packets in tx queue when link is lost.
> But present code only reset NIC to flush them, but does not prevent
> queuing new packets. Moreover reset sequence itself could generate
> new packets via netconsole and NIC falls into endless reset loop.
> 
> This patch wakes tx queue only when NIC is ready to send packets.
> 
> This is proper fix for problem addressed by commit 0f9e980bf5ee
> ("e1000e: fix cyclic resets at link up with active tx").
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index ba96e52aa8d1..fe643d66aa10 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -4209,7 +4209,7 @@ void e1000e_up(struct e1000_adapter *adapter)
>  		e1000_configure_msix(adapter);
>  	e1000_irq_enable(adapter);
>  
> -	netif_start_queue(adapter->netdev);
> +	/* tx queue started by watchdog timer when link is up */
>  
>  	e1000e_trigger_lsc(adapter);
>  }
> @@ -4607,6 +4607,7 @@ int e1000e_open(struct net_device *netdev)
>  	pm_runtime_get_sync(&pdev->dev);
>  
>  	netif_carrier_off(netdev);
> +	netif_stop_queue(netdev);
>  
>  	/* allocate transmit descriptors */
>  	err = e1000e_setup_tx_resources(adapter->tx_ring);
> @@ -4667,7 +4668,6 @@ int e1000e_open(struct net_device *netdev)
>  	e1000_irq_enable(adapter);
>  
>  	adapter->tx_hang_recheck = false;
> -	netif_start_queue(netdev);
>  
>  	hw->mac.get_link_status = true;
>  	pm_runtime_put(&pdev->dev);
> @@ -5289,6 +5289,7 @@ static void e1000_watchdog_task(struct work_struct *work)
>  			if (phy->ops.cfg_on_link_up)
>  				phy->ops.cfg_on_link_up(hw);
>  
> +			netif_wake_queue(netdev);
>  			netif_carrier_on(netdev);
>  
>  			if (!test_bit(__E1000_DOWN, &adapter->state))
> @@ -5302,6 +5303,7 @@ static void e1000_watchdog_task(struct work_struct *work)
>  			/* Link status message must follow this format */
>  			pr_info("%s NIC Link is Down\n", adapter->netdev->name);
>  			netif_carrier_off(netdev);
> +			netif_stop_queue(netdev);
>  			if (!test_bit(__E1000_DOWN, &adapter->state))
>  				mod_timer(&adapter->phy_info_timer,
>  					  round_jiffies(jiffies + 2 * HZ));
> 

Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>

Patch
diff mbox series

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index ba96e52aa8d1..fe643d66aa10 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4209,7 +4209,7 @@  void e1000e_up(struct e1000_adapter *adapter)
 		e1000_configure_msix(adapter);
 	e1000_irq_enable(adapter);
 
-	netif_start_queue(adapter->netdev);
+	/* tx queue started by watchdog timer when link is up */
 
 	e1000e_trigger_lsc(adapter);
 }
@@ -4607,6 +4607,7 @@  int e1000e_open(struct net_device *netdev)
 	pm_runtime_get_sync(&pdev->dev);
 
 	netif_carrier_off(netdev);
+	netif_stop_queue(netdev);
 
 	/* allocate transmit descriptors */
 	err = e1000e_setup_tx_resources(adapter->tx_ring);
@@ -4667,7 +4668,6 @@  int e1000e_open(struct net_device *netdev)
 	e1000_irq_enable(adapter);
 
 	adapter->tx_hang_recheck = false;
-	netif_start_queue(netdev);
 
 	hw->mac.get_link_status = true;
 	pm_runtime_put(&pdev->dev);
@@ -5289,6 +5289,7 @@  static void e1000_watchdog_task(struct work_struct *work)
 			if (phy->ops.cfg_on_link_up)
 				phy->ops.cfg_on_link_up(hw);
 
+			netif_wake_queue(netdev);
 			netif_carrier_on(netdev);
 
 			if (!test_bit(__E1000_DOWN, &adapter->state))
@@ -5302,6 +5303,7 @@  static void e1000_watchdog_task(struct work_struct *work)
 			/* Link status message must follow this format */
 			pr_info("%s NIC Link is Down\n", adapter->netdev->name);
 			netif_carrier_off(netdev);
+			netif_stop_queue(netdev);
 			if (!test_bit(__E1000_DOWN, &adapter->state))
 				mod_timer(&adapter->phy_info_timer,
 					  round_jiffies(jiffies + 2 * HZ));