| Message ID | 20231205103559.9605-11-fancer.lancer@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: Add memory-based management iface support | expand |
On Tue, Dec 05, 2023 at 01:35:31PM +0300, Serge Semin wrote: > @@ -1436,21 +1480,32 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, > struct dw_xpcs *xpcs; > int ret; > > + ret = device_attach(&mdiodev->dev); > + if (ret < 0 && ret != -ENODEV) > + return ERR_PTR(ret); > + > xpcs = xpcs_create_data(mdiodev); > if (IS_ERR(xpcs)) > return xpcs; > > + ret = xpcs_init_clks(xpcs); > + if (ret) > + goto out_free_data; > + > ret = xpcs_init_id(xpcs); > if (ret) > - goto out; > + goto out_clear_clks; > > ret = xpcs_init_iface(xpcs, interface); > if (ret) > - goto out; > + goto out_clear_clks; > > return xpcs; [ 4.083518] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000d0 [ 4.092356] Mem abort info: [ 4.095164] ESR = 0x0000000096000004 [ 4.098932] EC = 0x25: DABT (current EL), IL = 32 bits [ 4.104277] SET = 0, FnV = 0 [ 4.107352] EA = 0, S1PTW = 0 [ 4.110505] FSC = 0x04: level 0 translation fault [ 4.115408] Data abort info: [ 4.118296] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 [ 4.123807] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 4.128877] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 4.134214] [00000000000000d0] user address but active_mm is swapper [ 4.140595] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP [ 4.146882] Modules linked in: [ 4.149944] CPU: 0 PID: 11 Comm: kworker/u4:0 Not tainted 6.7.0-rc3-00719-g75be5ea8e111-dirty #1551 [ 4.164524] Workqueue: events_unbound deferred_probe_work_func [ 4.177372] pc : __device_attach+0x3c/0x1bc [ 4.181570] lr : __device_attach+0x38/0x1bc [ 4.185767] sp : ffff8000800f3800 [ 4.189087] x29: ffff8000800f3820 x28: 0000000000000001 x27: ffff063781bda150 [ 4.196252] x26: ffff063781bda150 x25: ffff063780827480 x24: ffffcb9a08138a40 [ 4.203416] x23: ffff063781114080 x22: 0000000000000000 x21: 0000000000000004 [ 4.210579] x20: ffff06378123a400 x19: ffff06378123a480 x18: ffffcb9a07c703a0 [ 4.217743] x17: ffffcb9a07c703a4 x16: 00000000000000d4 x15: ffffcb9a07be70fc [ 4.224906] x14: ffffcb9a08299638 x13: 0000000000000053 x12: ffff003000000200 [ 4.232069] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 [ 4.239233] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003a [ 4.246396] x5 : ffff0637809a037b x4 : ffffcb9a087b0d47 x3 : ffff10300000020f [ 4.253560] x2 : ffffcb9a0910c561 x1 : 0000000000000000 x0 : ffff06378123a480 [ 4.260724] Call trace: [ 4.263172] __device_attach+0x3c/0x1bc [ 4.267020] device_attach+0x14/0x20 [ 4.270606] xpcs_create+0x24/0x384 [ 4.274107] xpcs_create_byaddr+0x74/0xa0 [ 4.278129] sja1105_mdiobus_register+0xf8/0x478 [ 4.282763] sja1105_setup+0xb4/0x1194 [ 4.286524] dsa_register_switch+0xab0/0x11f8 [ 4.290895] sja1105_probe+0x2bc/0x2e4 [ 4.294654] spi_probe+0xa4/0xc4 [ 4.297890] really_probe+0x16c/0x3fc [ 4.301564] __driver_probe_device+0xa4/0x168 [ 4.305935] driver_probe_device+0x3c/0x220 [ 4.310131] __device_attach_driver+0x128/0x1cc [ 4.314676] bus_for_each_drv+0xf4/0x14c [ 4.318610] __device_attach+0xfc/0x1bc [ 4.322457] device_initial_probe+0x14/0x20 [ 4.326654] bus_probe_device+0x94/0x100 [ 4.330587] deferred_probe_work_func+0xa0/0xfc [ 4.335132] process_scheduled_works+0x210/0x318 [ 4.339764] worker_thread+0x28c/0x450 [ 4.343523] kthread+0xfc/0x184 [ 4.346669] ret_from_fork+0x10/0x20 [ 4.350256] Code: 2a0103f6 f81f83a8 9431ccd8 f9402688 (39434109) [ 4.356366] ---[ end trace 0000000000000000 ]--- I haven't looked at the code at all, but disassembling drivers/base/dd.lst, I think the NPD is at dev->p->dead (0xa68 + 0x3c = 0xaa4). 0000000000000a68 <__device_attach>: ; { a68: d503233f paciasp a6c: d10143ff sub sp, sp, #0x50 a70: a9027bfd stp x29, x30, [sp, #0x20] a74: a90357f6 stp x22, x21, [sp, #0x30] a78: a9044ff4 stp x20, x19, [sp, #0x40] a7c: 910083fd add x29, sp, #0x20 a80: d5384108 mrs x8, SP_EL0 ; mutex_lock(&dev->mutex); a84: 91020013 add x19, x0, #0x80 a88: f9423508 ldr x8, [x8, #0x468] a8c: aa0003f4 mov x20, x0 a90: aa1303e0 mov x0, x19 a94: 2a0103f6 mov w22, w1 a98: f81f83a8 stur x8, [x29, #-0x8] a9c: 94000000 bl 0xa9c <__device_attach+0x34> 0000000000000a9c: R_AARCH64_CALL26 mutex_lock ; if (dev->p->dead) { aa0: f9402688 ldr x8, [x20, #0x48] aa4: 39434109 ldrb w9, [x8, #0xd0] aa8: 37000129 tbnz w9, #0x0, 0xacc <__device_attach+0x64> ; } else if (dev->driver) { aac: f9403689 ldr x9, [x20, #0x68] ab0: b40003e9 cbz x9, 0xb2c <__device_attach+0xc4> ; return dev->p && klist_node_attached(&dev->p->knode_driver); ab4: b40002a8 cbz x8, 0xb08 <__device_attach+0xa0> ab8: 91012100 add x0, x8, #0x48 abc: 94000000 bl 0xabc <__device_attach+0x54> 0000000000000abc: R_AARCH64_CALL26 klist_node_attached ; if (device_is_bound(dev)) { ac0: 34000240 cbz w0, 0xb08 <__device_attach+0xa0> ac4: 52800035 mov w21, #0x1 ac8: 14000002 b 0xad0 <__device_attach+0x68> acc: 2a1f03f5 mov w21, wzr ; mutex_unlock(&dev->mutex); ad0: aa1303e0 mov x0, x19 ad4: 94000000 bl 0xad4 <__device_attach+0x6c> 0000000000000ad4: R_AARCH64_CALL26 mutex_unlock ad8: d5384108 mrs x8, SP_EL0 adc: f9423508 ldr x8, [x8, #0x468] ae0: f85f83a9 ldur x9, [x29, #-0x8] ae4: eb09011f cmp x8, x9 ae8: 540008c1 b.ne 0xc00 <__device_attach+0x198> ; return ret; aec: 2a1503e0 mov w0, w21 af0: a9444ff4 ldp x20, x19, [sp, #0x40] af4: a94357f6 ldp x22, x21, [sp, #0x30] af8: a9427bfd ldp x29, x30, [sp, #0x20] afc: 910143ff add sp, sp, #0x50 b00: d50323bf autiasp b04: d65f03c0 ret ; ret = driver_sysfs_add(dev); b08: aa1403e0 mov x0, x20 b0c: 97ffff21 bl 0x790 <driver_sysfs_add> ; if (!ret) { b10: 340006c0 cbz w0, 0xbe8 <__device_attach+0x180> ; bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND); b14: aa1403e0 mov x0, x20 b18: 528000e1 mov w1, #0x7 b1c: 94000000 bl 0xb1c <__device_attach+0xb4> 0000000000000b1c: R_AARCH64_CALL26 bus_notify b20: 2a1f03f5 mov w21, wzr ; dev->driver = NULL; b24: f900369f str xzr, [x20, #0x68] b28: 17ffffea b 0xad0 <__device_attach+0x68> b2c: 120002c8 and w8, w22, #0x1 ; if (dev->parent) b30: f9402280 ldr x0, [x20, #0x40] ; struct device_attach_data data = { b34: a900fff4 stp x20, xzr, [sp, #0x8] b38: 39004bff strb wzr, [sp, #0x12] b3c: 390043e8 strb w8, [sp, #0x10] ; if (dev->parent) b40: b4000060 cbz x0, 0xb4c <__device_attach+0xe4> ; return __pm_runtime_resume(dev, RPM_GET_PUT); b44: 52800081 mov w1, #0x4 b48: 94000000 bl 0xb48 <__device_attach+0xe0> 0000000000000b48: R_AARCH64_CALL26 __pm_runtime_resume ; ret = bus_for_each_drv(dev->bus, NULL, &data, b4c: f9403280 ldr x0, [x20, #0x60] b50: 90000003 adrp x3, 0x0 <driver_deferred_probe_add> 0000000000000b50: R_AARCH64_ADR_PREL_PG_HI21 .text+0x17ac b54: 91000063 add x3, x3, #0x0 0000000000000b54: R_AARCH64_ADD_ABS_LO12_NC .text+0x17ac b58: 910023e2 add x2, sp, #0x8 b5c: aa1f03e1 mov x1, xzr b60: 94000000 bl 0xb60 <__device_attach+0xf8> 0000000000000b60: R_AARCH64_CALL26 bus_for_each_drv b64: 39404be8 ldrb w8, [sp, #0x12] ; if (!ret && allow_async && data.have_async) { b68: 7100001f cmp w0, #0x0 b6c: 1a9f07e9 cset w9, ne ; ret = bus_for_each_drv(dev->bus, NULL, &data, b70: 2a0003f5 mov w21, w0 b74: 7100011f cmp w8, #0x0 ; if (!ret && allow_async && data.have_async) { b78: 520002c8 eor w8, w22, #0x1 b7c: 1a9f17ea cset w10, eq b80: 2a0a0108 orr w8, w8, w10 b84: 2a080136 orr w22, w9, w8 b88: 360000f6 tbz w22, #0x0, 0xba4 <__device_attach+0x13c> ; return __pm_runtime_idle(dev, RPM_ASYNC); b8c: aa1403e0 mov x0, x20 b90: 52800021 mov w1, #0x1 b94: 94000000 bl 0xb94 <__device_attach+0x12c> 0000000000000b94: R_AARCH64_CALL26 __pm_runtime_idle ; if (dev->parent) b98: f9402280 ldr x0, [x20, #0x40] b9c: b50000e0 cbnz x0, 0xbb8 <__device_attach+0x150> ba0: 14000008 b 0xbc0 <__device_attach+0x158> ; asm_volatile_goto( ba4: d503201f nop
Hi Vladimir On Tue, Dec 05, 2023 at 01:13:51PM +0200, Vladimir Oltean wrote: > On Tue, Dec 05, 2023 at 01:35:31PM +0300, Serge Semin wrote: > > @@ -1436,21 +1480,32 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, > > struct dw_xpcs *xpcs; > > int ret; > > > > + ret = device_attach(&mdiodev->dev); > > + if (ret < 0 && ret != -ENODEV) > > + return ERR_PTR(ret); > > + > > xpcs = xpcs_create_data(mdiodev); > > if (IS_ERR(xpcs)) > > return xpcs; > > > > + ret = xpcs_init_clks(xpcs); > > + if (ret) > > + goto out_free_data; > > + > > ret = xpcs_init_id(xpcs); > > if (ret) > > - goto out; > > + goto out_clear_clks; > > > > ret = xpcs_init_iface(xpcs, interface); > > if (ret) > > - goto out; > > + goto out_clear_clks; > > > > return xpcs; > > [ 4.083518] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000d0 > [ 4.092356] Mem abort info: > [ 4.095164] ESR = 0x0000000096000004 > [ 4.098932] EC = 0x25: DABT (current EL), IL = 32 bits > [ 4.104277] SET = 0, FnV = 0 > [ 4.107352] EA = 0, S1PTW = 0 > [ 4.110505] FSC = 0x04: level 0 translation fault > [ 4.115408] Data abort info: > [ 4.118296] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 > [ 4.123807] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > [ 4.128877] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > [ 4.134214] [00000000000000d0] user address but active_mm is swapper > [ 4.140595] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > [ 4.146882] Modules linked in: > [ 4.149944] CPU: 0 PID: 11 Comm: kworker/u4:0 Not tainted 6.7.0-rc3-00719-g75be5ea8e111-dirty #1551 > [ 4.164524] Workqueue: events_unbound deferred_probe_work_func > [ 4.177372] pc : __device_attach+0x3c/0x1bc > [ 4.181570] lr : __device_attach+0x38/0x1bc > [ 4.185767] sp : ffff8000800f3800 > [ 4.189087] x29: ffff8000800f3820 x28: 0000000000000001 x27: ffff063781bda150 > [ 4.196252] x26: ffff063781bda150 x25: ffff063780827480 x24: ffffcb9a08138a40 > [ 4.203416] x23: ffff063781114080 x22: 0000000000000000 x21: 0000000000000004 > [ 4.210579] x20: ffff06378123a400 x19: ffff06378123a480 x18: ffffcb9a07c703a0 > [ 4.217743] x17: ffffcb9a07c703a4 x16: 00000000000000d4 x15: ffffcb9a07be70fc > [ 4.224906] x14: ffffcb9a08299638 x13: 0000000000000053 x12: ffff003000000200 > [ 4.232069] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 > [ 4.239233] x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003a > [ 4.246396] x5 : ffff0637809a037b x4 : ffffcb9a087b0d47 x3 : ffff10300000020f > [ 4.253560] x2 : ffffcb9a0910c561 x1 : 0000000000000000 x0 : ffff06378123a480 > [ 4.260724] Call trace: > [ 4.263172] __device_attach+0x3c/0x1bc > [ 4.267020] device_attach+0x14/0x20 > [ 4.270606] xpcs_create+0x24/0x384 > [ 4.274107] xpcs_create_byaddr+0x74/0xa0 > [ 4.278129] sja1105_mdiobus_register+0xf8/0x478 > [ 4.282763] sja1105_setup+0xb4/0x1194 > [ 4.286524] dsa_register_switch+0xab0/0x11f8 > [ 4.290895] sja1105_probe+0x2bc/0x2e4 > [ 4.294654] spi_probe+0xa4/0xc4 > [ 4.297890] really_probe+0x16c/0x3fc > [ 4.301564] __driver_probe_device+0xa4/0x168 > [ 4.305935] driver_probe_device+0x3c/0x220 > [ 4.310131] __device_attach_driver+0x128/0x1cc > [ 4.314676] bus_for_each_drv+0xf4/0x14c > [ 4.318610] __device_attach+0xfc/0x1bc > [ 4.322457] device_initial_probe+0x14/0x20 > [ 4.326654] bus_probe_device+0x94/0x100 > [ 4.330587] deferred_probe_work_func+0xa0/0xfc > [ 4.335132] process_scheduled_works+0x210/0x318 > [ 4.339764] worker_thread+0x28c/0x450 > [ 4.343523] kthread+0xfc/0x184 > [ 4.346669] ret_from_fork+0x10/0x20 > [ 4.350256] Code: 2a0103f6 f81f83a8 9431ccd8 f9402688 (39434109) > [ 4.356366] ---[ end trace 0000000000000000 ]--- > > I haven't looked at the code at all, but disassembling drivers/base/dd.lst, > I think the NPD is at dev->p->dead (0xa68 + 0x3c = 0xaa4). > > 0000000000000a68 <__device_attach>: > ; { > a68: d503233f paciasp > a6c: d10143ff sub sp, sp, #0x50 > a70: a9027bfd stp x29, x30, [sp, #0x20] > a74: a90357f6 stp x22, x21, [sp, #0x30] > a78: a9044ff4 stp x20, x19, [sp, #0x40] > a7c: 910083fd add x29, sp, #0x20 > a80: d5384108 mrs x8, SP_EL0 > ; mutex_lock(&dev->mutex); > a84: 91020013 add x19, x0, #0x80 > a88: f9423508 ldr x8, [x8, #0x468] > a8c: aa0003f4 mov x20, x0 > a90: aa1303e0 mov x0, x19 > a94: 2a0103f6 mov w22, w1 > a98: f81f83a8 stur x8, [x29, #-0x8] > a9c: 94000000 bl 0xa9c <__device_attach+0x34> > 0000000000000a9c: R_AARCH64_CALL26 mutex_lock > ; if (dev->p->dead) { > aa0: f9402688 ldr x8, [x20, #0x48] > aa4: 39434109 ldrb w9, [x8, #0xd0] > aa8: 37000129 tbnz w9, #0x0, 0xacc <__device_attach+0x64> > ; } else if (dev->driver) { > aac: f9403689 ldr x9, [x20, #0x68] > ab0: b40003e9 cbz x9, 0xb2c <__device_attach+0xc4> > ; return dev->p && klist_node_attached(&dev->p->knode_driver); > ab4: b40002a8 cbz x8, 0xb08 <__device_attach+0xa0> > ab8: 91012100 add x0, x8, #0x48 > abc: 94000000 bl 0xabc <__device_attach+0x54> > 0000000000000abc: R_AARCH64_CALL26 klist_node_attached > ; if (device_is_bound(dev)) { > ac0: 34000240 cbz w0, 0xb08 <__device_attach+0xa0> > ac4: 52800035 mov w21, #0x1 > ac8: 14000002 b 0xad0 <__device_attach+0x68> > acc: 2a1f03f5 mov w21, wzr > ; mutex_unlock(&dev->mutex); > ad0: aa1303e0 mov x0, x19 > ad4: 94000000 bl 0xad4 <__device_attach+0x6c> > 0000000000000ad4: R_AARCH64_CALL26 mutex_unlock > ad8: d5384108 mrs x8, SP_EL0 > adc: f9423508 ldr x8, [x8, #0x468] > ae0: f85f83a9 ldur x9, [x29, #-0x8] > ae4: eb09011f cmp x8, x9 > ae8: 540008c1 b.ne 0xc00 <__device_attach+0x198> > ; return ret; > aec: 2a1503e0 mov w0, w21 > af0: a9444ff4 ldp x20, x19, [sp, #0x40] > af4: a94357f6 ldp x22, x21, [sp, #0x30] > af8: a9427bfd ldp x29, x30, [sp, #0x20] > afc: 910143ff add sp, sp, #0x50 > b00: d50323bf autiasp > b04: d65f03c0 ret > ; ret = driver_sysfs_add(dev); > b08: aa1403e0 mov x0, x20 > b0c: 97ffff21 bl 0x790 <driver_sysfs_add> > ; if (!ret) { > b10: 340006c0 cbz w0, 0xbe8 <__device_attach+0x180> > ; bus_notify(dev, BUS_NOTIFY_DRIVER_NOT_BOUND); > b14: aa1403e0 mov x0, x20 > b18: 528000e1 mov w1, #0x7 > b1c: 94000000 bl 0xb1c <__device_attach+0xb4> > 0000000000000b1c: R_AARCH64_CALL26 bus_notify > b20: 2a1f03f5 mov w21, wzr > ; dev->driver = NULL; > b24: f900369f str xzr, [x20, #0x68] > b28: 17ffffea b 0xad0 <__device_attach+0x68> > b2c: 120002c8 and w8, w22, #0x1 > ; if (dev->parent) > b30: f9402280 ldr x0, [x20, #0x40] > ; struct device_attach_data data = { > b34: a900fff4 stp x20, xzr, [sp, #0x8] > b38: 39004bff strb wzr, [sp, #0x12] > b3c: 390043e8 strb w8, [sp, #0x10] > ; if (dev->parent) > b40: b4000060 cbz x0, 0xb4c <__device_attach+0xe4> > ; return __pm_runtime_resume(dev, RPM_GET_PUT); > b44: 52800081 mov w1, #0x4 > b48: 94000000 bl 0xb48 <__device_attach+0xe0> > 0000000000000b48: R_AARCH64_CALL26 __pm_runtime_resume > ; ret = bus_for_each_drv(dev->bus, NULL, &data, > b4c: f9403280 ldr x0, [x20, #0x60] > b50: 90000003 adrp x3, 0x0 <driver_deferred_probe_add> > 0000000000000b50: R_AARCH64_ADR_PREL_PG_HI21 .text+0x17ac > b54: 91000063 add x3, x3, #0x0 > 0000000000000b54: R_AARCH64_ADD_ABS_LO12_NC .text+0x17ac > b58: 910023e2 add x2, sp, #0x8 > b5c: aa1f03e1 mov x1, xzr > b60: 94000000 bl 0xb60 <__device_attach+0xf8> > 0000000000000b60: R_AARCH64_CALL26 bus_for_each_drv > b64: 39404be8 ldrb w8, [sp, #0x12] > ; if (!ret && allow_async && data.have_async) { > b68: 7100001f cmp w0, #0x0 > b6c: 1a9f07e9 cset w9, ne > ; ret = bus_for_each_drv(dev->bus, NULL, &data, > b70: 2a0003f5 mov w21, w0 > b74: 7100011f cmp w8, #0x0 > ; if (!ret && allow_async && data.have_async) { > b78: 520002c8 eor w8, w22, #0x1 > b7c: 1a9f17ea cset w10, eq > b80: 2a0a0108 orr w8, w8, w10 > b84: 2a080136 orr w22, w9, w8 > b88: 360000f6 tbz w22, #0x0, 0xba4 <__device_attach+0x13c> > ; return __pm_runtime_idle(dev, RPM_ASYNC); > b8c: aa1403e0 mov x0, x20 > b90: 52800021 mov w1, #0x1 > b94: 94000000 bl 0xb94 <__device_attach+0x12c> > 0000000000000b94: R_AARCH64_CALL26 __pm_runtime_idle > ; if (dev->parent) > b98: f9402280 ldr x0, [x20, #0x40] > b9c: b50000e0 cbnz x0, 0xbb8 <__device_attach+0x150> > ba0: 14000008 b 0xbc0 <__device_attach+0x158> > ; asm_volatile_goto( > ba4: d503201f nop Omg, thank you very much for testing the series straight away and sorry for the immediate trouble it caused. I'll need some more time for investigation. I'll get back to this topic a bit later on this week. -Serge(y)
On Tue, Dec 05, 2023 at 02:35:46PM +0300, Serge Semin wrote: > Omg, thank you very much for testing the series straight away and > sorry for the immediate trouble it caused. I'll need some more time > for investigation. I'll get back to this topic a bit later on this > week. Don't worry, I got suspicious when I was CCed to review only a one-line change in patch 11/16. It's never about that one line, is it?) Anyway, the NULL dev->p is a symptom of device_add() not having been called, most likely from mdio_device_register(). I'll be honest and say that I still don't quite understand what you're trying to achieve. You're trying to bind the hardcoded mdio_devices created by xpcs_create() to a driver? That was attempted a while ago by Sean Anderson with the Lynx PCS. Are you aware of the fact that even in the good case in which binding the driver actually works, the user can then come along and unbind it from the PCS device, and phylink isn't prepared to handle that, so it will crash the kernel upon the next phylink_pcs call? The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least tear down the whole thing when the PCS is unbound, which is saner than crashing the kernel. I don't see the equivalent protection mechanism here? Can't the xpcs continue to live without a bound driver? Having a compatible string in the OF description is perfectly fine though, and should absolutely not preclude that.
Hi Vladimir On Tue, Dec 05, 2023 at 02:23:16PM +0200, Vladimir Oltean wrote: > On Tue, Dec 05, 2023 at 02:35:46PM +0300, Serge Semin wrote: > > Omg, thank you very much for testing the series straight away and > > sorry for the immediate trouble it caused. I'll need some more time > > for investigation. I'll get back to this topic a bit later on this > > week. > > Don't worry, I got suspicious when I was CCed to review only a one-line > change in patch 11/16. It's never about that one line, is it?) Right. I should have added you to the list of recipients of the entire series since the patchset changes more than that. The bug you caught brightly highlights my mistake. I'll make sure you are in the list. I'll add Jiawen and Mengyuan there too since the driver under their maintenance is also affected. Hopefully they'll get to test the series too. > > Anyway, the NULL dev->p is a symptom of device_add() not having been > called, most likely from mdio_device_register(). Absolutely right. I thought that mdio_device_create() having the device_initialize() method called was enough for the device_attach() function being happy. It turns out it wasn't and I missed that the device_private instance is allocated only on the device registration. So I'll need to call mdio_device_register() after all, if I get to preserve the current design of the solution. > > I'll be honest and say that I still don't quite understand what you're > trying to achieve. You're trying to bind the hardcoded mdio_devices > created by xpcs_create() to a driver? My idea was to reuse the mdio_device which has already been created either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus board_info infrastructure (can be utilized in the SJA1105 or Wangxun Tx GBE). The xpcs_create() method then either probes the device on the MDIO bus and gets ID from there, or just uses the custom IDs based on the OF compatible match table or on the platform_data. If no MDIO-device was created my patchset is supposed to preserve the previous semantics: create MDIO-device, probe the device on the MDIO-bus, get device IDs from there. See the next patch for more details: https://lore.kernel.org/netdev/20231205103559.9605-11-fancer.lancer@gmail.com/ > That was attempted a while ago by > Sean Anderson with the Lynx PCS. Are you aware of the fact that even in > the good case in which binding the driver actually works, the user can > then come along and unbind it from the PCS device, and phylink isn't > prepared to handle that, so it will crash the kernel upon the next > phylink_pcs call? To be honest I didn't consider the driver bind/unbind option. But my case a bit different. DW XPCS MDIO-device is supposed to be created automatically by means of the DW XPCS MI driver from the DT-nodes hierarchy like this: mdio@1f05d000 { compatible = "snps,dw-xpcs-mi"; reg = <0 0x1f05d000 0 0x1000>; xgmac_pcs: ethernet-pcs@0 { compatible = "snps,dw-xpcs"; reg = <0>; }; }; The platform-device is created for the mdio@1f05d000 node for which the DW XPCS MI driver is loaded, which calls the devm_of_mdiobus_register() in the probe() method which registers the MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0 node. The DW XPCS MDIO-device driver is attached to that MDIO-device then. In such model the PCS can be supplied to the DW *MAC via the "pcs-handle = &xgmac_pcs" property. Regarding the current semantics it's preserved in the framework of the xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of the next code snippet: if (mdiobus_is_registered_device(bus, addr)) { mdiodev = bus->mdio_map[addr]; mdio_device_get(mdiodev); } else { mdiodev = mdio_device_create(bus, addr); if (IS_ERR(mdiodev)) return ERR_CAST(mdiodev); } Device can be automatically created if before registering the MDIO-bus the xpcs_create_byaddr() caller registered the MDIO-device board info by means of the mdiobus_register_board_info() method. In addition to that it's now possible to supply some custom data (custom device IDs in my implementation) to the XPCS driver by means of the mdio_board_info.platform_data field. See the next patch for reference: https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@gmail.com So what the difference with the Lynx PCS is that in my case the MDIO-device is created automatically as a result of the DW XPCS MI MDIO-bus registration. Additionally I implemented the MDIO-device creation based on the MDIO-board-info, thus there won't be need in the calling mdio_device_create() on each xpcs_create_mdiodev() invocation. The later part isn't that important in the framework of this conversation, but just so you be aware. Regarding the driver bind/unbind. As I said I didn't actually consider that option. On the other hand my DW XPCS MDIO-device driver doesn't do actual probe() or remove(). The only implemented thing is the of_device_id table, which is used to assign PCS and PMA IDs if required based on the DT compatible property. So I can easily drop any MDIO device-driver part and parse the of_device_id table right in the xpcs_create_bynode(). From that perspective my implementation won't differ much from the Lynx PCS design. The only difference will be is the way the MDIO-bus is created and registered. In case of Lynx PCS the bus is created by the MAC-driver itself. In my case DW XPCS MI is currently created in the framework of the separate platform driver. Do you think it would be better to follow the Lynx design pattern in order to get rid from the possibility of the DW XPCS MI driver being unbound behind the STMMAC+XPCS couple back? In this case the Dw MAC DT-node hierarchy would look like this: xgmac: ethernet@1f054000 { compatible = "snps,dwxgmac"; reg = <0 0x1f054000 0 0x4000>; reg-names = "stmmaceth"; ranges; ... pcs-handle = &xgmac_pcs; // DW XPCS MI to access the DW XPCS attached to the device mdio@1f05d000 { compatible = "snps,dwmac-mi"; reg = <0 0x1f05d000 0 0x1000>; xgmac_pcs: ethernet-pcs@0 { compatible = "snps,dw-xpcs"; reg = <0>; }; }; // Normal MDIO-bus to access external PHYs (it's also called // as SMA - Station Management Agent - by Synopsys) mdio { compatible = "snps,dwmac-mdio"; #address-cells = <1>; #size-cells = <0>; }; }; I actually thought to use that hardware description pattern instead, but after some meditation around that I decided that having the DW XPCS device defined separately from the DW MAC node seemed better at least from the code separation point of view. Now I think that it wasn't the best decision. DW XPCS is always attached to the DW XGMAC controller. So it would be more correct having it defined as a sub-node. It would also helped to avoid the platform device driver bind/unbind problem. What do you think? Should I re-design my patchset to be supporting the design above? (After having conversion with you I am more inclined to do that now than to stick with the currently implemented solution.) > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least > tear down the whole thing when the PCS is unbound, which is saner than > crashing the kernel. I don't see the equivalent protection mechanism here? You are right. I don't have any equivalent protection here. Thanks for suggesting a solution. > > Can't the xpcs continue to live without a bound driver? Having a > compatible string in the OF description is perfectly fine though, > and should absolutely not preclude that. As I explained above Dw XPCS device can live without a bound driver because the DW XPCS MDIO-driver doesn't do much but merely gets to be bound based on the of_device_id table. In my case the problem is in the DW XPCS MI driver which indeed can be detached. Please see my long-read text above. -Serge(y)
On Fri, Dec 08, 2023 at 05:11:20PM +0300, Serge Semin wrote: > My idea was to reuse the mdio_device which has already been created > either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus > board_info infrastructure (can be utilized in the SJA1105 or Wangxun > Tx GBE). The xpcs_create() method then either probes the device on the MDIO > bus and gets ID from there, or just uses the custom IDs based on the > OF compatible match table or on the platform_data. If no MDIO-device > was created my patchset is supposed to preserve the previous > semantics: create MDIO-device, probe the device on the MDIO-bus, get > device IDs from there. See the next patch for more details: > https://lore.kernel.org/netdev/20231205103559.9605-11-fancer.lancer@gmail.com/ > > > That was attempted a while ago by > > Sean Anderson with the Lynx PCS. Are you aware of the fact that even in > > the good case in which binding the driver actually works, the user can > > then come along and unbind it from the PCS device, and phylink isn't > > prepared to handle that, so it will crash the kernel upon the next > > phylink_pcs call? > > To be honest I didn't consider the driver bind/unbind option. But my > case a bit different. DW XPCS MDIO-device is supposed to be created > automatically by means of the DW XPCS MI driver from the DT-nodes > hierarchy like this: > mdio@1f05d000 { > compatible = "snps,dw-xpcs-mi"; > reg = <0 0x1f05d000 0 0x1000>; > > xgmac_pcs: ethernet-pcs@0 { > compatible = "snps,dw-xpcs"; > reg = <0>; > }; > }; > The platform-device is created for the mdio@1f05d000 node for which > the DW XPCS MI driver is loaded, which calls the > devm_of_mdiobus_register() in the probe() method which registers the > MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0 > node. The DW XPCS MDIO-device driver is attached to that MDIO-device > then. In such model the PCS can be supplied to the DW *MAC via the > "pcs-handle = &xgmac_pcs" property. > > Regarding the current semantics it's preserved in the framework of the > xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of > the next code snippet: > if (mdiobus_is_registered_device(bus, addr)) { > mdiodev = bus->mdio_map[addr]; > mdio_device_get(mdiodev); > } else { > mdiodev = mdio_device_create(bus, addr); > if (IS_ERR(mdiodev)) > return ERR_CAST(mdiodev); > } > Device can be automatically created if before registering the MDIO-bus > the xpcs_create_byaddr() caller registered the MDIO-device board info > by means of the mdiobus_register_board_info() method. In addition to > that it's now possible to supply some custom data (custom device IDs > in my implementation) to the XPCS driver by means of the > mdio_board_info.platform_data field. See the next patch for > reference: > https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@gmail.com > > So what the difference with the Lynx PCS is that in my case the > MDIO-device is created automatically as a result of the DW XPCS MI > MDIO-bus registration. Additionally I implemented the MDIO-device > creation based on the MDIO-board-info, thus there won't be need in the > calling mdio_device_create() on each xpcs_create_mdiodev() invocation. > The later part isn't that important in the framework of this > conversation, but just so you be aware. It's not really different, though. You can connect to the Lynx PCS both ways, see dpaa2_pcs_create() which also searches for a pcs-handle before calling lynx_pcs_create_fwnode(). What's subtly different is that we don't (yet) have "fsl,lynx-pcs" compatible strings in the device tree. So the MDIO controller will register the PCS devices as struct phy_device (which still have an underlying struct mdio_device). The PCS layer connects to the underlying struct mdio_device, and the phy_device on top remains unconnected to any phylib/phylink MAC driver. That is confusing, I should really get to adding those compatible strings to suppress the phy_device creation. > Regarding the driver bind/unbind. As I said I didn't actually consider > that option. On the other hand my DW XPCS MDIO-device driver doesn't > do actual probe() or remove(). The only implemented thing is the > of_device_id table, which is used to assign PCS and PMA IDs if > required based on the DT compatible property. So I can easily drop any > MDIO device-driver part and parse the of_device_id table right in the > xpcs_create_bynode(). From that perspective my implementation won't > differ much from the Lynx PCS design. The only difference will be is > the way the MDIO-bus is created and registered. In case of Lynx PCS > the bus is created by the MAC-driver itself. Nope, not true. Follow the pcs-handle in arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi. > In my case DW XPCS MI is currently created in the framework of the > separate platform driver. Do you think it would be better to follow > the Lynx design pattern in order to get rid from the possibility of > the DW XPCS MI driver being unbound behind the STMMAC+XPCS couple > back? I think you actually pointed out a flaw in the Lynx PCS design too. Actually, it is a larger flaw in the kernel. You can also unbind the MDIO bus which holds the phy_device, and phylib (and therefore also phylink) won't expect that either, so it will crash. > In this case the Dw MAC DT-node hierarchy would look like this: > > xgmac: ethernet@1f054000 { > compatible = "snps,dwxgmac"; > reg = <0 0x1f054000 0 0x4000>; > reg-names = "stmmaceth"; > ranges; > > ... > > pcs-handle = &xgmac_pcs; > > // DW XPCS MI to access the DW XPCS attached to the device > mdio@1f05d000 { > compatible = "snps,dwmac-mi"; > reg = <0 0x1f05d000 0 0x1000>; > > xgmac_pcs: ethernet-pcs@0 { > compatible = "snps,dw-xpcs"; > reg = <0>; > }; > }; > > // Normal MDIO-bus to access external PHYs (it's also called > // as SMA - Station Management Agent - by Synopsys) > mdio { > compatible = "snps,dwmac-mdio"; > #address-cells = <1>; > #size-cells = <0>; > }; > }; > > I actually thought to use that hardware description pattern instead, > but after some meditation around that I decided that having the DW > XPCS device defined separately from the DW MAC node seemed better at > least from the code separation point of view. Now I think that it > wasn't the best decision. DW XPCS is always attached to the DW XGMAC > controller. So it would be more correct having it defined as a > sub-node. It would also helped to avoid the platform device driver > bind/unbind problem. > > What do you think? Should I re-design my patchset to be supporting the > design above? (After having conversion with you I am more inclined to > do that now than to stick with the currently implemented solution.) I think that the placement of the "mdio" node as lateral vs subordinate to the "ethernet" node would have fixed the issue by mistake. We should be looking at it as a structural problem of the kernel instead. Don't let it influence what you believe should be the correct design. > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least > > tear down the whole thing when the PCS is unbound, which is saner than > > crashing the kernel. I don't see the equivalent protection mechanism here? > > You are right. I don't have any equivalent protection here. Thanks for > suggesting a solution. I think that a device link between the "ethernet" device and the "mdio" device (controller, parent of the PHY or PCS), if the Ethernet is not a parent of the MDIO controller, could also solve that. But it would also require ACK from PHY maintainers, who may have grander plans to address this snag. > > Can't the xpcs continue to live without a bound driver? Having a > > compatible string in the OF description is perfectly fine though, > > and should absolutely not preclude that. > > As I explained above Dw XPCS device can live without a bound driver > because the DW XPCS MDIO-driver doesn't do much but merely gets to be > bound based on the of_device_id table. In my case the problem is in > the DW XPCS MI driver which indeed can be detached. Please see my > long-read text above. Yeah, common design, common problem.
Hi Vladimir, Sorry for the delayed response. Too much work at this time of the year (ah, Decembers)..( On Fri, Dec 08, 2023 at 06:33:43PM +0200, Vladimir Oltean wrote: > On Fri, Dec 08, 2023 at 05:11:20PM +0300, Serge Semin wrote: > > My idea was to reuse the mdio_device which has already been created > > either by means of the MDIO-bus OF-subnode or by means of the MDIO-bus > > board_info infrastructure (can be utilized in the SJA1105 or Wangxun > > Tx GBE). The xpcs_create() method then either probes the device on the MDIO > > bus and gets ID from there, or just uses the custom IDs based on the > > OF compatible match table or on the platform_data. If no MDIO-device > > was created my patchset is supposed to preserve the previous > > semantics: create MDIO-device, probe the device on the MDIO-bus, get > > device IDs from there. See the next patch for more details: > > https://lore.kernel.org/netdev/20231205103559.9605-11-fancer.lancer@gmail.com/ > > > > > That was attempted a while ago by > > > Sean Anderson with the Lynx PCS. Are you aware of the fact that even in > > > the good case in which binding the driver actually works, the user can > > > then come along and unbind it from the PCS device, and phylink isn't > > > prepared to handle that, so it will crash the kernel upon the next > > > phylink_pcs call? > > > > To be honest I didn't consider the driver bind/unbind option. But my > > case a bit different. DW XPCS MDIO-device is supposed to be created > > automatically by means of the DW XPCS MI driver from the DT-nodes > > hierarchy like this: > > mdio@1f05d000 { > > compatible = "snps,dw-xpcs-mi"; > > reg = <0 0x1f05d000 0 0x1000>; > > > > xgmac_pcs: ethernet-pcs@0 { > > compatible = "snps,dw-xpcs"; > > reg = <0>; > > }; > > }; > > The platform-device is created for the mdio@1f05d000 node for which > > the DW XPCS MI driver is loaded, which calls the > > devm_of_mdiobus_register() in the probe() method which registers the > > MDIO-bus and then creates the MDIO-device from the ethernet-pcs@0 > > node. The DW XPCS MDIO-device driver is attached to that MDIO-device > > then. In such model the PCS can be supplied to the DW *MAC via the > > "pcs-handle = &xgmac_pcs" property. > > > > Regarding the current semantics it's preserved in the framework of the > > xpcs_create_byaddr() method (former xpcs_create_mdiodev()) by means of > > the next code snippet: > > if (mdiobus_is_registered_device(bus, addr)) { > > mdiodev = bus->mdio_map[addr]; > > mdio_device_get(mdiodev); > > } else { > > mdiodev = mdio_device_create(bus, addr); > > if (IS_ERR(mdiodev)) > > return ERR_CAST(mdiodev); > > } > > Device can be automatically created if before registering the MDIO-bus > > the xpcs_create_byaddr() caller registered the MDIO-device board info > > by means of the mdiobus_register_board_info() method. In addition to > > that it's now possible to supply some custom data (custom device IDs > > in my implementation) to the XPCS driver by means of the > > mdio_board_info.platform_data field. See the next patch for > > reference: > > https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@gmail.com > > > > So what the difference with the Lynx PCS is that in my case the > > MDIO-device is created automatically as a result of the DW XPCS MI > > MDIO-bus registration. Additionally I implemented the MDIO-device > > creation based on the MDIO-board-info, thus there won't be need in the > > calling mdio_device_create() on each xpcs_create_mdiodev() invocation. > > The later part isn't that important in the framework of this > > conversation, but just so you be aware. > > It's not really different, though. You can connect to the Lynx PCS both > ways, see dpaa2_pcs_create() which also searches for a pcs-handle before > calling lynx_pcs_create_fwnode(). Ah, right. Lynx PCS also implements the fwnode-based PCS descriptor creation. > What's subtly different is that we > don't (yet) have "fsl,lynx-pcs" compatible strings in the device tree. > So the MDIO controller will register the PCS devices as struct phy_device > (which still have an underlying struct mdio_device). The PCS layer > connects to the underlying struct mdio_device, and the phy_device on top > remains unconnected to any phylib/phylink MAC driver. That is confusing, > I should really get to adding those compatible strings to suppress the > phy_device creation. It hasn't been confirmed yet here https://lore.kernel.org/netdev/na6krkoco7pmsl62dfuj2xlrvpsnod74ptpfyy6gv7dzwmowga@mzsiknjian2i/ but AFAICS it is wrong to have a PCS device registered as PHY by any means: unmasking address in mii_bus->phy_mask or having the of_mdiobus_child_is_phy() returned true for a DT-node. So right, a specific compatible should be added to the PCS DT-nodes. > > > Regarding the driver bind/unbind. As I said I didn't actually consider > > that option. On the other hand my DW XPCS MDIO-device driver doesn't > > do actual probe() or remove(). The only implemented thing is the > > of_device_id table, which is used to assign PCS and PMA IDs if > > required based on the DT compatible property. So I can easily drop any > > MDIO device-driver part and parse the of_device_id table right in the > > xpcs_create_bynode(). From that perspective my implementation won't > > differ much from the Lynx PCS design. The only difference will be is > > the way the MDIO-bus is created and registered. In case of Lynx PCS > > the bus is created by the MAC-driver itself. > > Nope, not true. Follow the pcs-handle in arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi. Ah, right, I missed that case. I was referring to the cases when the Lynx PCS MDIO node is defined as a sub-node of the MAC node. > > > In my case DW XPCS MI is currently created in the framework of the > > separate platform driver. Do you think it would be better to follow > > the Lynx design pattern in order to get rid from the possibility of > > the DW XPCS MI driver being unbound behind the STMMAC+XPCS couple > > back? > > I think you actually pointed out a flaw in the Lynx PCS design too. > Actually, it is a larger flaw in the kernel. You can also unbind the > MDIO bus which holds the phy_device, and phylib (and therefore also > phylink) won't expect that either, so it will crash. > > > In this case the Dw MAC DT-node hierarchy would look like this: > > > > xgmac: ethernet@1f054000 { > > compatible = "snps,dwxgmac"; > > reg = <0 0x1f054000 0 0x4000>; > > reg-names = "stmmaceth"; > > ranges; > > > > ... > > > > pcs-handle = &xgmac_pcs; > > > > // DW XPCS MI to access the DW XPCS attached to the device > > mdio@1f05d000 { > > compatible = "snps,dwmac-mi"; > > reg = <0 0x1f05d000 0 0x1000>; > > > > xgmac_pcs: ethernet-pcs@0 { > > compatible = "snps,dw-xpcs"; > > reg = <0>; > > }; > > }; > > > > // Normal MDIO-bus to access external PHYs (it's also called > > // as SMA - Station Management Agent - by Synopsys) > > mdio { > > compatible = "snps,dwmac-mdio"; > > #address-cells = <1>; > > #size-cells = <0>; > > }; > > }; > > > > I actually thought to use that hardware description pattern instead, > > but after some meditation around that I decided that having the DW > > XPCS device defined separately from the DW MAC node seemed better at > > least from the code separation point of view. Now I think that it > > wasn't the best decision. DW XPCS is always attached to the DW XGMAC > > controller. So it would be more correct having it defined as a > > sub-node. It would also helped to avoid the platform device driver > > bind/unbind problem. > > > > What do you think? Should I re-design my patchset to be supporting the > > design above? (After having conversion with you I am more inclined to > > do that now than to stick with the currently implemented solution.) > > I think that the placement of the "mdio" node as lateral vs subordinate > to the "ethernet" node would have fixed the issue by mistake. We should > be looking at it as a structural problem of the kernel instead. Don't > let it influence what you believe should be the correct design. Ok. Thanks for clarification. I won't move the PCS device DT-node to being subordinate of the MAC DT-node then. Although after some more digging into the problem I figured out that the solution still needs to be re-designed a bit. Currently I have the DW XPCS device represented as the nodes hierarchy: mdio@1f05d000 { compatible = "snps,dwmac-mi"; reg = <0 0x1f05d000 0 0x1000>; xgmac_pcs: ethernet-pcs@0 { compatible = "snps,dw-xpcs"; reg = <0>; }; }; When I introduced such representation I assumed that it was possible to have more than one DW XPCS devices accessible over the same MCI/APB interface with for instance some static offset. But it turned out I was wrong again. DW XPCS HW databook explicitly states that port_id_i[4:0] input signal is specific to the MDIO interface only. That signal is the MDIO-bus address of the device and it doesn't exist for the DW XPCS devices mapped to the system IO-memory via the MCI/APB buses. So there can't be more than one XPCS device accessible over the same MCI/APB port and the correct way to represent the DW XPCS device is just: xgmac_pcs: ethernet-pcs@0 { compatible = "snps,dw-xpcs"; reg = <0 0x1f05d000 0 0x1000>; }; with no superior MI node. I'll re-design my patchset to support the device representation above then: just create a hidden MDIO-bus in the DW XPCS driver probe method and directly register the XPCS-device on it. The patch for the MDIO-bus subsystem will be gone after that. > > > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least > > > tear down the whole thing when the PCS is unbound, which is saner than > > > crashing the kernel. I don't see the equivalent protection mechanism here? > > > > You are right. I don't have any equivalent protection here. Thanks for > > suggesting a solution. > > I think that a device link between the "ethernet" device and the "mdio" > device (controller, parent of the PHY or PCS), if the Ethernet is not a > parent of the MDIO controller, could also solve that. But it would also > require ACK from PHY maintainers, who may have grander plans to address > this snag. Ok. I'll add it in v2. Let's see what the maintainers think about that. Thanks for all your comments in my patchset regard. They are really helpful. -Serge(y) > > > > Can't the xpcs continue to live without a bound driver? Having a > > > compatible string in the OF description is perfectly fine though, > > > and should absolutely not preclude that. > > > > As I explained above Dw XPCS device can live without a bound driver > > because the DW XPCS MDIO-driver doesn't do much but merely gets to be > > bound based on the of_device_id table. In my case the problem is in > > the DW XPCS MI driver which indeed can be detached. Please see my > > long-read text above. > > Yeah, common design, common problem.
On Thu, Dec 14, 2023 at 02:54:00PM +0300, Serge Semin wrote: > > > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least > > > > tear down the whole thing when the PCS is unbound, which is saner than > > > > crashing the kernel. I don't see the equivalent protection mechanism here? > > > > > > You are right. I don't have any equivalent protection here. Thanks for > > > suggesting a solution. > > > > I think that a device link between the "ethernet" device and the "mdio" > > device (controller, parent of the PHY or PCS), if the Ethernet is not a > > parent of the MDIO controller, could also solve that. But it would also > > require ACK from PHY maintainers, who may have grander plans to address > > this snag. > > Ok. I'll add it in v2. Let's see what the maintainers think about > that. Are you not following the parallel discussion on the topic of PCS devices having bound drivers? https://lore.kernel.org/netdev/ZXnV%2FPk1PYxAm%2FjS@shell.armlinux.org.uk/ Sadly I don't have much spare time to join that discussion, but it looks like you could.
On Thu, Dec 14, 2023 at 02:00:16PM +0200, Vladimir Oltean wrote: > On Thu, Dec 14, 2023 at 02:54:00PM +0300, Serge Semin wrote: > > > > > The pcs-rzn1-miic.c driver puts a device_link to the MAC to at least > > > > > tear down the whole thing when the PCS is unbound, which is saner than > > > > > crashing the kernel. I don't see the equivalent protection mechanism here? > > > > > > > > You are right. I don't have any equivalent protection here. Thanks for > > > > suggesting a solution. > > > > > > I think that a device link between the "ethernet" device and the "mdio" > > > device (controller, parent of the PHY or PCS), if the Ethernet is not a > > > parent of the MDIO controller, could also solve that. But it would also > > > require ACK from PHY maintainers, who may have grander plans to address > > > this snag. > > > > Ok. I'll add it in v2. Let's see what the maintainers think about > > that. > > Are you not following the parallel discussion on the topic of PCS > devices having bound drivers? > https://lore.kernel.org/netdev/ZXnV%2FPk1PYxAm%2FjS@shell.armlinux.org.uk/ > > Sadly I don't have much spare time to join that discussion, but it looks > like you could. Ok. Thanks for sharing the link. At least I'll follow up the discussion in order to pick up/wait for a solution they'll come up with. -Serge(y)
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig index 87cf308fc6d8..f6aa437473de 100644 --- a/drivers/net/pcs/Kconfig +++ b/drivers/net/pcs/Kconfig @@ -6,11 +6,11 @@ menu "PCS device drivers" config PCS_XPCS - tristate + tristate "Synopsys DesignWare Ethernet XPCS" select PHYLINK help - This module provides helper functions for Synopsys DesignWare XPCS - controllers. + This module provides a driver and helper functions for Synopsys + DesignWare XPCS controllers. config PCS_LYNX tristate diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index ea6f56339595..183a37929b60 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -6,10 +6,14 @@ * Author: Jose Abreu <Jose.Abreu@synopsys.com> */ +#include <linux/clk.h> #include <linux/delay.h> -#include <linux/pcs/pcs-xpcs.h> +#include <linux/device.h> #include <linux/mdio.h> +#include <linux/module.h> +#include <linux/pcs/pcs-xpcs.h> #include <linux/phylink.h> +#include <linux/property.h> #include "pcs-xpcs.h" @@ -1386,17 +1390,57 @@ static void xpcs_free_data(struct dw_xpcs *xpcs) kfree(xpcs); } +static int xpcs_init_clks(struct dw_xpcs *xpcs) +{ + static const char *ids[DW_XPCS_NUM_CLKS] = { + [DW_XPCS_CLK_CORE] = "core", + [DW_XPCS_CLK_PAD] = "pad", + }; + struct device *dev = &xpcs->mdiodev->dev; + int ret, i; + + for (i = 0; i < DW_XPCS_NUM_CLKS; ++i) + xpcs->clks[i].id = ids[i]; + + ret = clk_bulk_get_optional(dev, DW_XPCS_NUM_CLKS, xpcs->clks); + if (ret) + return dev_err_probe(dev, ret, "Failed to get clocks\n"); + + ret = clk_bulk_prepare_enable(DW_XPCS_NUM_CLKS, xpcs->clks); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable clocks\n"); + + return 0; +} + +static void xpcs_clear_clks(struct dw_xpcs *xpcs) +{ + clk_bulk_disable_unprepare(DW_XPCS_NUM_CLKS, xpcs->clks); + + clk_bulk_put(DW_XPCS_NUM_CLKS, xpcs->clks); +} + static int xpcs_init_id(struct dw_xpcs *xpcs) { - u32 xpcs_id; + const struct dw_xpcs_info *info; int i, ret; - xpcs_id = xpcs_get_id(xpcs); + info = device_get_match_data(&xpcs->mdiodev->dev) ?: + dev_get_platdata(&xpcs->mdiodev->dev); + if (!info) { + xpcs->info.did = DW_XPCS_ID_NATIVE; + xpcs->info.pma = DW_XPCS_PMA_UNKNOWN; + } else { + xpcs->info = *info; + } + + if (xpcs->info.did == DW_XPCS_ID_NATIVE) + xpcs->info.did = xpcs_get_id(xpcs); for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) { const struct xpcs_id *entry = &xpcs_id_list[i]; - if ((xpcs_id & entry->mask) != entry->id) + if ((xpcs->info.did & entry->mask) != entry->id) continue; xpcs->id = entry; @@ -1436,21 +1480,32 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, struct dw_xpcs *xpcs; int ret; + ret = device_attach(&mdiodev->dev); + if (ret < 0 && ret != -ENODEV) + return ERR_PTR(ret); + xpcs = xpcs_create_data(mdiodev); if (IS_ERR(xpcs)) return xpcs; + ret = xpcs_init_clks(xpcs); + if (ret) + goto out_free_data; + ret = xpcs_init_id(xpcs); if (ret) - goto out; + goto out_clear_clks; ret = xpcs_init_iface(xpcs, interface); if (ret) - goto out; + goto out_clear_clks; return xpcs; -out: +out_clear_clks: + xpcs_clear_clks(xpcs); + +out_free_data: xpcs_free_data(xpcs); return ERR_PTR(ret); @@ -1489,8 +1544,51 @@ void xpcs_destroy(struct dw_xpcs *xpcs) mdio_device_put(xpcs->mdiodev); + xpcs_clear_clks(xpcs); + xpcs_free_data(xpcs); } EXPORT_SYMBOL_GPL(xpcs_destroy); +DW_XPCS_INFO_DECLARE(xpcs_generic, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_UNKNOWN); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen1_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN1_3G); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen2_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN2_3G); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen2_6g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN2_6G); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen4_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN4_3G); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen4_6g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN4_6G); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen5_10g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN5_10G); +DW_XPCS_INFO_DECLARE(xpcs_pma_gen5_12g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN5_12G); + +static const struct of_device_id xpcs_of_ids[] = { + { .compatible = "snps,dw-xpcs", .data = &xpcs_generic }, + { .compatible = "snps,dw-xpcs-gen1-3g", .data = &xpcs_pma_gen1_3g }, + { .compatible = "snps,dw-xpcs-gen2-3g", .data = &xpcs_pma_gen2_3g }, + { .compatible = "snps,dw-xpcs-gen2-6g", .data = &xpcs_pma_gen2_6g }, + { .compatible = "snps,dw-xpcs-gen4-3g", .data = &xpcs_pma_gen4_3g }, + { .compatible = "snps,dw-xpcs-gen4-6g", .data = &xpcs_pma_gen4_6g }, + { .compatible = "snps,dw-xpcs-gen5-10g", .data = &xpcs_pma_gen5_10g }, + { .compatible = "snps,dw-xpcs-gen5-12g", .data = &xpcs_pma_gen5_12g }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, xpcs_of_ids); + +static struct mdio_device_id __maybe_unused xpcs_mdio_ids[] = { + { DW_XPCS_ID, DW_XPCS_ID_MASK }, + { NXP_SJA1105_XPCS_ID, DW_XPCS_ID_MASK }, + { NXP_SJA1110_XPCS_ID, DW_XPCS_ID_MASK }, + { } +}; +MODULE_DEVICE_TABLE(mdio, xpcs_mdio_ids); + +static struct mdio_driver xpcs_driver = { + .mdiodrv.driver = { + .name = "dwxpcs", + .of_match_table = xpcs_of_ids, + .probe_type = PROBE_FORCE_SYNCHRONOUS, + }, +}; +mdio_module_driver(xpcs_driver); + +MODULE_DESCRIPTION("DWC Ethernet XPCS platform driver"); +MODULE_AUTHOR("Jose Abreu <Jose.Abreu@synopsys.com>"); MODULE_LICENSE("GPL v2"); diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h index 369e9196f45a..45fea2641d23 100644 --- a/drivers/net/pcs/pcs-xpcs.h +++ b/drivers/net/pcs/pcs-xpcs.h @@ -6,6 +6,9 @@ * Author: Jose Abreu <Jose.Abreu@synopsys.com> */ +#include <linux/bits.h> +#include <linux/pcs/pcs-xpcs.h> + /* Vendor regs access */ #define DW_VENDOR BIT(15) @@ -117,6 +120,9 @@ /* VR MII EEE Control 1 defines */ #define DW_VR_MII_EEE_TRN_LPI BIT(0) /* Transparent Mode Enable */ +#define DW_XPCS_INFO_DECLARE(_name, _did, _pma) \ + static const struct dw_xpcs_info _name = { .did = _did, .pma = _pma } + int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg); int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val); int xpcs_read_vpcs(struct dw_xpcs *xpcs, int reg); diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h index 8dfe90295f12..53adbffb4c0a 100644 --- a/include/linux/pcs/pcs-xpcs.h +++ b/include/linux/pcs/pcs-xpcs.h @@ -7,9 +7,12 @@ #ifndef __LINUX_PCS_XPCS_H #define __LINUX_PCS_XPCS_H +#include <linux/clk.h> +#include <linux/mdio.h> #include <linux/phy.h> #include <linux/phylink.h> +#define DW_XPCS_ID_NATIVE 0x00000000 #define NXP_SJA1105_XPCS_ID 0x00000010 #define NXP_SJA1110_XPCS_ID 0x00000020 #define DW_XPCS_ID 0x7996ced0 @@ -30,9 +33,33 @@ struct xpcs_id; +enum dw_xpcs_pma { + DW_XPCS_PMA_UNKNOWN = 0, + DW_XPCS_PMA_GEN1_3G, + DW_XPCS_PMA_GEN2_3G, + DW_XPCS_PMA_GEN2_6G, + DW_XPCS_PMA_GEN4_3G, + DW_XPCS_PMA_GEN4_6G, + DW_XPCS_PMA_GEN5_10G, + DW_XPCS_PMA_GEN5_12G, +}; + +enum dw_xpcs_clock { + DW_XPCS_CLK_CORE, + DW_XPCS_CLK_PAD, + DW_XPCS_NUM_CLKS, +}; + +struct dw_xpcs_info { + u32 did; + u32 pma; +}; + struct dw_xpcs { struct mdio_device *mdiodev; + struct dw_xpcs_info info; const struct xpcs_id *id; + struct clk_bulk_data clks[DW_XPCS_NUM_CLKS]; struct phylink_pcs pcs; phy_interface_t interface; int dev_flag;
Recently the memory mapped Synopsys DW XPCS management interface support was added to the kernel. In that case the DW XPCS device can be registered via a standard MDIO-bus subsystem. In order to have such devices fully accessible and properly configured let's add a respective functionality to the DW XPCS driver. The main goal of this update is to add a functionality to activate vendor-specific XPCS capabilities in the driver (like a limited number of network interfaces, linkmodes, PMA-specific initializations). It's reached by having DW XPCS devices registered as the platform devices (OF, ACPI, legacy platform ,etc). From that point of view the suggested update is threefold. First the driver is now capable to be attached to the OF-devices registered on the MDIO-bus with the "snps,dw-xpcs*" compatible string. Second it's possible to have the driver bound to the DW XPCS device with no OF/ACPI-nodes by means of defining the mdio_board_info descriptor and registering one with mdiobus_register_board_info() (see dwmac-intel.c for example). Thirdly it's still possible to use the unregistered device to auto-detect the DW XPCS device on the MDIO bus. In all these cases the DW XPCS device info can be passed by means of the driver-data pointer (of_device_id.data or device.platform_data). In addition to that the update provides the DW XPCS reference clock sources request and enabling. These clocks are named as "core" and "pad" as per the DT-bindings. Note normally they are mutually exclusive: only one of them can be used at a time, but the system software is responsible for switching between them. Such functionality will be added later in the framework of the pma_config() internal callback. Note all the platform resources initialization is performed in the externally called xpcs_create() method as before this update. The only crucial update is that it now makes sure that the device is bound to the DW XPCS driver if it's possible. Otherwise the legacy auto-detection procedure takes place as before. Moreover due to that semantic there is no device probe() and remove() methods defined since there is nothing left to initialize/de-initialize on these stages. Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/net/pcs/Kconfig | 6 +- drivers/net/pcs/pcs-xpcs.c | 112 ++++++++++++++++++++++++++++++++--- drivers/net/pcs/pcs-xpcs.h | 6 ++ include/linux/pcs/pcs-xpcs.h | 27 +++++++++ 4 files changed, 141 insertions(+), 10 deletions(-)