Patchwork [e1000e] BUG triggered in blink path

login
register
mail settings
Submitter holger@eitzenberger.org
Date Nov. 11, 2010, 10:17 a.m.
Message ID <20101111101738.GA2972@mail.eitzenberger.org>
Download mbox | patch
Permalink /patch/70790/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

holger@eitzenberger.org - Nov. 11, 2010, 10:17 a.m.
Hi Jesse,

I've attached the patch against net-next-2.6.  Please check if it's ok
for you.  I checked e1000, igb and ixgbe as well, they don't have that
problem.

 /holger

> > After taking a look I think this may be caused by initializing
> > adapter->led_blink_task several times in e1000_phys_id(), while possibly
> > led_blink_task is running:
> > 
> > 	if ((hw->phy.type == e1000_phy_ife) ||
> > 	    (hw->mac.type == e1000_pchlan) ||
> > 	    (hw->mac.type == e1000_82574)) {
> > 		INIT_WORK(&adapter->led_blink_task, e1000e_led_blink_task);
> > 		if (!adapter->blink_timer.function) {
> > 
> > I can't reproduce it after moving it inside the following if block,
> > but I'm not quite sure if this catches all races in there.  Especially
> > the msleep_interruptible() may be too optimistic because it may
> > actually not wait long enough.  Someone with more knowledge of the
> > driver should take a look.
> 
> thanks for your investigation and troubleshooting.  I don't think it is 
> correct at all to be calling INIT_WORK more than once.  In fact the 
> INIT_WORK should just be moved into probe, and then e1000_phys_id should 
> just do schedule_work.
> 
>
Jeff Kirsher - Nov. 11, 2010, 11:59 p.m.
On Thu, Nov 11, 2010 at 02:17, Holger Eitzenberger
<holger@eitzenberger.org> wrote:
> Hi Jesse,
>
> I've attached the patch against net-next-2.6.  Please check if it's ok
> for you.  I checked e1000, igb and ixgbe as well, they don't have that
> problem.
>
>  /holger
>

Patch looks good, I have added the patch to my tree.

Cheers,
Jeff

>> > After taking a look I think this may be caused by initializing
>> > adapter->led_blink_task several times in e1000_phys_id(), while possibly
>> > led_blink_task is running:
>> >
>> >     if ((hw->phy.type == e1000_phy_ife) ||
>> >         (hw->mac.type == e1000_pchlan) ||
>> >         (hw->mac.type == e1000_82574)) {
>> >             INIT_WORK(&adapter->led_blink_task, e1000e_led_blink_task);
>> >             if (!adapter->blink_timer.function) {
>> >
>> > I can't reproduce it after moving it inside the following if block,
>> > but I'm not quite sure if this catches all races in there.  Especially
>> > the msleep_interruptible() may be too optimistic because it may
>> > actually not wait long enough.  Someone with more knowledge of the
>> > driver should take a look.
>>
>> thanks for your investigation and troubleshooting.  I don't think it is
>> correct at all to be calling INIT_WORK more than once.  In fact the
>> INIT_WORK should just be moved into probe, and then e1000_phys_id should
>> just do schedule_work.
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Brandeburg - Nov. 12, 2010, 12:40 a.m.
On Thu, 11 Nov 2010, Holger Eitzenberger wrote:
> I've attached the patch against net-next-2.6.  Please check if it's ok
> for you.  I checked e1000, igb and ixgbe as well, they don't have that
> problem.

Holger, 

I think the patch looks good and thanks.  Great catch too!  I see Jeff has 
put it into our testing process.

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

e1000e: fix double initialization in blink path

The kernel goes BUG() at the time 'ethtool -p eth0 3' comes
back, which is due to adapter->led_blink_task initialized
several times.  At the time it is still running this results
in a corrupted task_list of the associated workqueue.

The fix is to move the workqueue initialization to the
probe function instead.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>

Index: net-next-2.6/drivers/net/e1000e/ethtool.c
===================================================================
--- net-next-2.6.orig/drivers/net/e1000e/ethtool.c	2010-11-11 10:57:28.000000000 +0100
+++ net-next-2.6/drivers/net/e1000e/ethtool.c	2010-11-11 11:02:21.000000000 +0100
@@ -1860,7 +1860,7 @@ 
 /* bit defines for adapter->led_status */
 #define E1000_LED_ON		0
 
-static void e1000e_led_blink_task(struct work_struct *work)
+void e1000e_led_blink_task(struct work_struct *work)
 {
 	struct e1000_adapter *adapter = container_of(work,
 	                                struct e1000_adapter, led_blink_task);
@@ -1892,7 +1892,6 @@ 
 	    (hw->mac.type == e1000_pch2lan) ||
 	    (hw->mac.type == e1000_82583) ||
 	    (hw->mac.type == e1000_82574)) {
-		INIT_WORK(&adapter->led_blink_task, e1000e_led_blink_task);
 		if (!adapter->blink_timer.function) {
 			init_timer(&adapter->blink_timer);
 			adapter->blink_timer.function =
Index: net-next-2.6/drivers/net/e1000e/netdev.c
===================================================================
--- net-next-2.6.orig/drivers/net/e1000e/netdev.c	2010-11-11 10:57:28.000000000 +0100
+++ net-next-2.6/drivers/net/e1000e/netdev.c	2010-11-11 11:01:17.000000000 +0100
@@ -5864,6 +5864,7 @@ 
 	INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround);
 	INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task);
 	INIT_WORK(&adapter->print_hang_task, e1000_print_hw_hang);
+	INIT_WORK(&adapter->led_blink_task, e1000e_led_blink_task);
 
 	/* Initialize link parameters. User can change them with ethtool */
 	adapter->hw.mac.autoneg = 1;
Index: net-next-2.6/drivers/net/e1000e/e1000.h
===================================================================
--- net-next-2.6.orig/drivers/net/e1000e/e1000.h	2010-11-11 10:57:28.000000000 +0100
+++ net-next-2.6/drivers/net/e1000e/e1000.h	2010-11-11 11:01:57.000000000 +0100
@@ -482,6 +482,7 @@ 
 
 extern void e1000e_check_options(struct e1000_adapter *adapter);
 extern void e1000e_set_ethtool_ops(struct net_device *netdev);
+extern void e1000e_led_blink_task(struct work_struct *work);
 
 extern int e1000e_up(struct e1000_adapter *adapter);
 extern void e1000e_down(struct e1000_adapter *adapter);