Re: [PATCH] e100_shutdown: netif_poll_disable hang

Re: [PATCH] e100_shutdown: netif_poll_disable hang

[Auke Kok]

Daniel Walker wrote:
> My machine annoyingly hangs while rebooting. I tracked it down
> to e100-fix-reboot-f-with-netconsole-enabled.patch in 2.6.18-rc2-mm2
> 
> I review the changes and it seemed to be calling netif_poll_disable
> one too many time. Once in e100_down(), and again in e100_shutdown().
> 
> The second one in e100_shutdown() caused the hang. So this patch
> removes it. 
> 
> Signed-Off-By: Daniel Walker <dwalker@mvista.com>
> 
> ---
>  drivers/net/e100.c |    1 -
>  1 files changed, 1 deletion(-)
> 
> Index: linux-2.6.18/drivers/net/e100.c
> ===================================================================
> --- linux-2.6.18.orig/drivers/net/e100.c
> +++ linux-2.6.18/drivers/net/e100.c
> @@ -2709,7 +2709,6 @@ static void e100_shutdown(struct pci_dev
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct nic *nic = netdev_priv(netdev);
>  
> -	netif_poll_disable(nic->netdev);
>  	del_timer_sync(&nic->watchdog);
>  	netif_carrier_off(nic->netdev);
>  
> --

this won't help netconsole shutdown/reboot -f, probably locking it up again!

NAK

Also missing is the NAPI conditional, which I left out. Also missing is the same code 
for suspend.

Here's a better approach, allowing both normal shutdown code path and reboot -f to 
disable polling.

Cheers,

Auke
---

e100: disable polling only when up during suspend and shutdown

Signed-off-by: Auke Kok <auke-jan.h.kok>
Cc: Daniel Walker <dwalker@mvista.com>
Cc: Andrew Morton <akpm@osdl.org>

---
  drivers/net/e100.c |   10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

---
diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index d4a2572..815eb29 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -2719,7 +2719,10 @@ static int e100_suspend(struct pci_dev *
  	struct net_device *netdev = pci_get_drvdata(pdev);
  	struct nic *nic = netdev_priv(netdev);

-	netif_poll_disable(nic->netdev);
+#ifdef CONFIG_E100_NAPI
+	if (netif_running(netdev))
+		netif_poll_disable(nic->netdev);
+#endif
  	del_timer_sync(&nic->watchdog);
  	netif_carrier_off(nic->netdev);

@@ -2763,7 +2766,10 @@ static void e100_shutdown(struct pci_dev
  	struct net_device *netdev = pci_get_drvdata(pdev);
  	struct nic *nic = netdev_priv(netdev);

-	netif_poll_disable(nic->netdev);
+#ifdef CONFIG_E100_NAPI
+	if (netif_running(netdev))
+		netif_poll_disable(nic->netdev);
+#endif
  	del_timer_sync(&nic->watchdog);
  	netif_carrier_off(nic->netdev);

Re: [PATCH] e100_shutdown: netif_poll_disable hang

[Auke Kok]

Auke Kok wrote:
> Daniel Walker wrote:
>> My machine annoyingly hangs while rebooting. I tracked it down
>> to e100-fix-reboot-f-with-netconsole-enabled.patch in 2.6.18-rc2-mm2
>>
>> I review the changes and it seemed to be calling netif_poll_disable
>> one too many time. Once in e100_down(), and again in e100_shutdown().
>>
>> The second one in e100_shutdown() caused the hang. So this patch
>> removes it.

it doesn't even do harm to netif_poll_disable() twice as far as I can see, as it merely 
calls test_and_set_bit(), which will instantly succeed on the first attempt if the bit 
was already set.

did this change actually fix it for you? I'm wondering if the netif_carrier_off might 
not be the culprit here...

Auke

Re: [PATCH] e100_shutdown: netif_poll_disable hang

[Damien Wyart]

> > > My machine annoyingly hangs while rebooting. I tracked it down to
> > > e100-fix-reboot-f-with-netconsole-enabled.patch in 2.6.18-rc2-mm2
> > > I review the changes and it seemed to be calling
> > > netif_poll_disable one too many time. Once in e100_down(), and
> > > again in e100_shutdown().
> > > The second one in e100_shutdown() caused the hang. So this patch
> > > removes it.

* Auke Kok <auke-jan.h.kok@intel.com> [061020 23:09]:
> it doesn't even do harm to netif_poll_disable() twice as far as I can
> see, as it merely calls test_and_set_bit(), which will instantly
> succeed on the first attempt if the bit was already set.

> did this change actually fix it for you? I'm wondering if the
> netif_carrier_off might not be the culprit here...

I can confirm the proposed original change of D. Walker fixed the
problem for me. I did not test the change you proposed as a followup.

-- 
Damien Wyart

Re: [PATCH] e100_shutdown: netif_poll_disable hang

[Auke Kok]

Damien Wyart wrote:
>>>> My machine annoyingly hangs while rebooting. I tracked it down to
>>>> e100-fix-reboot-f-with-netconsole-enabled.patch in 2.6.18-rc2-mm2
>>>> I review the changes and it seemed to be calling
>>>> netif_poll_disable one too many time. Once in e100_down(), and
>>>> again in e100_shutdown().
>>>> The second one in e100_shutdown() caused the hang. So this patch
>>>> removes it.
> 
> * Auke Kok <auke-jan.h.kok@intel.com> [061020 23:09]:
>> it doesn't even do harm to netif_poll_disable() twice as far as I can
>> see, as it merely calls test_and_set_bit(), which will instantly
>> succeed on the first attempt if the bit was already set.
> 
>> did this change actually fix it for you? I'm wondering if the
>> netif_carrier_off might not be the culprit here...
> 
> I can confirm the proposed original change of D. Walker fixed the
> problem for me. I did not test the change you proposed as a followup.

his change breaks something else (a reboot with netconsole, possibly suspend). Please 
give the latest version I sent a try. Daniel confirmed me that it works, but it's always 
nice to hear it from more people.

Thanks

Auke

Re: [PATCH] e100_shutdown: netif_poll_disable hang

[Damien Wyart]

* Auke Kok <auke-jan.h.kok@intel.com> [2006-10-21 10:54]: his change
> breaks something else (a reboot with netconsole, possibly suspend).
> Please give the latest version I sent a try. Daniel confirmed me that
> it works, but it's always nice to hear it from more people.

Yes, your version works here too.

-- 
Damien Wyart