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

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

[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 <doug.chapman@hp.com>

Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---

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

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 49be393..cbc7feb 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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 */

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

[Dave Jones]

On Mon, May 21, 2007 at 02:51:35PM -0700, Auke Kok wrote:
 > 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

-- 
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/

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

[Herbert Xu]

On Mon, May 21, 2007 at 06:16:35PM -0400, Dave Jones wrote:
> On Mon, May 21, 2007 at 02:51:35PM -0700, Auke Kok wrote:
>  > 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()

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Kok, Auke]

Herbert Xu wrote:
> On Mon, May 21, 2007 at 06:16:35PM -0400, Dave Jones wrote:
>> On Mon, May 21, 2007 at 02:51:35PM -0700, Auke Kok wrote:
>>  > 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()
> 
> 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 <auke-jan.h.kok@intel.com>
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 <auke-jan.h.kok@intel.com>
     Signed-off-by: Jeff Garzik <jeff@garzik.org>

Dave, would that be an option for you?

Auke

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

[Dave Jones]

On Mon, May 21, 2007 at 05:58:27PM -0700, Kok, Auke wrote:
 > >> 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 <auke-jan.h.kok@intel.com>
 > 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 <auke-jan.h.kok@intel.com>
 >      Signed-off-by: Jeff Garzik <jeff@garzik.org>
 > 
 > 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

-- 
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/

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

[Jeff Garzik]

Auke Kok wrote:
> 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 <doug.chapman@hp.com>
> 
> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> 
>  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

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

[Kok, Auke]

Jeff Garzik wrote:
> Auke Kok wrote:
>> 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 <doug.chapman@hp.com>
>>
>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
>> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>> ---
>>
>>  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/

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

[Herbert Xu]

On Mon, May 21, 2007 at 07:42:39PM -0400, Jeff Garzik wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Chuck Ebbert]

Herbert Xu wrote:
> On Mon, May 21, 2007 at 07:42:39PM -0400, Jeff Garzik wrote:
>> 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.

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

[Kok, Auke]

Chuck Ebbert wrote:
> Herbert Xu wrote:
>> On Mon, May 21, 2007 at 07:42:39PM -0400, Jeff Garzik wrote:
>>> 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.

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/

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

[Herbert Xu]

On Tue, May 22, 2007 at 10:02:53AM -0700, Kok, Auke wrote:
> 
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index b28a915..1e4e722 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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;

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

[Chris Wright]

* Herbert Xu (herbert@gondor.apana.org.au) wrote:
> On Tue, May 22, 2007 at 10:02:53AM -0700, Kok, Auke wrote:
> > 
> > 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/

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

[Kok, Auke]

Chris Wright wrote:
> * Herbert Xu (herbert@gondor.apana.org.au) wrote:
>> On Tue, May 22, 2007 at 10:02:53AM -0700, Kok, Auke wrote:
>>> 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

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

[Herbert Xu]

On Wed, May 23, 2007 at 04:38:20PM -0700, Kok, Auke wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
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/

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

[Chris Wright]

* Herbert Xu (herbert@gondor.apana.org.au) wrote:
> On Wed, May 23, 2007 at 04:38:20PM -0700, Kok, Auke wrote:
> >
> > 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/

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

[Herbert Xu]

On Wed, May 23, 2007 at 04:34:18PM -0700, Chris Wright wrote:
> 
> 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 <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index b28a915..1e4e722 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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/

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

[Auke Kok]

Herbert Xy 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.

Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---

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

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 49be393..cbc7feb 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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 */

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

[Herbert Xu]

On Mon, May 21, 2007 at 08:32:41AM -0700, Auke Kok wrote:
> Herbert Xy 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.
> 
> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>

Looks good!

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt