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

On August 2, 2016 9:22:53 PM MST, Ding Tianhong [off-list ref] wrote:

On 2016/8/3 10:41, Florian Fainelli wrote:

quoted

On August 2, 2016 6:21:52 PM MST, Ding Tianhong

[off-list ref] wrote:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

unloaded: marvell]

quoted

quoted

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

quoted

quoted

    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:

quoted

quoted

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

quoted

quoted

quoted

quoted

The phy_state_machine was still running and didn't know whether

the

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

is

quoted

this still reproducible with latest vanilla? Is this with a

mainline

quoted

quoted

quoted

Ethernet driver?

Yes，the network driver already attached to the PHY and started it,

and

quoted

quoted

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

quoted

quoted

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

quoted

the module until the last user dropped the reference, which would be

when the Ethernet driver 

quoted

called phy_disconnect(). Your fix does not prevent a dangling

phy_device pointer from being 

quoted

accessed in the Ethernet driver for instance because the PHY would be

silently dropped.

quoted

I couldn't stop the root or other user to do the stupid action just
like unload a drivers, but we need to make sure
the system could still work and avoid the more serious consequences,
and I think the phy_remove has done a lot of
things for unloading action, but still miss to stop the
phy_state_machine.

Maybe you could give me more suggestion how to fix this problem, thanks
very much for any opinion.

My previous email was giving some directions, if the module use count is zero while a network device is attached to a PHY, then we want to fix that.

-- 
Florian