* [BISECTED] v4.5-rc1 phylib regression
@ 2016-01-25 15:45 Aaro Koskinen
2016-01-25 16:38 ` Andrew Lunn
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Aaro Koskinen @ 2016-01-25 15:45 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David S. Miller; +Cc: netdev
Hi,
I get the below crash on OCTEON (with octeon_mgmt interface, genphy)
always during systemd boot.
Bisected to:
commit a9049e0c513c4521dbfaa302af8ed08b3366b41f
Author: Andrew Lunn <andrew@lunn.ch>
Date: Wed Jan 6 20:11:26 2016 +0100
mdio: Add support for mdio drivers.
[ 250.179887] CPU 2 Unable to handle kernel paging request at virtual address 0000000000000000, epc == ffffffff81637bac, ra == ffffffff81637b7c
[ 250.218161] Oops[#1]:
[ 250.224970] CPU: 2 PID: 850 Comm: systemd-network Not tainted 4.5.0-rc1-octeon-distro.git-test #1
[ 250.251569] task: 8000000031188000 ti: 800000002f8e8000 task.ti: 800000002f8e8000
[ 250.251586] $ 0 : 0000000000000000 ffffffff81639774 0000000000000000 ffffffffffffffff
[ 250.251595] $ 4 : 0000000000000000 0000000000000000 ffffffff81740000 0000000000000001
[ 250.251604] $ 8 : 0000000000000001 0000000000000000 ffffffff81106100 0000000100000001
[ 250.251613] $12 : 0000000000000000 ffffffff813eb81c ffffffff8150be18 0000000000000000
[ 250.251622] $16 : 8000000031290fc0 ffffffffffffffff 8000000031290fc4 8000000031188000
[ 250.251631] $20 : 0000000000000002 0000000000000001 8000000031290fc8 800000002f8eba40
[ 250.251640] $24 : 0000000000000038 ffffffff81105fb0
[ 250.251649] $28 : 800000002f8e8000 800000002f8eb740 000000fff7857f88 ffffffff81637b7c
[ 250.251651] Hi : 431bde82d7b50717
[ 250.251653] Lo : c8de2ac3222855ea
[ 250.251668] epc : ffffffff81637bac __mutex_lock_slowpath+0x7c/0x190
[ 250.251675] ra : ffffffff81637b7c __mutex_lock_slowpath+0x4c/0x190
[ 250.251684] Status: 10108ce3 KX SX UX KERNEL EXL IE
[ 250.251686] Cause : 0080000c (ExcCode 03)
[ 250.251688] BadVA : 0000000000000000
[ 250.251690] PrId : 000d0409 (Cavium Octeon+)
[ 250.251699] Modules linked in: pata_octeon_cf libata autofs4
[ 250.251704] Process systemd-network (pid: 850, threadinfo=800000002f8e8000, task=8000000031188000, tls=000000fff75ba700)
[ 250.251782] Stack : 8000000031290fc8 0000000000000000 0000000000000001 8000000003f9bca0
[ 250.251782] 8000000031290c00 ffffffff81856558 8000000031290fc0 8000000031225000
[ 250.251782] 0000000000000001 0000000000000000 8000000031008800 ffffffff814b6284
[ 250.251782] ffffffff81639774 ffffffff81175dc4 8000000031290c00 ffffffff814baf80
[ 250.251782] 8000000031210000 ffffffff814b64ac 800000002f8eb810 800000002f8eb810
[ 250.251782] ffffffff814f7790 8000000031290c00 ffffffff814baf80 ffffffff814baf80
[ 250.251782] 8000000031225000 0000000000000000 800000003290ca10 ffffffff814b66a4
[ 250.251782] 8000000031225000 8000000031290c00 0000000000000001 ffffffff814f7b80
[ 250.251782] 8000000031225000 8000000031225788 8000000031225830 0000000000001002
[ 250.251782] 800000002f8ebb80 ffffffff814ba81c 8000000fb7105c32 ffffffff8116aab4
[ 250.251782] ...
[ 250.251784] Call Trace:
[ 250.251791] [<ffffffff81637bac>] __mutex_lock_slowpath+0x7c/0x190
[ 250.251801] [<ffffffff814b6284>] phy_probe+0x6c/0x120
[ 250.251807] [<ffffffff814b64ac>] phy_attach_direct+0xdc/0x1a8
[ 250.251814] [<ffffffff814b66a4>] phy_connect_direct+0x2c/0x98
[ 250.251822] [<ffffffff814f7b80>] of_phy_connect+0x60/0xb8
[ 250.251830] [<ffffffff814ba81c>] octeon_mgmt_open+0x36c/0xad0
[ 250.251838] [<ffffffff81531444>] __dev_open+0x11c/0x1b0
[ 250.251844] [<ffffffff815317b0>] __dev_change_flags+0xa0/0x188
[ 250.251850] [<ffffffff815318c8>] dev_change_flags+0x30/0x78
[ 250.251856] [<ffffffff81543b7c>] do_setlink+0x374/0xa38
[ 250.251862] [<ffffffff8154431c>] rtnl_setlink+0xdc/0x120
[ 250.251868] [<ffffffff81542f6c>] rtnetlink_rcv_msg+0xac/0x2a0
[ 250.251875] [<ffffffff8155f210>] netlink_rcv_skb+0xf0/0x120
[ 250.251880] [<ffffffff81542eb0>] rtnetlink_rcv+0x38/0x48
[ 250.251886] [<ffffffff8155e850>] netlink_unicast+0x1c0/0x2d8
[ 250.251891] [<ffffffff8155eefc>] netlink_sendmsg+0x424/0x480
[ 250.251898] [<ffffffff81509cbc>] sock_sendmsg+0x24/0x40
[ 250.251904] [<ffffffff8150bedc>] SyS_sendto+0xc4/0x108
[ 250.251914] [<ffffffff8112de90>] syscall_common+0x34/0x58
[ 250.251918]
[ 250.251936]
[ 250.251936] Code: 24150001 ffa20008 24140002 <fc5d0000> 0858def5 ffb30010 fe740000 0c58e612 0240202d
[ 250.251983] ---[ end trace 1be6b781dce6d4dc ]---
A.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-25 15:45 [BISECTED] v4.5-rc1 phylib regression Aaro Koskinen @ 2016-01-25 16:38 ` Andrew Lunn 2016-01-25 21:31 ` Andrew Lunn 2016-01-26 4:46 ` Andrew Lunn 2 siblings, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2016-01-25 16:38 UTC (permalink / raw) To: Aaro Koskinen; +Cc: Florian Fainelli, David S. Miller, netdev On Mon, Jan 25, 2016 at 05:45:21PM +0200, Aaro Koskinen wrote: > Hi, > > I get the below crash on OCTEON (with octeon_mgmt interface, genphy) > always during systemd boot. Hi Aaro Olof reported a similar issue with a Marvell Ethernet driver/MDIO driver. Olof thinking was the mutex was used before it was initialised. I will look into this tonight. Thanks Andrew > > Bisected to: > > commit a9049e0c513c4521dbfaa302af8ed08b3366b41f > Author: Andrew Lunn <andrew@lunn.ch> > Date: Wed Jan 6 20:11:26 2016 +0100 > > mdio: Add support for mdio drivers. > > [ 250.179887] CPU 2 Unable to handle kernel paging request at virtual address 0000000000000000, epc == ffffffff81637bac, ra == ffffffff81637b7c > [ 250.218161] Oops[#1]: > [ 250.224970] CPU: 2 PID: 850 Comm: systemd-network Not tainted 4.5.0-rc1-octeon-distro.git-test #1 > [ 250.251569] task: 8000000031188000 ti: 800000002f8e8000 task.ti: 800000002f8e8000 > [ 250.251586] $ 0 : 0000000000000000 ffffffff81639774 0000000000000000 ffffffffffffffff > [ 250.251595] $ 4 : 0000000000000000 0000000000000000 ffffffff81740000 0000000000000001 > [ 250.251604] $ 8 : 0000000000000001 0000000000000000 ffffffff81106100 0000000100000001 > [ 250.251613] $12 : 0000000000000000 ffffffff813eb81c ffffffff8150be18 0000000000000000 > [ 250.251622] $16 : 8000000031290fc0 ffffffffffffffff 8000000031290fc4 8000000031188000 > [ 250.251631] $20 : 0000000000000002 0000000000000001 8000000031290fc8 800000002f8eba40 > [ 250.251640] $24 : 0000000000000038 ffffffff81105fb0 > [ 250.251649] $28 : 800000002f8e8000 800000002f8eb740 000000fff7857f88 ffffffff81637b7c > [ 250.251651] Hi : 431bde82d7b50717 > [ 250.251653] Lo : c8de2ac3222855ea > [ 250.251668] epc : ffffffff81637bac __mutex_lock_slowpath+0x7c/0x190 > [ 250.251675] ra : ffffffff81637b7c __mutex_lock_slowpath+0x4c/0x190 > [ 250.251684] Status: 10108ce3 KX SX UX KERNEL EXL IE > [ 250.251686] Cause : 0080000c (ExcCode 03) > [ 250.251688] BadVA : 0000000000000000 > [ 250.251690] PrId : 000d0409 (Cavium Octeon+) > [ 250.251699] Modules linked in: pata_octeon_cf libata autofs4 > [ 250.251704] Process systemd-network (pid: 850, threadinfo=800000002f8e8000, task=8000000031188000, tls=000000fff75ba700) > [ 250.251782] Stack : 8000000031290fc8 0000000000000000 0000000000000001 8000000003f9bca0 > [ 250.251782] 8000000031290c00 ffffffff81856558 8000000031290fc0 8000000031225000 > [ 250.251782] 0000000000000001 0000000000000000 8000000031008800 ffffffff814b6284 > [ 250.251782] ffffffff81639774 ffffffff81175dc4 8000000031290c00 ffffffff814baf80 > [ 250.251782] 8000000031210000 ffffffff814b64ac 800000002f8eb810 800000002f8eb810 > [ 250.251782] ffffffff814f7790 8000000031290c00 ffffffff814baf80 ffffffff814baf80 > [ 250.251782] 8000000031225000 0000000000000000 800000003290ca10 ffffffff814b66a4 > [ 250.251782] 8000000031225000 8000000031290c00 0000000000000001 ffffffff814f7b80 > [ 250.251782] 8000000031225000 8000000031225788 8000000031225830 0000000000001002 > [ 250.251782] 800000002f8ebb80 ffffffff814ba81c 8000000fb7105c32 ffffffff8116aab4 > [ 250.251782] ... > [ 250.251784] Call Trace: > [ 250.251791] [<ffffffff81637bac>] __mutex_lock_slowpath+0x7c/0x190 > [ 250.251801] [<ffffffff814b6284>] phy_probe+0x6c/0x120 > [ 250.251807] [<ffffffff814b64ac>] phy_attach_direct+0xdc/0x1a8 > [ 250.251814] [<ffffffff814b66a4>] phy_connect_direct+0x2c/0x98 > [ 250.251822] [<ffffffff814f7b80>] of_phy_connect+0x60/0xb8 > [ 250.251830] [<ffffffff814ba81c>] octeon_mgmt_open+0x36c/0xad0 > [ 250.251838] [<ffffffff81531444>] __dev_open+0x11c/0x1b0 > [ 250.251844] [<ffffffff815317b0>] __dev_change_flags+0xa0/0x188 > [ 250.251850] [<ffffffff815318c8>] dev_change_flags+0x30/0x78 > [ 250.251856] [<ffffffff81543b7c>] do_setlink+0x374/0xa38 > [ 250.251862] [<ffffffff8154431c>] rtnl_setlink+0xdc/0x120 > [ 250.251868] [<ffffffff81542f6c>] rtnetlink_rcv_msg+0xac/0x2a0 > [ 250.251875] [<ffffffff8155f210>] netlink_rcv_skb+0xf0/0x120 > [ 250.251880] [<ffffffff81542eb0>] rtnetlink_rcv+0x38/0x48 > [ 250.251886] [<ffffffff8155e850>] netlink_unicast+0x1c0/0x2d8 > [ 250.251891] [<ffffffff8155eefc>] netlink_sendmsg+0x424/0x480 > [ 250.251898] [<ffffffff81509cbc>] sock_sendmsg+0x24/0x40 > [ 250.251904] [<ffffffff8150bedc>] SyS_sendto+0xc4/0x108 > [ 250.251914] [<ffffffff8112de90>] syscall_common+0x34/0x58 > [ 250.251918] > [ 250.251936] > [ 250.251936] Code: 24150001 ffa20008 24140002 <fc5d0000> 0858def5 ffb30010 fe740000 0c58e612 0240202d > [ 250.251983] ---[ end trace 1be6b781dce6d4dc ]--- > > A. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-25 15:45 [BISECTED] v4.5-rc1 phylib regression Aaro Koskinen 2016-01-25 16:38 ` Andrew Lunn @ 2016-01-25 21:31 ` Andrew Lunn 2016-01-26 4:46 ` Andrew Lunn 2 siblings, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2016-01-25 21:31 UTC (permalink / raw) To: Aaro Koskinen, Olof Johansson; +Cc: Florian Fainelli, David S. Miller, netdev On Mon, Jan 25, 2016 at 05:45:21PM +0200, Aaro Koskinen wrote: > Hi, > > I get the below crash on OCTEON (with octeon_mgmt interface, genphy) > always during systemd boot. > > Bisected to: > > commit a9049e0c513c4521dbfaa302af8ed08b3366b41f > Author: Andrew Lunn <andrew@lunn.ch> > Date: Wed Jan 6 20:11:26 2016 +0100 > > mdio: Add support for mdio drivers. > > [ 250.179887] CPU 2 Unable to handle kernel paging request at virtual address 0000000000000000, epc == ffffffff81637bac, ra == ffffffff81637b7c > [ 250.218161] Oops[#1]: > [ 250.224970] CPU: 2 PID: 850 Comm: systemd-network Not tainted 4.5.0-rc1-octeon-distro.git-test #1 > [ 250.251569] task: 8000000031188000 ti: 800000002f8e8000 task.ti: 800000002f8e8000 > [ 250.251586] $ 0 : 0000000000000000 ffffffff81639774 0000000000000000 ffffffffffffffff > [ 250.251595] $ 4 : 0000000000000000 0000000000000000 ffffffff81740000 0000000000000001 > [ 250.251604] $ 8 : 0000000000000001 0000000000000000 ffffffff81106100 0000000100000001 > [ 250.251613] $12 : 0000000000000000 ffffffff813eb81c ffffffff8150be18 0000000000000000 > [ 250.251622] $16 : 8000000031290fc0 ffffffffffffffff 8000000031290fc4 8000000031188000 > [ 250.251631] $20 : 0000000000000002 0000000000000001 8000000031290fc8 800000002f8eba40 > [ 250.251640] $24 : 0000000000000038 ffffffff81105fb0 > [ 250.251649] $28 : 800000002f8e8000 800000002f8eb740 000000fff7857f88 ffffffff81637b7c > [ 250.251651] Hi : 431bde82d7b50717 > [ 250.251653] Lo : c8de2ac3222855ea > [ 250.251668] epc : ffffffff81637bac __mutex_lock_slowpath+0x7c/0x190 > [ 250.251675] ra : ffffffff81637b7c __mutex_lock_slowpath+0x4c/0x190 > [ 250.251684] Status: 10108ce3 KX SX UX KERNEL EXL IE > [ 250.251686] Cause : 0080000c (ExcCode 03) > [ 250.251688] BadVA : 0000000000000000 > [ 250.251690] PrId : 000d0409 (Cavium Octeon+) > [ 250.251699] Modules linked in: pata_octeon_cf libata autofs4 > [ 250.251704] Process systemd-network (pid: 850, threadinfo=800000002f8e8000, task=8000000031188000, tls=000000fff75ba700) > [ 250.251782] Stack : 8000000031290fc8 0000000000000000 0000000000000001 8000000003f9bca0 > [ 250.251782] 8000000031290c00 ffffffff81856558 8000000031290fc0 8000000031225000 > [ 250.251782] 0000000000000001 0000000000000000 8000000031008800 ffffffff814b6284 > [ 250.251782] ffffffff81639774 ffffffff81175dc4 8000000031290c00 ffffffff814baf80 > [ 250.251782] 8000000031210000 ffffffff814b64ac 800000002f8eb810 800000002f8eb810 > [ 250.251782] ffffffff814f7790 8000000031290c00 ffffffff814baf80 ffffffff814baf80 > [ 250.251782] 8000000031225000 0000000000000000 800000003290ca10 ffffffff814b66a4 > [ 250.251782] 8000000031225000 8000000031290c00 0000000000000001 ffffffff814f7b80 > [ 250.251782] 8000000031225000 8000000031225788 8000000031225830 0000000000001002 > [ 250.251782] 800000002f8ebb80 ffffffff814ba81c 8000000fb7105c32 ffffffff8116aab4 > [ 250.251782] ... > [ 250.251784] Call Trace: > [ 250.251791] [<ffffffff81637bac>] __mutex_lock_slowpath+0x7c/0x190 > [ 250.251801] [<ffffffff814b6284>] phy_probe+0x6c/0x120 > [ 250.251807] [<ffffffff814b64ac>] phy_attach_direct+0xdc/0x1a8 > [ 250.251814] [<ffffffff814b66a4>] phy_connect_direct+0x2c/0x98 > [ 250.251822] [<ffffffff814f7b80>] of_phy_connect+0x60/0xb8 > [ 250.251830] [<ffffffff814ba81c>] octeon_mgmt_open+0x36c/0xad0 Hi Aaro, Olof I've not been able to reproduce this. I've tried a Kirkwood Qnap, a Kirkwood DIR665, an Armada XP WRT1900AC and a Freescale Vybrid. And i've turned on mutex debugging.... Nothing, always boots to user space, no complaints. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-25 15:45 [BISECTED] v4.5-rc1 phylib regression Aaro Koskinen 2016-01-25 16:38 ` Andrew Lunn 2016-01-25 21:31 ` Andrew Lunn @ 2016-01-26 4:46 ` Andrew Lunn 2016-01-26 12:14 ` Aaro Koskinen 2 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-01-26 4:46 UTC (permalink / raw) To: Aaro Koskinen; +Cc: Florian Fainelli, David S. Miller, netdev On Mon, Jan 25, 2016 at 05:45:21PM +0200, Aaro Koskinen wrote: > Hi, > > I get the below crash on OCTEON (with octeon_mgmt interface, genphy) > always during systemd boot. Hi Aaro I think i know what is going on now. What does your phy look like in DT? Thanks Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 4:46 ` Andrew Lunn @ 2016-01-26 12:14 ` Aaro Koskinen 2016-01-26 13:34 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Aaro Koskinen @ 2016-01-26 12:14 UTC (permalink / raw) To: Andrew Lunn; +Cc: Florian Fainelli, David S. Miller, netdev Hi, On Tue, Jan 26, 2016 at 05:46:24AM +0100, Andrew Lunn wrote: > On Mon, Jan 25, 2016 at 05:45:21PM +0200, Aaro Koskinen wrote: > > I get the below crash on OCTEON (with octeon_mgmt interface, genphy) > > always during systemd boot. > > I think i know what is going on now. > > What does your phy look like in DT? It's using the in-kernel DT: arch/mips/boot/dts/cavium-octeon/octeon_3xxx.dts The management interface is "mix0: ethernet@1070000100000". The phy entry might be bogus for this specific board, and I don't have MARVELL_PHY enabled... A. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 12:14 ` Aaro Koskinen @ 2016-01-26 13:34 ` Andrew Lunn 2016-01-26 17:23 ` Olof Johansson 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-01-26 13:34 UTC (permalink / raw) To: Aaro Koskinen, Olof Johansson, Florian Fainelli Cc: Florian Fainelli, David S. Miller, netdev On Tue, Jan 26, 2016 at 02:14:35PM +0200, Aaro Koskinen wrote: > Hi, > > On Tue, Jan 26, 2016 at 05:46:24AM +0100, Andrew Lunn wrote: > > On Mon, Jan 25, 2016 at 05:45:21PM +0200, Aaro Koskinen wrote: > > > I get the below crash on OCTEON (with octeon_mgmt interface, genphy) > > > always during systemd boot. > > > > I think i know what is going on now. > > > > What does your phy look like in DT? > > It's using the in-kernel DT: > > arch/mips/boot/dts/cavium-octeon/octeon_3xxx.dts > > The management interface is "mix0: ethernet@1070000100000". The phy entry > might be bogus for this specific board, and I don't have MARVELL_PHY > enabled... phy1: ethernet-phy@1 { cavium,qlm-trim = "4,sgmii"; reg = <1>; compatible = "marvell,88e1149r"; marvell,reg-init = <3 0x10 0 0x5777>, <3 0x11 0 0x00aa>, <3 0x12 0 0x4105>, <3 0x13 0 0x0a60>; }; Dove Cubox is the other board which Olof has problems with. It has ðphy { compatible = "marvell,88e1310"; reg = <1>; }; The issue here is the compatible string. The binding says: Optional Properties: - compatible: Compatible list, may contain "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45 specifications. If neither of these are specified, the default is to assume clause 22. The compatible list may also contain other elements. If the phy's identifier is known then the list may contain an entry of the form: "ethernet-phy-idAAAA.BBBB" where AAAA - The value of the 16 bit Phy Identifier 1 register as 4 hex digits. This is the chip vendor OUI bits 3:18 BBBB - The value of the 16 bit Phy Identifier 2 register as 4 hex digits. This is the chip vendor OUI bits 19:24, followed by 10 bits of a vendor specific ID. We are in a grey area, it does not say you can specifically exactly what PHY you have, but it also does not rule it out, since "may also contain other elements". When adding support for generic MDIO device, like switches, i have to differentiate between PHYs and generic MDIO devices in DT. What i implemented is: /* * Return true if the child node is for a phy. It must either: * o Compatible string of "ethernet-phy-idX.X" * o Compatible string of "ethernet-phy-ieee802.3-c45" * o Compatible string of "ethernet-phy-ieee802.3-c22" * o No compatibility string * * A device which is not a phy is expected to have a compatible string * indicating what sort of device it is. */ static bool of_mdiobus_child_is_phy(struct device_node *child) { The last rule is the issue. Before this patch, saying "marvell,88e1149r" would be totally ignored, never used. The PHY drivers don't have an of_device_id table to match against. Now it means the device is a generic MDIO device, and use the compatible string to find the correct driver for it. The first part of the fix is clear. If we have a generic MDIO device, but somebody asks for a PHY device, return an error. I've not checked the code paths yet, but i expect the generic MDIO device is being returned, container_of() into a phydev, and then the non-existent mutex is lock. The code already have a flag to indicate if it is a PHY or not, so it looks like a check is missing somewhere. The harder problem, is what to do with these compatible strings, and how to decide if we have a generic MDIO device, or a PHY. Grep'ing the DTS files, it seems to be an issue for octeon_68xx.dts, octeon_3xxx.dts, k2e-evm.dts, k2he-evm.dts, k2l-evm.dts, kirkwood-dockstar.dts, moxart-uc7112lx.dts, dove-cubox.dts and maybe others my grep foo missed. This is too many to ignore. Having a useless compatible string for a PHY needs to be supported. So i guess i need a bool property, "generic-mdio", and assume anything without that is a PHY. Florian, are you O.K. with this? Thanks Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 13:34 ` Andrew Lunn @ 2016-01-26 17:23 ` Olof Johansson 2016-01-26 17:53 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Olof Johansson @ 2016-01-26 17:23 UTC (permalink / raw) To: Andrew Lunn Cc: Aaro Koskinen, Florian Fainelli, David S. Miller, Network Development On Tue, Jan 26, 2016 at 5:34 AM, Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Jan 26, 2016 at 02:14:35PM +0200, Aaro Koskinen wrote: >> Hi, >> >> On Tue, Jan 26, 2016 at 05:46:24AM +0100, Andrew Lunn wrote: >> > On Mon, Jan 25, 2016 at 05:45:21PM +0200, Aaro Koskinen wrote: >> > > I get the below crash on OCTEON (with octeon_mgmt interface, genphy) >> > > always during systemd boot. >> > >> > I think i know what is going on now. >> > >> > What does your phy look like in DT? >> >> It's using the in-kernel DT: >> >> arch/mips/boot/dts/cavium-octeon/octeon_3xxx.dts >> >> The management interface is "mix0: ethernet@1070000100000". The phy entry >> might be bogus for this specific board, and I don't have MARVELL_PHY >> enabled... > > phy1: ethernet-phy@1 { > cavium,qlm-trim = "4,sgmii"; > reg = <1>; > compatible = "marvell,88e1149r"; > marvell,reg-init = <3 0x10 0 0x5777>, > <3 0x11 0 0x00aa>, > <3 0x12 0 0x4105>, > <3 0x13 0 0x0a60>; > }; > > Dove Cubox is the other board which Olof has problems with. It has > > ðphy { > compatible = "marvell,88e1310"; > reg = <1>; > }; > > The issue here is the compatible string. The binding says: > > Optional Properties: > > - compatible: Compatible list, may contain > "ethernet-phy-ieee802.3-c22" or "ethernet-phy-ieee802.3-c45" for > PHYs that implement IEEE802.3 clause 22 or IEEE802.3 clause 45 > specifications. If neither of these are specified, the default is to > assume clause 22. The compatible list may also contain other > elements. > > If the phy's identifier is known then the list may contain an entry > of the form: "ethernet-phy-idAAAA.BBBB" where > AAAA - The value of the 16 bit Phy Identifier 1 register as > 4 hex digits. This is the chip vendor OUI bits 3:18 > BBBB - The value of the 16 bit Phy Identifier 2 register as > 4 hex digits. This is the chip vendor OUI bits 19:24, > followed by 10 bits of a vendor specific ID. > > We are in a grey area, it does not say you can specifically exactly > what PHY you have, but it also does not rule it out, since "may also > contain other elements". > > When adding support for generic MDIO device, like switches, i have to > differentiate between PHYs and generic MDIO devices in DT. What i > implemented is: > > /* > * Return true if the child node is for a phy. It must either: > * o Compatible string of "ethernet-phy-idX.X" > * o Compatible string of "ethernet-phy-ieee802.3-c45" > * o Compatible string of "ethernet-phy-ieee802.3-c22" > * o No compatibility string > * > * A device which is not a phy is expected to have a compatible string > * indicating what sort of device it is. > */ > static bool of_mdiobus_child_is_phy(struct device_node *child) > { > > The last rule is the issue. Before this patch, saying > "marvell,88e1149r" would be totally ignored, never used. The PHY > drivers don't have an of_device_id table to match against. Now it > means the device is a generic MDIO device, and use the compatible > string to find the correct driver for it. > > The first part of the fix is clear. If we have a generic MDIO device, > but somebody asks for a PHY device, return an error. I've not checked > the code paths yet, but i expect the generic MDIO device is being > returned, container_of() into a phydev, and then the non-existent > mutex is lock. The code already have a flag to indicate if it is a PHY > or not, so it looks like a check is missing somewhere. > > The harder problem, is what to do with these compatible strings, and > how to decide if we have a generic MDIO device, or a PHY. Grep'ing the > DTS files, it seems to be an issue for octeon_68xx.dts, > octeon_3xxx.dts, k2e-evm.dts, k2he-evm.dts, k2l-evm.dts, > kirkwood-dockstar.dts, moxart-uc7112lx.dts, dove-cubox.dts and maybe > others my grep foo missed. > > This is too many to ignore. Having a useless compatible string for a > PHY needs to be supported. So i guess i need a bool property, > "generic-mdio", and assume anything without that is a PHY. > > Florian, are you O.K. with this? General solution of having a boolean property to indicate that the mdio device is not a phy sounds good to me. Alternatively, have a generic compatible that all non-phys must set (same way as you're expecting the ethernet-phy-id* above). Either way is alright with me. I hate to bikeshed, but I'm not sure if "generic-mdio" is too... generic? Will someone writing a DT be thinking "well, this is a generic mdio PHY, I should set it"? "mdio-device"? "generic-nonphy-mdio"? Neither of those seem much better. -Olof ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 17:23 ` Olof Johansson @ 2016-01-26 17:53 ` Andrew Lunn 2016-01-26 18:08 ` Olof Johansson 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-01-26 17:53 UTC (permalink / raw) To: Olof Johansson Cc: Aaro Koskinen, Florian Fainelli, David S. Miller, Network Development > I hate to bikeshed, but I'm not sure if "generic-mdio" is too... > generic? Will someone writing a DT be thinking "well, this is a > generic mdio PHY, I should set it"? "mdio-device"? > "generic-nonphy-mdio"? Neither of those seem much better. How about 'not-a-phy'? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 17:53 ` Andrew Lunn @ 2016-01-26 18:08 ` Olof Johansson 2016-01-26 18:14 ` Olof Johansson 0 siblings, 1 reply; 16+ messages in thread From: Olof Johansson @ 2016-01-26 18:08 UTC (permalink / raw) To: Andrew Lunn Cc: Aaro Koskinen, Florian Fainelli, David S. Miller, Network Development, devicetree-discuss On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew@lunn.ch> wrote: >> I hate to bikeshed, but I'm not sure if "generic-mdio" is too... >> generic? Will someone writing a DT be thinking "well, this is a >> generic mdio PHY, I should set it"? "mdio-device"? >> "generic-nonphy-mdio"? Neither of those seem much better. > > How about 'not-a-phy'? "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if you expect other generic mdio properties that can do with a namespace. Probably time to add devicetree-discuss. Doing so. -Olof ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 18:08 ` Olof Johansson @ 2016-01-26 18:14 ` Olof Johansson [not found] ` <CAOesGMi+ymn_FdDfgTjwgyO-MsEJNGBDU9ayRJvGkjJ7T_ExfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Olof Johansson @ 2016-01-26 18:14 UTC (permalink / raw) To: Andrew Lunn Cc: Aaro Koskinen, Florian Fainelli, David S. Miller, Network Development, devicetree +devicetree@vger.kernel.org instead. On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof@lixom.net> wrote: > On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew@lunn.ch> wrote: >>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too... >>> generic? Will someone writing a DT be thinking "well, this is a >>> generic mdio PHY, I should set it"? "mdio-device"? >>> "generic-nonphy-mdio"? Neither of those seem much better. >> >> How about 'not-a-phy'? > > "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if > you expect other generic mdio properties that can do with a namespace. > > Probably time to add devicetree-discuss. Doing so. > > > -Olof ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <CAOesGMi+ymn_FdDfgTjwgyO-MsEJNGBDU9ayRJvGkjJ7T_ExfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [BISECTED] v4.5-rc1 phylib regression [not found] ` <CAOesGMi+ymn_FdDfgTjwgyO-MsEJNGBDU9ayRJvGkjJ7T_ExfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2016-01-26 20:46 ` Florian Fainelli [not found] ` <56A7DB27.6080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Florian Fainelli @ 2016-01-26 20:46 UTC (permalink / raw) To: Olof Johansson, Andrew Lunn Cc: Aaro Koskinen, David S. Miller, Network Development, devicetree-u79uwXL29TY76Z2rM5mHXA On 26/01/16 10:14, Olof Johansson wrote: > +devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org instead. > > On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote: >> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote: >>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too... >>>> generic? Will someone writing a DT be thinking "well, this is a >>>> generic mdio PHY, I should set it"? "mdio-device"? >>>> "generic-nonphy-mdio"? Neither of those seem much better. >>> >>> How about 'not-a-phy'? >> >> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if >> you expect other generic mdio properties that can do with a namespace. Really not a fan of having to add an additional boolean property to differentiate an Ethernet PHY from something else, the proper solution would really be to enforce the use of the c22 or c45 compatible string as the least compatible string to be used, but I am assuming this is not necessarily an option here with DTBs out there. What plays in favor of this boolean property is that the very concept of MDIO devices has been recently introduced, so presumably, there are not that many DTBs out there which would be affected... The only other idea I had was to force the MDIO device creation to be dependent on finding a matching compatible string provided by a driver (yikes). -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <56A7DB27.6080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [BISECTED] v4.5-rc1 phylib regression [not found] ` <56A7DB27.6080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-01-26 21:36 ` Andrew Lunn 2016-01-26 21:48 ` Olof Johansson 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-01-26 21:36 UTC (permalink / raw) To: Florian Fainelli Cc: Olof Johansson, Aaro Koskinen, David S. Miller, Network Development, devicetree-u79uwXL29TY76Z2rM5mHXA On Tue, Jan 26, 2016 at 12:46:31PM -0800, Florian Fainelli wrote: > On 26/01/16 10:14, Olof Johansson wrote: > > +devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org instead. > > > > On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote: > >> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote: > >>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too... > >>>> generic? Will someone writing a DT be thinking "well, this is a > >>>> generic mdio PHY, I should set it"? "mdio-device"? > >>>> "generic-nonphy-mdio"? Neither of those seem much better. > >>> > >>> How about 'not-a-phy'? > >> > >> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if > >> you expect other generic mdio properties that can do with a namespace. > > Really not a fan of having to add an additional boolean property to > differentiate an Ethernet PHY from something else, the proper solution > would really be to enforce the use of the c22 or c45 compatible string > as the least compatible string to be used, but I am assuming this is not > necessarily an option here with DTBs out there. Nope, not an option. Only a small number of DTB actually use c22 or c45. The majority of devices have no compatible at all. Why should they, the binding documentation says it is optional! > What plays in favor of this boolean property is that the very concept of > MDIO devices has been recently introduced, so presumably, there are not > that many DTBs out there which would be affected... In kernel, 0. I have one out of kernel, which i hope to contribute once we decide on the new binding for DSA. > The only other idea I had was to force the MDIO device creation to be > dependent on finding a matching compatible string provided by a driver > (yikes). Complex. There are ordering issues, since the driver can be loaded a long time after of_mdiobus_register() is called, yet it needs to be of_mdiobus_register() which decides if a device is a PHY or not. I think the bool is the only practical solution. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 21:36 ` Andrew Lunn @ 2016-01-26 21:48 ` Olof Johansson 2016-01-26 22:09 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Olof Johansson @ 2016-01-26 21:48 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, Aaro Koskinen, David S. Miller, Network Development, devicetree On Tue, Jan 26, 2016 at 1:36 PM, Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Jan 26, 2016 at 12:46:31PM -0800, Florian Fainelli wrote: >> On 26/01/16 10:14, Olof Johansson wrote: >> > +devicetree@vger.kernel.org instead. >> > >> > On Tue, Jan 26, 2016 at 10:08 AM, Olof Johansson <olof@lixom.net> wrote: >> >> On Tue, Jan 26, 2016 at 9:53 AM, Andrew Lunn <andrew@lunn.ch> wrote: >> >>>> I hate to bikeshed, but I'm not sure if "generic-mdio" is too... >> >>>> generic? Will someone writing a DT be thinking "well, this is a >> >>>> generic mdio PHY, I should set it"? "mdio-device"? >> >>>> "generic-nonphy-mdio"? Neither of those seem much better. >> >>> >> >>> How about 'not-a-phy'? >> >> >> >> "mdio,not-a-phy" or "mdio,non-phy" will scope it a bit, especially if >> >> you expect other generic mdio properties that can do with a namespace. >> >> Really not a fan of having to add an additional boolean property to >> differentiate an Ethernet PHY from something else, the proper solution >> would really be to enforce the use of the c22 or c45 compatible string >> as the least compatible string to be used, but I am assuming this is not >> necessarily an option here with DTBs out there. > > Nope, not an option. Only a small number of DTB actually use c22 or > c45. The majority of devices have no compatible at all. Why should > they, the binding documentation says it is optional! So one thing that can be done is to just have a whitelist in the driver that we add the known phy compatibles to, with a nice comment above that this should only be for legacy device trees. I.e. in addition to the c22 or c45 strings. You can even fix up existing trees, and do a pr_warn() for these cases to ask people to update their DT. There's no strict requirement to do so though, so the kernel still *has* to work with the old ones. >> What plays in favor of this boolean property is that the very concept of >> MDIO devices has been recently introduced, so presumably, there are not >> that many DTBs out there which would be affected... > > In kernel, 0. I have one out of kernel, which i hope to contribute > once we decide on the new binding for DSA. > >> The only other idea I had was to force the MDIO device creation to be >> dependent on finding a matching compatible string provided by a driver >> (yikes). > > Complex. There are ordering issues, since the driver can be loaded a > long time after of_mdiobus_register() is called, yet it needs to be > of_mdiobus_register() which decides if a device is a PHY or not. > > I think the bool is the only practical solution. -Olof ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 21:48 ` Olof Johansson @ 2016-01-26 22:09 ` Andrew Lunn 2016-01-26 23:54 ` Florian Fainelli 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2016-01-26 22:09 UTC (permalink / raw) To: Olof Johansson Cc: Florian Fainelli, Aaro Koskinen, David S. Miller, Network Development, devicetree > > Nope, not an option. Only a small number of DTB actually use c22 or > > c45. The majority of devices have no compatible at all. Why should > > they, the binding documentation says it is optional! > > So one thing that can be done is to just have a whitelist in the > driver that we add the known phy compatibles to, with a nice comment > above that this should only be for legacy device trees. So you mean drivers/of/of_mdio.c:of_mdiobus_child_is_phy() has a white list like: "brcm,40nm-ephy" "marvell,88E1111", "marvell,88e1116", "marvell,88e1118", "marvell,88e1149r", "marvell,88e1310", "marvell,88E1510", "marvell,88E1514", "moxa,moxart-rtl8201cp", Yes, that would work. We should also update the binding documentation to limit what is legal in the compatible string. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [BISECTED] v4.5-rc1 phylib regression 2016-01-26 22:09 ` Andrew Lunn @ 2016-01-26 23:54 ` Florian Fainelli [not found] ` <56A80725.1070808-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 16+ messages in thread From: Florian Fainelli @ 2016-01-26 23:54 UTC (permalink / raw) To: Andrew Lunn, Olof Johansson Cc: Aaro Koskinen, David S. Miller, Network Development, devicetree On 26/01/16 14:09, Andrew Lunn wrote: >>> Nope, not an option. Only a small number of DTB actually use c22 or >>> c45. The majority of devices have no compatible at all. Why should >>> they, the binding documentation says it is optional! >> >> So one thing that can be done is to just have a whitelist in the >> driver that we add the known phy compatibles to, with a nice comment >> above that this should only be for legacy device trees. > > So you mean drivers/of/of_mdio.c:of_mdiobus_child_is_phy() > has a white list like: > > "brcm,40nm-ephy" > "marvell,88E1111", > "marvell,88e1116", > "marvell,88e1118", > "marvell,88e1149r", > "marvell,88e1310", > "marvell,88E1510", > "marvell,88E1514", > "moxa,moxart-rtl8201cp", > > Yes, that would work. > > We should also update the binding documentation to limit what is legal > in the compatible string. Agreed, and while at it, take the opportunity to make the compatible string clause 22/45 mandatory properties so we do not multiply the whitelist. Thanks! -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <56A80725.1070808-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [BISECTED] v4.5-rc1 phylib regression [not found] ` <56A80725.1070808-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2016-01-27 0:15 ` Andrew Lunn 0 siblings, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2016-01-27 0:15 UTC (permalink / raw) To: Florian Fainelli Cc: Olof Johansson, Aaro Koskinen, David S. Miller, Network Development, devicetree-u79uwXL29TY76Z2rM5mHXA On Tue, Jan 26, 2016 at 03:54:13PM -0800, Florian Fainelli wrote: > On 26/01/16 14:09, Andrew Lunn wrote: > >>> Nope, not an option. Only a small number of DTB actually use c22 or > >>> c45. The majority of devices have no compatible at all. Why should > >>> they, the binding documentation says it is optional! > >> > >> So one thing that can be done is to just have a whitelist in the > >> driver that we add the known phy compatibles to, with a nice comment > >> above that this should only be for legacy device trees. > > > > So you mean drivers/of/of_mdio.c:of_mdiobus_child_is_phy() > > has a white list like: > > > > "brcm,40nm-ephy" > > "marvell,88E1111", > > "marvell,88e1116", > > "marvell,88e1118", > > "marvell,88e1149r", > > "marvell,88e1310", > > "marvell,88E1510", > > "marvell,88E1514", > > "moxa,moxart-rtl8201cp", > > > > Yes, that would work. > > > > We should also update the binding documentation to limit what is legal > > in the compatible string. > > Agreed, and while at it, take the opportunity to make the compatible > string clause 22/45 mandatory properties so we do not multiply the > whitelist. Hi Florian I just posted the whitelist code. Making clause 22/45 mandatory is going further than a fix. So i've left this out for the moment. We can add it to net-next later. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-01-27 0:15 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-25 15:45 [BISECTED] v4.5-rc1 phylib regression Aaro Koskinen
2016-01-25 16:38 ` Andrew Lunn
2016-01-25 21:31 ` Andrew Lunn
2016-01-26 4:46 ` Andrew Lunn
2016-01-26 12:14 ` Aaro Koskinen
2016-01-26 13:34 ` Andrew Lunn
2016-01-26 17:23 ` Olof Johansson
2016-01-26 17:53 ` Andrew Lunn
2016-01-26 18:08 ` Olof Johansson
2016-01-26 18:14 ` Olof Johansson
[not found] ` <CAOesGMi+ymn_FdDfgTjwgyO-MsEJNGBDU9ayRJvGkjJ7T_ExfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-01-26 20:46 ` Florian Fainelli
[not found] ` <56A7DB27.6080203-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-01-26 21:36 ` Andrew Lunn
2016-01-26 21:48 ` Olof Johansson
2016-01-26 22:09 ` Andrew Lunn
2016-01-26 23:54 ` Florian Fainelli
[not found] ` <56A80725.1070808-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-01-27 0:15 ` Andrew Lunn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.