* [PATCH] net: stmicro: stmac: do not grab a mutex in irq off section
@ 2014-02-18 17:34 Sebastian Andrzej Siewior
2014-02-19 20:23 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2014-02-18 17:34 UTC (permalink / raw)
To: Giuseppe Cavallaro; +Cc: netdev, Sebastian Andrzej Siewior
I captured this:
|libphy: stmmac-0:04 - Link is Up - 1000/Full
|BUG: sleeping function called from invalid context at kernel/mutex.c:616
|in_atomic(): 1, irqs_disabled(): 128, pid: 31, name: kworker/1:1
|CPU: 1 PID: 31 Comm: kworker/1:1 Not tainted 3.12.0-00324-g28301d2-dirty #5
|[<8042f8e8>] (mutex_lock_nested+0x28/0x3cc) from [<802a226c>] (mdiobus_read+0x38/0x64)
|[<802a226c>] (mdiobus_read+0x38/0x64) from [<802a1968>] (genphy_update_link+0x18/0x50)
|[<802a1968>] (genphy_update_link+0x18/0x50) from [<802a19ac>] (genphy_read_status+0xc/0x180)
|[<802a19ac>] (genphy_read_status+0xc/0x180) from [<8029fb0c>] (phy_init_eee+0x4c/0x2ac)
|[<8029fb0c>] (phy_init_eee+0x4c/0x2ac) from [<802a7550>] (stmmac_eee_init+0x40/0x104)
|[<802a7550>] (stmmac_eee_init+0x40/0x104) from [<802a8678>] (stmmac_adjust_link+0x118/0x228)
|[<802a8678>] (stmmac_adjust_link+0x118/0x228) from [<802a0af4>] (phy_state_machine+0x23c/0x39c)
|[<802a0af4>] (phy_state_machine+0x23c/0x39c) from [<800371b8>] (process_one_work+0x1a4/0x44c)
|[<800371b8>] (process_one_work+0x1a4/0x44c) from [<80037884>] (worker_thread+0x138/0x390)
|[<80037884>] (worker_thread+0x138/0x390) from [<8003e850>] (kthread+0xb4/0xb8)
|[<8003e850>] (kthread+0xb4/0xb8) from [<8000e5a8>] (ret_from_fork+0x14/0x2c)
|stmmac: Energy-Efficient Ethernet initialized
stmmac_eee_init() is called in other places without this lock.
phy_print_status() is just a printk() and the if condition is not
protected by the lock so I moved the unlock part just before that.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 751a73a..a514d8b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -730,6 +730,7 @@ static void stmmac_adjust_link(struct net_device *dev)
priv->speed = 0;
priv->oldduplex = -1;
}
+ spin_unlock_irqrestore(&priv->lock, flags);
if (new_state && netif_msg_link(priv))
phy_print_status(phydev);
@@ -738,8 +739,6 @@ static void stmmac_adjust_link(struct net_device *dev)
* MAC related HW registers.
*/
priv->eee_enabled = stmmac_eee_init(priv);
-
- spin_unlock_irqrestore(&priv->lock, flags);
}
/**
--
1.9.0.rc3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net: stmicro: stmac: do not grab a mutex in irq off section
2014-02-18 17:34 [PATCH] net: stmicro: stmac: do not grab a mutex in irq off section Sebastian Andrzej Siewior
@ 2014-02-19 20:23 ` David Miller
2014-02-26 10:00 ` Giuseppe CAVALLARO
0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2014-02-19 20:23 UTC (permalink / raw)
To: bigeasy; +Cc: peppe.cavallaro, netdev
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Tue, 18 Feb 2014 18:34:03 +0100
> I captured this:
>
> |libphy: stmmac-0:04 - Link is Up - 1000/Full
> |BUG: sleeping function called from invalid context at kernel/mutex.c:616
> |in_atomic(): 1, irqs_disabled(): 128, pid: 31, name: kworker/1:1
> |CPU: 1 PID: 31 Comm: kworker/1:1 Not tainted 3.12.0-00324-g28301d2-dirty #5
> |[<8042f8e8>] (mutex_lock_nested+0x28/0x3cc) from [<802a226c>] (mdiobus_read+0x38/0x64)
> |[<802a226c>] (mdiobus_read+0x38/0x64) from [<802a1968>] (genphy_update_link+0x18/0x50)
> |[<802a1968>] (genphy_update_link+0x18/0x50) from [<802a19ac>] (genphy_read_status+0xc/0x180)
> |[<802a19ac>] (genphy_read_status+0xc/0x180) from [<8029fb0c>] (phy_init_eee+0x4c/0x2ac)
> |[<8029fb0c>] (phy_init_eee+0x4c/0x2ac) from [<802a7550>] (stmmac_eee_init+0x40/0x104)
> |[<802a7550>] (stmmac_eee_init+0x40/0x104) from [<802a8678>] (stmmac_adjust_link+0x118/0x228)
> |[<802a8678>] (stmmac_adjust_link+0x118/0x228) from [<802a0af4>] (phy_state_machine+0x23c/0x39c)
> |[<802a0af4>] (phy_state_machine+0x23c/0x39c) from [<800371b8>] (process_one_work+0x1a4/0x44c)
> |[<800371b8>] (process_one_work+0x1a4/0x44c) from [<80037884>] (worker_thread+0x138/0x390)
> |[<80037884>] (worker_thread+0x138/0x390) from [<8003e850>] (kthread+0xb4/0xb8)
> |[<8003e850>] (kthread+0xb4/0xb8) from [<8000e5a8>] (ret_from_fork+0x14/0x2c)
> |stmmac: Energy-Efficient Ethernet initialized
>
> stmmac_eee_init() is called in other places without this lock.
> phy_print_status() is just a printk() and the if condition is not
> protected by the lock so I moved the unlock part just before that.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
It does look like a problem though, two concurrent calls into
stmmac_eee_init would be really bad. Two threads could see
eee enabled being false at the same time, and both try to
initialize the eee state.
stmmac_hw_setup()'s invocation is fine because we doing something like
bringing the device up or bringing it back after resume, and thus are
synchonized by the RTNL semaphore.
Actually, the same "mutex in irq off section" problem exists in that
code path, because stmmac_resume() invokes stmmac_hw_setup() with
the priv->lock held.
This is a much deeper problem then what your patch is tackling, and
thus a lot more thought about how to fix this is needed.
I'm not applying this, sorry.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net: stmicro: stmac: do not grab a mutex in irq off section
2014-02-19 20:23 ` David Miller
@ 2014-02-26 10:00 ` Giuseppe CAVALLARO
0 siblings, 0 replies; 3+ messages in thread
From: Giuseppe CAVALLARO @ 2014-02-26 10:00 UTC (permalink / raw)
To: David Miller, bigeasy; +Cc: netdev
On 2/19/2014 9:23 PM, David Miller wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date: Tue, 18 Feb 2014 18:34:03 +0100
>
Re: [PATCH] net: stmicro: stmac: do not grab a mutex in irq off section
^^^^^^^ stmmac
>> I captured this:
>>
>> |libphy: stmmac-0:04 - Link is Up - 1000/Full
>> |BUG: sleeping function called from invalid context at kernel/mutex.c:616
>> |in_atomic(): 1, irqs_disabled(): 128, pid: 31, name: kworker/1:1
>> |CPU: 1 PID: 31 Comm: kworker/1:1 Not tainted 3.12.0-00324-g28301d2-dirty #5
>> |[<8042f8e8>] (mutex_lock_nested+0x28/0x3cc) from [<802a226c>] (mdiobus_read+0x38/0x64)
>> |[<802a226c>] (mdiobus_read+0x38/0x64) from [<802a1968>] (genphy_update_link+0x18/0x50)
>> |[<802a1968>] (genphy_update_link+0x18/0x50) from [<802a19ac>] (genphy_read_status+0xc/0x180)
>> |[<802a19ac>] (genphy_read_status+0xc/0x180) from [<8029fb0c>] (phy_init_eee+0x4c/0x2ac)
>> |[<8029fb0c>] (phy_init_eee+0x4c/0x2ac) from [<802a7550>] (stmmac_eee_init+0x40/0x104)
>> |[<802a7550>] (stmmac_eee_init+0x40/0x104) from [<802a8678>] (stmmac_adjust_link+0x118/0x228)
>> |[<802a8678>] (stmmac_adjust_link+0x118/0x228) from [<802a0af4>] (phy_state_machine+0x23c/0x39c)
>> |[<802a0af4>] (phy_state_machine+0x23c/0x39c) from [<800371b8>] (process_one_work+0x1a4/0x44c)
>> |[<800371b8>] (process_one_work+0x1a4/0x44c) from [<80037884>] (worker_thread+0x138/0x390)
>> |[<80037884>] (worker_thread+0x138/0x390) from [<8003e850>] (kthread+0xb4/0xb8)
>> |[<8003e850>] (kthread+0xb4/0xb8) from [<8000e5a8>] (ret_from_fork+0x14/0x2c)
>> |stmmac: Energy-Efficient Ethernet initialized
>>
>> stmmac_eee_init() is called in other places without this lock.
>> phy_print_status() is just a printk() and the if condition is not
>> protected by the lock so I moved the unlock part just before that.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> It does look like a problem though, two concurrent calls into
> stmmac_eee_init would be really bad. Two threads could see
> eee enabled being false at the same time, and both try to
> initialize the eee state.
>
> stmmac_hw_setup()'s invocation is fine because we doing something like
> bringing the device up or bringing it back after resume, and thus are
> synchonized by the RTNL semaphore.
>
> Actually, the same "mutex in irq off section" problem exists in that
> code path, because stmmac_resume() invokes stmmac_hw_setup() with
> the priv->lock held.
>
> This is a much deeper problem then what your patch is tackling, and
> thus a lot more thought about how to fix this is needed.
David, I will take care about this problem and provide a patch
in case of I realize the problem. I need to review the stmmac_hw_setup
function and suspend/resume and I will follow your advice.
Peppe
>
> I'm not applying this, sorry.
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-26 10:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-18 17:34 [PATCH] net: stmicro: stmac: do not grab a mutex in irq off section Sebastian Andrzej Siewior
2014-02-19 20:23 ` David Miller
2014-02-26 10:00 ` Giuseppe CAVALLARO
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).