* [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken.
@ 2011-09-30 21:51 David Daney
2011-09-30 22:54 ` David Miller
2011-11-11 0:29 ` Tabi Timur-B04825
0 siblings, 2 replies; 5+ messages in thread
From: David Daney @ 2011-09-30 21:51 UTC (permalink / raw)
To: netdev, davem; +Cc: linux-mips, David Daney
Accesses to the mdio busses must be done with the mdio_lock to ensure
proper operation. Conveniently we have the helper function
mdiobus_read() to do that for us. Lets use it in get_phy_id() instead
of accessing the bus without the lock held.
Signed-off-by: David Daney <david.daney@cavium.com>
---
drivers/net/phy/phy_device.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ff109fe..83a5a5a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -213,7 +213,7 @@ int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id)
/* Grab the bits from PHYIR1, and put them
* in the upper half */
- phy_reg = bus->read(bus, addr, MII_PHYSID1);
+ phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
if (phy_reg < 0)
return -EIO;
@@ -221,7 +221,7 @@ int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id)
*phy_id = (phy_reg & 0xffff) << 16;
/* Grab the bits from PHYIR2, and put them in the lower half */
- phy_reg = bus->read(bus, addr, MII_PHYSID2);
+ phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
if (phy_reg < 0)
return -EIO;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken.
2011-09-30 21:51 [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken David Daney
@ 2011-09-30 22:54 ` David Miller
2011-11-11 0:29 ` Tabi Timur-B04825
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2011-09-30 22:54 UTC (permalink / raw)
To: david.daney; +Cc: netdev, linux-mips
From: David Daney <david.daney@cavium.com>
Date: Fri, 30 Sep 2011 14:51:22 -0700
> Accesses to the mdio busses must be done with the mdio_lock to ensure
> proper operation. Conveniently we have the helper function
> mdiobus_read() to do that for us. Lets use it in get_phy_id() instead
> of accessing the bus without the lock held.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
Applied to net-next.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken.
2011-09-30 21:51 [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken David Daney
2011-09-30 22:54 ` David Miller
@ 2011-11-11 0:29 ` Tabi Timur-B04825
2011-11-11 0:48 ` David Daney
1 sibling, 1 reply; 5+ messages in thread
From: Tabi Timur-B04825 @ 2011-11-11 0:29 UTC (permalink / raw)
To: David Daney
Cc: netdev@vger.kernel.org, davem@davemloft.net,
linux-mips@linux-mips.org
On Fri, Sep 30, 2011 at 4:51 PM, David Daney <david.daney@cavium.com> wrote:
> Accesses to the mdio busses must be done with the mdio_lock to ensure
> proper operation. Conveniently we have the helper function
> mdiobus_read() to do that for us. Lets use it in get_phy_id() instead
> of accessing the bus without the lock held.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
This patch is causing me problems, but I'm not exactly certain how.
The problems only appear when I add some unrelated code to my platform
file (p1022ds.c), but the trap is definitely in the phy code:
Fixed MDIO Bus: probed
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
Call Trace:
[e685dcc0] [c0008c7c] show_stack+0x44/0x154 (unreliable)
[e685dd00] [c007bf74] __lock_acquire+0x1374/0x1824
[e685ddb0] [c007c870] lock_acquire+0x4c/0x68
[e685ddd0] [c0455fe4] mutex_lock_nested+0x6c/0x394
[e685de30] [c02acc08] mdiobus_read+0x3c/0x78
[e685de50] [c02abc98] get_phy_id+0x24/0x80
[e685de70] [c02b1860] fsl_pq_mdio_probe+0x3ac/0x458
[e685deb0] [c027ba2c] platform_drv_probe+0x20/0x30
[e685dec0] [c027a4b0] driver_probe_device+0xa4/0x1d4
[e685dee0] [c027a6a4] __driver_attach+0xc4/0xc8
[e685df00] [c027939c] bus_for_each_dev+0x60/0x9c
[e685df30] [c027a0e4] driver_attach+0x24/0x34
[e685df40] [c0279d30] bus_add_driver+0x1b0/0x278
[e685df70] [c027aab8] driver_register+0x88/0x154
[e685df90] [c027bd5c] platform_driver_register+0x68/0x78
[e685dfa0] [c05d47c8] fsl_pq_mdio_init+0x18/0x28
[e685dfb0] [c0001eb8] do_one_initcall+0x34/0x1ac
[e685dfe0] [c05b984c] kernel_init+0xa0/0x13c
[e685dff0] [c000e588] kernel_thread+0x4c/0x68
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc0456080
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=8 P1022 DS
Modules linked in:
NIP: c0456080 LR: c0456068 CTR: 00000000
REGS: e685dd20 TRAP: 0300 Not tainted (3.2.0-10b-00092-g66f2305-dirty)
MSR: 00021000 <ME,CE> CR: 22042044 XER: 20000000
DEAR: 00000000, ESR: 00800000
TASK = e6860000[1] 'swapper' THREAD: e685c000 CPU: 0
GPR00: ffffffff e685ddd0 e6860000 e6e58028 e685ddd8 e685c000 e685dde4 00000002
GPR08: 00000000 00000000 00000000 00000000 44042022 40401800 00000000 00000000
GPR16: c0000a00 00000014 3fffffff 03ff9000 00000015 7ff3a68c c061e000 00000000
GPR24: e6e5804c e685ddd8 e6e5802c 00029000 e6860000 c0620000 e685c000 e6e58028
NIP [c0456080] mutex_lock_nested+0x108/0x394
LR [c0456068] mutex_lock_nested+0xf0/0x394
Call Trace:
[e685ddd0] [c0456068] mutex_lock_nested+0xf0/0x394 (unreliable)
[e685de30] [c02acc08] mdiobus_read+0x3c/0x78
[e685de50] [c02abc98] get_phy_id+0x24/0x80
[e685de70] [c02b1860] fsl_pq_mdio_probe+0x3ac/0x458
[e685deb0] [c027ba2c] platform_drv_probe+0x20/0x30
[e685dec0] [c027a4b0] driver_probe_device+0xa4/0x1d4
[e685dee0] [c027a6a4] __driver_attach+0xc4/0xc8
[e685df00] [c027939c] bus_for_each_dev+0x60/0x9c
[e685df30] [c027a0e4] driver_attach+0x24/0x34
[e685df40] [c0279d30] bus_add_driver+0x1b0/0x278
[e685df70] [c027aab8] driver_register+0x88/0x154
[e685df90] [c027bd5c] platform_driver_register+0x68/0x78
[e685dfa0] [c05d47c8] fsl_pq_mdio_init+0x18/0x28
[e685dfb0] [c0001eb8] do_one_initcall+0x34/0x1ac
[e685dfe0] [c05b984c] kernel_init+0xa0/0x13c
[e685dff0] [c000e588] kernel_thread+0x4c/0x68
Instruction dump:
7f24cb78 4bc21141 80bc0004 7fe3fb78 7f24cb78 4bc21325 813f0028 3b1f0024
933f0028 3800ffff 93010008 9121000c
93810010 7c0004ac 7d20f828
---[ end trace 7cc8bbd19b132dac ]---
note: swapper[1] exited with preempt_count 1
Kernel panic - not syncing: Attempted to kill init!
Call Trace:
[e685dc00] [c0008c7c] show_stack+0x44/0x154 (unreliable)
[e685dc40] [c04583a0] panic+0xb4/0x1f0
[e685dc90] [c004724c] do_exit+0x5dc/0x684
[e685dce0] [c000b368] die+0xdc/0x1b4
[e685dd00] [c00137fc] bad_page_fault+0xb4/0xfc
[e685dd10] [c000f998] handle_page_fault+0x7c/0x80
--- Exception: 300 at mutex_lock_nested+0x108/0x394
LR = mutex_lock_nested+0xf0/0x394
[e685de30] [c02acc08] mdiobus_read+0x3c/0x78
[e685de50] [c02abc98] get_phy_id+0x24/0x80
[e685de70] [c02b1860] fsl_pq_mdio_probe+0x3ac/0x458
[e685deb0] [c027ba2c] platform_drv_probe+0x20/0x30
[e685dec0] [c027a4b0] driver_probe_device+0xa4/0x1d4
[e685dee0] [c027a6a4] __driver_attach+0xc4/0xc8
[e685df00] [c027939c] bus_for_each_dev+0x60/0x9c
[e685df30] [c027a0e4] driver_attach+0x24/0x34
[e685df40] [c0279d30] bus_add_driver+0x1b0/0x278
[e685df70] [c027aab8] driver_register+0x88/0x154
[e685df90] [c027bd5c] platform_driver_register+0x68/0x78
[e685dfa0] [c05d47c8] fsl_pq_mdio_init+0x18/0x28
[e685dfb0] [c0001eb8] do_one_initcall+0x34/0x1ac
[e685dfe0] [c05b984c] kernel_init+0xa0/0x13c
[e685dff0] [c000e588] kernel_thread+0x4c/0x68
Rebooting in 1 seconds..
I'm still trying to narrow down what's causing the problem, but when I
revert this patch, I don't see these traps.
Sometimes, I don't get the trap, but I get a hang on this line:
int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
{
int retval;
BUG_ON(in_interrupt());
---> mutex_lock(&bus->mdio_lock);
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken.
2011-11-11 0:29 ` Tabi Timur-B04825
@ 2011-11-11 0:48 ` David Daney
[not found] ` <CAKWjMd4WjDNHH8dOr9GPxQTC69SGgjh+euWh_DSjhtRxquB8qA@mail.gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: David Daney @ 2011-11-11 0:48 UTC (permalink / raw)
To: Tabi Timur-B04825
Cc: netdev@vger.kernel.org, davem@davemloft.net,
linux-mips@linux-mips.org
On 11/10/2011 04:29 PM, Tabi Timur-B04825 wrote:
> On Fri, Sep 30, 2011 at 4:51 PM, David Daney<david.daney@cavium.com> wrote:
>> Accesses to the mdio busses must be done with the mdio_lock to ensure
>> proper operation. Conveniently we have the helper function
>> mdiobus_read() to do that for us. Lets use it in get_phy_id() instead
>> of accessing the bus without the lock held.
>>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>> ---
>
> This patch is causing me problems, but I'm not exactly certain how.
I think it is because fsl_pq_mdio_probe() is defective.
You are using the bus, by calling fsl_pq_mdio_find_free(), before the
driver is registered and initialized by calling of_mdiobus_register().
At least that is my take on it.
David Daney
> The problems only appear when I add some unrelated code to my platform
> file (p1022ds.c), but the trap is definitely in the phy code:
>
> Fixed MDIO Bus: probed
> INFO: trying to register non-static key.
> the code is fine but needs lockdep annotation.
> turning off the locking correctness validator.
> Call Trace:
> [e685dcc0] [c0008c7c] show_stack+0x44/0x154 (unreliable)
> [e685dd00] [c007bf74] __lock_acquire+0x1374/0x1824
> [e685ddb0] [c007c870] lock_acquire+0x4c/0x68
> [e685ddd0] [c0455fe4] mutex_lock_nested+0x6c/0x394
> [e685de30] [c02acc08] mdiobus_read+0x3c/0x78
> [e685de50] [c02abc98] get_phy_id+0x24/0x80
> [e685de70] [c02b1860] fsl_pq_mdio_probe+0x3ac/0x458
> [e685deb0] [c027ba2c] platform_drv_probe+0x20/0x30
> [e685dec0] [c027a4b0] driver_probe_device+0xa4/0x1d4
> [e685dee0] [c027a6a4] __driver_attach+0xc4/0xc8
> [e685df00] [c027939c] bus_for_each_dev+0x60/0x9c
> [e685df30] [c027a0e4] driver_attach+0x24/0x34
> [e685df40] [c0279d30] bus_add_driver+0x1b0/0x278
> [e685df70] [c027aab8] driver_register+0x88/0x154
> [e685df90] [c027bd5c] platform_driver_register+0x68/0x78
> [e685dfa0] [c05d47c8] fsl_pq_mdio_init+0x18/0x28
> [e685dfb0] [c0001eb8] do_one_initcall+0x34/0x1ac
> [e685dfe0] [c05b984c] kernel_init+0xa0/0x13c
> [e685dff0] [c000e588] kernel_thread+0x4c/0x68
> Unable to handle kernel paging request for data at address 0x00000000
> Faulting instruction address: 0xc0456080
> Oops: Kernel access of bad area, sig: 11 [#1]
> SMP NR_CPUS=8 P1022 DS
> Modules linked in:
> NIP: c0456080 LR: c0456068 CTR: 00000000
> REGS: e685dd20 TRAP: 0300 Not tainted (3.2.0-10b-00092-g66f2305-dirty)
> MSR: 00021000<ME,CE> CR: 22042044 XER: 20000000
> DEAR: 00000000, ESR: 00800000
> TASK = e6860000[1] 'swapper' THREAD: e685c000 CPU: 0
> GPR00: ffffffff e685ddd0 e6860000 e6e58028 e685ddd8 e685c000 e685dde4 00000002
> GPR08: 00000000 00000000 00000000 00000000 44042022 40401800 00000000 00000000
> GPR16: c0000a00 00000014 3fffffff 03ff9000 00000015 7ff3a68c c061e000 00000000
> GPR24: e6e5804c e685ddd8 e6e5802c 00029000 e6860000 c0620000 e685c000 e6e58028
> NIP [c0456080] mutex_lock_nested+0x108/0x394
> LR [c0456068] mutex_lock_nested+0xf0/0x394
> Call Trace:
> [e685ddd0] [c0456068] mutex_lock_nested+0xf0/0x394 (unreliable)
> [e685de30] [c02acc08] mdiobus_read+0x3c/0x78
> [e685de50] [c02abc98] get_phy_id+0x24/0x80
> [e685de70] [c02b1860] fsl_pq_mdio_probe+0x3ac/0x458
> [e685deb0] [c027ba2c] platform_drv_probe+0x20/0x30
> [e685dec0] [c027a4b0] driver_probe_device+0xa4/0x1d4
> [e685dee0] [c027a6a4] __driver_attach+0xc4/0xc8
> [e685df00] [c027939c] bus_for_each_dev+0x60/0x9c
> [e685df30] [c027a0e4] driver_attach+0x24/0x34
> [e685df40] [c0279d30] bus_add_driver+0x1b0/0x278
> [e685df70] [c027aab8] driver_register+0x88/0x154
> [e685df90] [c027bd5c] platform_driver_register+0x68/0x78
> [e685dfa0] [c05d47c8] fsl_pq_mdio_init+0x18/0x28
> [e685dfb0] [c0001eb8] do_one_initcall+0x34/0x1ac
> [e685dfe0] [c05b984c] kernel_init+0xa0/0x13c
> [e685dff0] [c000e588] kernel_thread+0x4c/0x68
> Instruction dump:
> 7f24cb78 4bc21141 80bc0004 7fe3fb78 7f24cb78 4bc21325 813f0028 3b1f0024
> 933f0028 3800ffff 93010008 9121000c
> 93810010 7c0004ac 7d20f828
> ---[ end trace 7cc8bbd19b132dac ]---
> note: swapper[1] exited with preempt_count 1
> Kernel panic - not syncing: Attempted to kill init!
> Call Trace:
> [e685dc00] [c0008c7c] show_stack+0x44/0x154 (unreliable)
> [e685dc40] [c04583a0] panic+0xb4/0x1f0
> [e685dc90] [c004724c] do_exit+0x5dc/0x684
> [e685dce0] [c000b368] die+0xdc/0x1b4
> [e685dd00] [c00137fc] bad_page_fault+0xb4/0xfc
> [e685dd10] [c000f998] handle_page_fault+0x7c/0x80
> --- Exception: 300 at mutex_lock_nested+0x108/0x394
> LR = mutex_lock_nested+0xf0/0x394
> [e685de30] [c02acc08] mdiobus_read+0x3c/0x78
> [e685de50] [c02abc98] get_phy_id+0x24/0x80
> [e685de70] [c02b1860] fsl_pq_mdio_probe+0x3ac/0x458
> [e685deb0] [c027ba2c] platform_drv_probe+0x20/0x30
> [e685dec0] [c027a4b0] driver_probe_device+0xa4/0x1d4
> [e685dee0] [c027a6a4] __driver_attach+0xc4/0xc8
> [e685df00] [c027939c] bus_for_each_dev+0x60/0x9c
> [e685df30] [c027a0e4] driver_attach+0x24/0x34
> [e685df40] [c0279d30] bus_add_driver+0x1b0/0x278
> [e685df70] [c027aab8] driver_register+0x88/0x154
> [e685df90] [c027bd5c] platform_driver_register+0x68/0x78
> [e685dfa0] [c05d47c8] fsl_pq_mdio_init+0x18/0x28
> [e685dfb0] [c0001eb8] do_one_initcall+0x34/0x1ac
> [e685dfe0] [c05b984c] kernel_init+0xa0/0x13c
> [e685dff0] [c000e588] kernel_thread+0x4c/0x68
> Rebooting in 1 seconds..
>
> I'm still trying to narrow down what's causing the problem, but when I
> revert this patch, I don't see these traps.
>
> Sometimes, I don't get the trap, but I get a hang on this line:
>
> int mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
> {
> int retval;
>
> BUG_ON(in_interrupt());
>
> ---> mutex_lock(&bus->mdio_lock);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken.
[not found] ` <CAKWjMd4WjDNHH8dOr9GPxQTC69SGgjh+euWh_DSjhtRxquB8qA@mail.gmail.com>
@ 2011-11-14 17:10 ` Timur Tabi
0 siblings, 0 replies; 5+ messages in thread
From: Timur Tabi @ 2011-11-14 17:10 UTC (permalink / raw)
To: Andy Fleming; +Cc: David Daney, netdev@vger.kernel.org, davem@davemloft.net
Andy Fleming wrote:
> Yes, that is correct. I have a patch, I just have to resend it. Will do
> next time I come to a break in what I'm working on.
Can you point me to that patch, so that I can try it out now?
I fixed some lockdep-related code in my audio driver. The driver was not initializing a sysfs attr structure, so I added a call to sysfs_attr_init(). But when I do that, I get the following bug report. I don't understand the connection.
=============================================
[ INFO: possible recursive locking detected ]
3.2.0-10b-00093-gebea711-dirty #10
---------------------------------------------
kworker/1:1/271 is trying to acquire lock:
(&(&(priv->tx_queue[i]->txlock))->rlock){......}, at: [<c02b2e28>] lock_tx_qs+0
x34/0x54
but task is already holding lock:
(&(&(priv->tx_queue[i]->txlock))->rlock){......}, at: [<c02b2e28>] lock_tx_qs+0
x34/0x54
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(&(&(priv->tx_queue[i]->txlock))->rlock);
lock(&(&(priv->tx_queue[i]->txlock))->rlock);
*** DEADLOCK ***
May be due to missing lock nesting notation
4 locks held by kworker/1:1/271:
#0: (events){.+.+.+}, at: [<c005b7c8>] process_one_work+0x138/0x490
#1: ((&(&dev->state_queue)->work)){+.+...}, at: [<c005b7c8>] process_one_work+
0x138/0x490
#2: (&dev->lock){+.+...}, at: [<c02ab3cc>] phy_state_machine+0x30/0x580
#3: (&(&(priv->tx_queue[i]->txlock))->rlock){......}, at: [<c02b2e28>] lock_tx
_qs+0x34/0x54
stack backtrace:
Call Trace:
[e69d5d80] [c0008c7c] show_stack+0x44/0x154 (unreliable)
[e69d5dc0] [c007bafc] __lock_acquire+0xefc/0x1824
[e69d5e70] [c007c870] lock_acquire+0x4c/0x68
[e69d5e90] [c04575d8] _raw_spin_lock+0x44/0x60
[e69d5ea0] [c02b2e28] lock_tx_qs+0x34/0x54
[e69d5ec0] [c02b2f24] adjust_link+0x34/0x1d8
[e69d5ef0] [c02ab73c] phy_state_machine+0x3a0/0x580
[e69d5f10] [c005b83c] process_one_work+0x1ac/0x490
[e69d5f50] [c005e4c0] worker_thread+0x18c/0x35c
[e69d5f90] [c00631d4] kthread+0x7c/0x80
[e69d5ff0] [c000e588] kernel_thread+0x4c/0x68
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-11-14 17:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-30 21:51 [PATCH] netdev/phy: Use mdiobus_read() so that proper locks are taken David Daney
2011-09-30 22:54 ` David Miller
2011-11-11 0:29 ` Tabi Timur-B04825
2011-11-11 0:48 ` David Daney
[not found] ` <CAKWjMd4WjDNHH8dOr9GPxQTC69SGgjh+euWh_DSjhtRxquB8qA@mail.gmail.com>
2011-11-14 17:10 ` Timur Tabi
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).