Re: [PATCH] phy: fix the bug when remove the phy driver

On August 2, 2016 6:21:52 PM MST, Ding Tianhong [off-list ref] wrote:

On 2016/8/3 0:42, Florian Fainelli wrote:

quoted

Le 02/08/2016 à 06:00, Ding Tianhong a écrit :

quoted

The nic in my board use the phy dev from marvell, and
the system will load the marvell phy driver automatically,
but when I remove the phy drivers, the system immediately panic:

localhost login: [ 2582.052465] Unable to handle kernel NULL pointer

dereference at virtual address 000000c0

quoted

quoted

[ 2582.061429] pgd = ffff800001226000
[ 2582.065277] [000000c0] *pgd=0000003f7f893003,

*pud=0000003f7f894003, *pmd=0000003f7f895003, *pte=006000006d000707

quoted

quoted

[ 2582.076681] Internal error: Oops: 96000006 [#1] SMP
[ 2582.082049] Modules linked in: sr_mod(E) cdrom(E) ses(E)

enclosure(E) shpchp(E) crc32_arm64(E) aes_ce_blk(E) ablk_helper(E)
cryptd(E) aes_ce_cipher(E) ghash_ce(E) sha2_ce(E) sha1_ce(E)
usb_storage(E) dm_mirror(E) dm_region_hash(E) dm_log(E) dm_mod(E) [last
unloaded: marvell]

quoted

quoted

[ 2582.109226] CPU: 21 PID: 1514 Comm: kworker/21:1 Tainted: G      

    E   4.1.27-vhulk3.6.5.aarch64 #1

quoted

quoted

[ 2582.119415] Hardware name: Huawei Taishan 2180 /BC11SPCC, BIOS

1.31 06/23/2016

quoted

quoted

[ 2582.127346] Workqueue: events_power_efficient phy_state_machine
[ 2582.133796] task: ffff803f6f41bac0 ti: ffff803f6eca4000 task.ti:

ffff803f6eca4000

quoted

quoted

[ 2582.141910] PC is at phy_state_machine+0x3c/0x438
[ 2582.147081] LR is at phy_state_machine+0x34/0x438
[ 2582.152173] pc : [<ffff800000715384>] lr : [<ffff80000071537c>]

pstate: 60000145

quoted

quoted

[ 2582.160189] sp : ffff803f6eca7d30
[ 2582.163825] x29: ffff803f6eca7d30 x28: 0000000000000000
[ 2582.169689] x27: ffff805fdfd0aa00 x26: 0000000000000008
[ 2582.175482] x25: 0000000000000000 x24: ffff80000112c57c
[ 2582.181391] x23: ffff805f4fc8a800 x22: ffff805fdfd0f700
[ 2582.187241] x21: ffff805f4fc8abf8 x20: ffff805f4fc8a770
[ 2582.193035] x19: ffff805f4fc8ab70 x18: 0000000000000007
[ 2582.198914] x17: 000000000000000e x16: 0000000000000001
[ 2582.204710] x15: 0000000000000007 x14: 000000000000000e
[ 2582.210584] x13: 0000000000000200 x12: 0000000055555556
[ 2582.216373] x11: 0000000000001c00 x10: 0000000000000000
[ 2582.222166] x9 : ffff800000a36880 x8 : ffff803f6f41c060
[ 2582.227994] x7 : 000000010008b39b x6 : ffff80000101e690
[ 2582.233788] x5 : 0000000000000000 x4 : 0000000000800000
[ 2582.239612] x3 : 0000000000000000 x2 : 0000000000000000
[ 2582.245404] x1 : 0000000000000000 x0 : 0000000000000000
[ 2582.265813]
[ 2582.282971] Process kworker/21:1 (pid: 1514, stack limit =

0xffff803f6eca4020)

quoted

quoted

[ 2582.307284] Stack: (0xffff803f6eca7d30 to 0xffff803f6eca8000)
[ 2582.331183] 7d20:                                  

ffff803f6eca7d70 ffff8000000db3b8

quoted

quoted

[ 2582.354788] 7d40: ffff803f6e696000 ffff805f4fc8ab70

ffff805fdfd0aa00 ffff805fdfd0f700

quoted

quoted

[ 2582.378341] 7d60: 0000000000000000 ffff80000112c57c

ffff803f6eca7dc0 ffff8000000db7d4

quoted

quoted

[ 2582.403700] 7d80: ffff803f6e696000 ffff803f6e696030

ffff805fdfd0aa18 ffff805fdfd0aa00

quoted

quoted

[ 2582.428385] 7da0: ffff803f6eca4000 ffff80000112c57c

0000000000000000 0000000000000008

quoted

quoted

[ 2582.451222] 7dc0: ffff803f6eca7e20 ffff8000000e1d0c

ffff805f4fc15000 ffff80000115f188

quoted

quoted

[ 2582.474577] 7de0: ffff800000d1cf88 ffff803f6e696000

ffff8000000db690 0000000000000000

quoted

quoted

[ 2582.497281] 7e00: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.522992] 7e20: 0000000000000000 ffff800000083dd0

ffff8000000e1c10 ffff805f4fc15000

quoted

quoted

[ 2582.546945] 7e40: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.568550] 7e60: 0000000000000000 ffff8000000ee33c

ffff803f6e696000 ffff805f00000000

quoted

quoted

[ 2582.589157] 7e80: 0000000000000000 ffff803f6eca7e88

ffff803f6eca7e88 0000000000000000

quoted

quoted

[ 2582.609767] 7ea0: 0000000000000000 ffff803f6eca7ea8

ffff803f6eca7ea8 cb88537fdc8ba31b

quoted

quoted

[ 2582.633228] 7ec0: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.655386] 7ee0: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.674223] 7f00: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.692994] 7f20: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.711765] 7f40: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.730809] 7f60: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.748601] 7f80: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.769388] 7fa0: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.786756] 7fc0: 0000000000000000 0000000000000005

0000000000000000 0000000000000000

quoted

quoted

[ 2582.804134] 7fe0: 0000000000000000 0000000000000000

0000000000000000 0000000000000000

quoted

quoted

[ 2582.821488] Call trace:
[ 2582.834493] [<ffff800000715384>] phy_state_machine+0x3c/0x438
[ 2582.851754] [<ffff8000000db3b8>] process_one_work+0x150/0x428
[ 2582.868188] [<ffff8000000db7d4>] worker_thread+0x144/0x4b0
[ 2582.883882] [<ffff8000000e1d0c>] kthread+0xfc/0x110

============================cut here===============================

The phy_state_machine was still running and didn't know whether the

phydev's driver was

quoted

quoted

removed or not, then occur this problem, so we need to stop the

phy_state_machine when

quoted

quoted

removing the phy driver.

Your explanation of the problem is unclear to me, unless a network
driver attached to the PHY and started it, and then never stopped the
PHY state machine, this should not happen, also there should be

proper

quoted

reference counting in place to avoid that. Your trace is based on 4.1

is

quoted

this still reproducible with latest vanilla? Is this with a mainline
Ethernet driver?

Yes，the network driver already attached to the PHY and started it, and
all
of them could work well if I didn't do anything, but if I suddenly
remove the marvall.ko,
the phy_state_machine was still running, but the phydev->drv is NULL at
this time, than oops,
I found this problem at 4.1 lts, and didn't see any effective
improvement in the kernel 4.8, so
report this bug and fix it.

Ok, then the issue seems to be with reference counting here, we should not be able to unload the module until the last user dropped the reference, which would be when the Ethernet driver called phy_disconnect(). Your fix does not prevent a dangling phy_device pointer from being accessed in the Ethernet driver for instance because the PHY would be silently dropped.

-- 
Florian