Skip to contentSkip to conversation

e1000: Don't enable polling in open() (was: e1000: assertion hit in e1000_clean(), kernel 2.6.21.1)

PATCH Applied Auke Kok 7 participants 16 messages May 21, 2007 – May 24, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Auke Kok

author
#0 lore raw

PATCH 1/1 e1000: Don't enable polling in open() (was: e1000: assertion hit in e1000_clean(), kernel 2.6.21.1)

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Tested by Doug Chapman <[email protected]>

Signed-off-by Auke Kok <[email protected]>
Acked-by Herbert Xu <[email protected]>

---

 drivers/net/e1000/e1000_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
drivers/net/e1000/e1000_main.c +0 -4
index 49be393..cbc7feb 100644
@@ -1431,10 +1431,6 @@ e1000_open(struct net_device *netdev)
 	/* From here on the code is the same as e1000_up() */
 	clear_bit(__E1000_DOWN, &adapter->flags);
 
-#ifdef CONFIG_E1000_NAPI
-	netif_poll_enable(netdev);
-#endif
-
 	e1000_irq_enable(adapter);
 
 	/* fire a link status change interrupt to start the watchdog */
#1 lore raw
Quoted 11 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()

Dave

Signature · 7 lines
http://www.codemonkey.org.uk

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
#2 lore raw
Quoted 10 lines · Dave Jones
Quoted 9 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Quoted 7 lines
Quoted 2 lines

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()

Yes we need a different fix for 2.6.21. There e1000_open calls e1000_up which is why we still get the netif_poll_enable.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#3 lore raw
Quoted 14 lines · Herbert Xu
Quoted 13 lines · Dave Jones
Quoted 11 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Quoted 7 lines
Quoted 4 lines

This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()

Yes we need a different fix for 2.6.21. There e1000_open calls e1000_up which is why we still get the netif_poll_enable.

yes, basically they need the patch that introduced(exposed) the problem as well, but that is a rather significant change and kind of moves the whole netstack-init code in e1000 around. The size was the reason why that patch didn't go into 2.6.21 in the first place, but perhaps they can pull both patches into the FC tree.

For reference, this is the commit:

commit e0aac5a289b1dacbc94bd9ae8c449bcdf9ab508c

Author: Auke Kok <[email protected]>
Date:   Tue Mar 6 08:57:21 2007 -0800

e1000: FIX: be ready for incoming irq at pci_request_irq

DEBUG_SHIRQ code exposed that e1000 was not ready for incoming interrupts after having called pci_request_irq. This obviously requires us to finish our software setup which assigns the irq handler before we request the irq.

Signed-off-by Auke Kok <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>

Dave, would that be an option for you?

Auke

#4 lore raw
Quoted 27 lines · Auke
Quoted 7 lines
Quoted 4 lines

This probably doesn't solve the latter bug.
The code you reference isn't there in the kernel tested in that bug
(2.6.21) In 2.6.21, netif_poll_enable is only called from
e1000_up(), not e1000_open()

Yes we need a different fix for 2.6.21. There e1000_open calls e1000_up which is why we still get the netif_poll_enable.

yes, basically they need the patch that introduced(exposed) the problem as well, but that is a rather significant change and kind of moves the whole netstack-init code in e1000 around. The size was the reason why that patch didn't go into 2.6.21 in the first place, but perhaps they can pull both patches into the FC tree.

For reference, this is the commit:

commit e0aac5a289b1dacbc94bd9ae8c449bcdf9ab508c

Author: Auke Kok <[email protected]>
Date:   Tue Mar 6 08:57:21 2007 -0800

e1000: FIX: be ready for incoming irq at pci_request_irq

DEBUG_SHIRQ code exposed that e1000 was not ready for incoming interrupts after having called pci_request_irq. This obviously requires us to finish our software setup which assigns the irq handler before we request the irq.

Quoted 4 lines
Signed-off-by Auke Kok <[email protected]>
Signed-off-by Jeff Garzik <[email protected]>

Dave, would that be an option for you?

Sounds like a plan. I'll do a test-build with this and the other patch, and throw it at the people seeing the problem tomorrow.

Thanks,

Dave

Signature · 7 lines
http://www.codemonkey.org.uk

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
Applied
#5 lore raw
In reply to Auke Kok
Quoted 12 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Quoted 8 lines

Tested by Doug Chapman <[email protected]>

Signed-off-by Auke Kok <[email protected]>
Acked-by Herbert Xu <[email protected]>

---

 drivers/net/e1000/e1000_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

author
#6 lore raw
Quoted 19 lines · Jeff Garzik
Quoted 18 lines · Auke Kok

Herbert Xu wrote:
"netif_poll_enable can only be called if you've previously called netif_poll_disable. Otherwise a poll might already be in action and you may get a crash like this."

Removing the call to netif_poll_enable in e1000_open should fix this issue, the only other call to netif_poll_enable is in e1000_up() which is only reached after a device reset or resume.

Bugzilla: http://bugzilla.kernel.org/show_bug.cgi?id=8455
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=240339

Tested by Doug Chapman <[email protected]>

Signed-off-by Auke Kok <[email protected]>
Acked-by Herbert Xu <[email protected]>

---

Quoted 5 lines
Quoted 2 lines
 drivers/net/e1000/e1000_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

It fixes the HP IA64 issue reported by Doug Chapman, but indeed does not even apply to the FC bugzilla. I was mistaken due to the fact that I sent the code that introduced the bug a while ago, but it's not in 2.6.21 (it was in the e1000-fixes tree you had and merged after 2.6.21).

So, this patch doesn't apply for -stable.

Auke

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

#7 lore raw
In reply to Jeff Garzik
Quoted 3 lines · Jeff Garzik

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

It should fix the problem completely in 2.6.22. For 2.6.21, we need a different fix because e1000_open is directly calling e1000_up.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#8 lore raw
Quoted 7 lines · Herbert Xu
Quoted 2 lines · Jeff Garzik

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

It should fix the problem completely in 2.6.22. For 2.6.21, we need a different fix because e1000_open is directly calling e1000_up.

Is there going to be a 2.6.21-stable fix for this?

Fedora is going to backport a big patchset but that won't work for -stable.

author
#9 lore raw
Quoted 4 lines · Chuck Ebbert
Quoted 3 lines · Herbert Xu
Quoted 2 lines · Jeff Garzik

applied, though as a poster (DaveJ?) noted, I'm not sure it completely fixes the bug

Quoted 8 lines
Quoted 3 lines

It should fix the problem completely in 2.6.22. For 2.6.21, we need a different fix because e1000_open is directly calling e1000_up.

Is there going to be a 2.6.21-stable fix for this?

Fedora is going to backport a big patchset but that won't work for -stable.

I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.

BTW this bug is present in most recent kernels, certainly before 2.6.20...

Auke

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

#10 lore raw
Quoted 4 lines · Auke

I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.

This patch should work for 2.6.21.

Cheers,

Signature · 5 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
drivers/net/e1000/e1000_main.c +1 -0
index b28a915..1e4e722 100644
@@ -1423,6 +1423,7 @@ e1000_open(struct net_device *netdev)
 
 	e1000_power_up_phy(adapter);
 
+	netif_poll_disable(netdev);
 	if ((err = e1000_up(adapter)))
 		goto err_up;
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
#11 lore raw
Quoted 7 lines · * Herbert Xu ([email protected]) wrote:
Quoted 4 lines · Auke

I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.

This patch should work for 2.6.21.

Mind adding an updated changelog and signed-off-by please?

thanks,
-chris

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

author
#12 lore raw
Quoted 6 lines · Chris Wright
Quoted 3 lines · * Herbert Xu ([email protected]) wrote:
Quoted 1 line · Auke

I've posted that before and it's up to the stable team. I think that it's a seriously toolarge change unless Herbert posts his short version of the fix for 2.6.21.1. I would be OK with that.

This patch should work for 2.6.21.

Mind adding an updated changelog and signed-off-by please?

Herbert,

With that, please add my Acked-By...

Auke

#13 lore raw
Quoted 2 lines · Auke

With that, please add my Acked-By...

Sorry, I only read yours after Chris's message :)

Chris, please add that Acked-By by Auke.

Cheers,

Signature · 10 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
#14 lore raw
Quoted 7 lines · * Herbert Xu ([email protected]) wrote:
Quoted 2 lines · Auke

With that, please add my Acked-By...

Sorry, I only read yours after Chris's message :)

Chris, please add that Acked-By by Auke.

Got it, thanks.
-chris

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

#15 lore raw
In reply to Chris Wright
Quoted 2 lines · Chris Wright

Mind adding an updated changelog and signed-off-by please?

Sure.

[E1000]: Fix netif_poll_enable crash in e1000_open

e1000_open calls e1000_up which will always call netif_poll_enable. However, it's illegal to call netif_poll_enable without calling netif_poll_disable at some prior time. This constraint is violated in e1000_open.

This patch fixes it by simply calling netif_poll_disable in e1000_open.

Signed-off-by Herbert Xu <[email protected]>

Cheers,

Signature · 5 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
drivers/net/e1000/e1000_main.c +1 -0
index b28a915..1e4e722 100644
@@ -1423,6 +1423,7 @@ e1000_open(struct net_device *netdev)
 
 	e1000_power_up_phy(adapter);
 
+	netif_poll_disable(netdev);
 	if ((err = e1000_up(adapter)))
 		goto err_up;
 	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/