| Message ID | 20220711160519.741990-5-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: Add support for devices probed in the "usual" manner | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On Mon, Jul 11, 2022 at 12:05:14PM -0400, Sean Anderson wrote: > This converts the lynx PCS driver to a proper MDIO driver. This allows > using a more conventional driver lifecycle (e.g. with a probe and > remove). For compatibility with existing device trees lacking a > compatible property, we bind the driver in lynx_pcs_create. This is > intended only as a transitional method. After compatible properties are > added to all existing device trees (and a reasonable amount of time has > passed), then lynx_pcs_create can be removed, and users can be converted > to pcs_get_fwnode. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- I'm compiling and testing patch by patch now. Here's how things go on LS1028A at this stage: [ 6.317357] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000110 [ 6.326219] Mem abort info: [ 6.329027] ESR = 0x0000000096000004 [ 6.332815] EC = 0x25: DABT (current EL), IL = 32 bits [ 6.338182] SET = 0, FnV = 0 [ 6.341252] EA = 0, S1PTW = 0 [ 6.344436] FSC = 0x04: level 0 translation fault [ 6.349378] Data abort info: [ 6.352273] ISV = 0, ISS = 0x00000004 [ 6.356154] CM = 0, WnR = 0 [ 6.359164] [0000000000000110] user address but active_mm is swapper [ 6.365629] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 6.371221] Modules linked in: [ 6.374284] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3317 [ 6.383364] Hardware name: LS1028A RDB Board (DT) [ 6.388081] Workqueue: events_unbound deferred_probe_work_func [ 6.393939] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 6.400926] pc : __driver_probe_device+0x1c/0x150 [ 6.405646] lr : device_driver_attach+0x58/0xc0 [ 6.410190] sp : ffff8000085639c0 [ 6.413510] x29: ffff8000085639c0 x28: ffffb1a2587dae50 x27: ffff2b6943304bc0 [ 6.420676] x26: ffff2b694330c000 x25: ffff2b69433010a0 x24: ffff2b69bf719898 [ 6.427840] x23: ffff2b6941074000 x22: ffff2b6943304000 x21: ffff2b6943301880 [ 6.435004] x20: ffff2b6943301800 x19: ffffb1a259faf3d0 x18: ffffffffffffffff [ 6.442168] x17: 000000002b64f81b x16: 000000006d50a0b2 x15: ffff2b6943307196 [ 6.449332] x14: 0000000000000002 x13: ffff2b6943307194 x12: 0000000000000003 [ 6.456497] x11: ffff2b69433018f0 x10: 0000000000000003 x9 : ffffb1a2578b1e08 [ 6.463662] x8 : ffff2b6940b36200 x7 : ffffb1a25a0da000 x6 : 000000003225858e [ 6.470826] x5 : 0000000000000000 x4 : ffff79c76227a000 x3 : 0000000000000000 [ 6.477989] x2 : 0000000000000000 x1 : ffff2b6943301800 x0 : ffffb1a259faf3d0 [ 6.485153] Call trace: [ 6.487601] __driver_probe_device+0x1c/0x150 [ 6.491971] device_driver_attach+0x58/0xc0 [ 6.496167] lynx_pcs_create+0x30/0x7c [ 6.499927] enetc_pf_probe+0x984/0xeb0 [ 6.503775] local_pci_probe+0x4c/0xc0 [ 6.507536] pci_device_probe+0xb8/0x210 [ 6.511470] really_probe.part.0+0xa4/0x2b0 [ 6.515665] __driver_probe_device+0xa0/0x150 [ 6.520033] driver_probe_device+0xb4/0x150 [ 6.524228] __device_attach_driver+0xc4/0x130 [ 6.528684] bus_for_each_drv+0x84/0xe0 [ 6.532529] __device_attach+0xb0/0x1d0 [ 6.536375] device_initial_probe+0x20/0x2c [ 6.540569] bus_probe_device+0xac/0xb4 [ 6.544414] deferred_probe_work_func+0x98/0xd4 [ 6.548956] process_one_work+0x294/0x6d0 [ 6.552979] worker_thread+0x80/0x460 [ 6.556651] kthread+0x124/0x130 [ 6.559887] ret_from_fork+0x10/0x20 [ 6.563475] Code: a9bd7bfd 910003fd a90153f3 f9402422 (39444042) Disassembly of drivers/base/dd.c shows that dev->p is a NULL pointer, and dev->p->dead goes right through it. How did we even get here... device_private_init() should be called by device_add(). Curiously enough, mdio_device_create() only calls device_initialize(). It's mdio_device_register() that calls device_add(). So after this patch, we cannot call lynx_pcs_create() without calling mdio_device_register().
On 7/19/22 12:01 PM, Vladimir Oltean wrote: > On Mon, Jul 11, 2022 at 12:05:14PM -0400, Sean Anderson wrote: >> This converts the lynx PCS driver to a proper MDIO driver. This allows >> using a more conventional driver lifecycle (e.g. with a probe and >> remove). For compatibility with existing device trees lacking a >> compatible property, we bind the driver in lynx_pcs_create. This is >> intended only as a transitional method. After compatible properties are >> added to all existing device trees (and a reasonable amount of time has >> passed), then lynx_pcs_create can be removed, and users can be converted >> to pcs_get_fwnode. >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- > > I'm compiling and testing patch by patch now. Here's how things go on > LS1028A at this stage: > > [ 6.317357] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000110 > [ 6.326219] Mem abort info: > [ 6.329027] ESR = 0x0000000096000004 > [ 6.332815] EC = 0x25: DABT (current EL), IL = 32 bits > [ 6.338182] SET = 0, FnV = 0 > [ 6.341252] EA = 0, S1PTW = 0 > [ 6.344436] FSC = 0x04: level 0 translation fault > [ 6.349378] Data abort info: > [ 6.352273] ISV = 0, ISS = 0x00000004 > [ 6.356154] CM = 0, WnR = 0 > [ 6.359164] [0000000000000110] user address but active_mm is swapper > [ 6.365629] Internal error: Oops: 96000004 [#1] PREEMPT SMP > [ 6.371221] Modules linked in: > [ 6.374284] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc6-07010-ga9b9500ffaac-dirty #3317 > [ 6.383364] Hardware name: LS1028A RDB Board (DT) > [ 6.388081] Workqueue: events_unbound deferred_probe_work_func > [ 6.393939] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 6.400926] pc : __driver_probe_device+0x1c/0x150 > [ 6.405646] lr : device_driver_attach+0x58/0xc0 > [ 6.410190] sp : ffff8000085639c0 > [ 6.413510] x29: ffff8000085639c0 x28: ffffb1a2587dae50 x27: ffff2b6943304bc0 > [ 6.420676] x26: ffff2b694330c000 x25: ffff2b69433010a0 x24: ffff2b69bf719898 > [ 6.427840] x23: ffff2b6941074000 x22: ffff2b6943304000 x21: ffff2b6943301880 > [ 6.435004] x20: ffff2b6943301800 x19: ffffb1a259faf3d0 x18: ffffffffffffffff > [ 6.442168] x17: 000000002b64f81b x16: 000000006d50a0b2 x15: ffff2b6943307196 > [ 6.449332] x14: 0000000000000002 x13: ffff2b6943307194 x12: 0000000000000003 > [ 6.456497] x11: ffff2b69433018f0 x10: 0000000000000003 x9 : ffffb1a2578b1e08 > [ 6.463662] x8 : ffff2b6940b36200 x7 : ffffb1a25a0da000 x6 : 000000003225858e > [ 6.470826] x5 : 0000000000000000 x4 : ffff79c76227a000 x3 : 0000000000000000 > [ 6.477989] x2 : 0000000000000000 x1 : ffff2b6943301800 x0 : ffffb1a259faf3d0 > [ 6.485153] Call trace: > [ 6.487601] __driver_probe_device+0x1c/0x150 > [ 6.491971] device_driver_attach+0x58/0xc0 > [ 6.496167] lynx_pcs_create+0x30/0x7c > [ 6.499927] enetc_pf_probe+0x984/0xeb0 > [ 6.503775] local_pci_probe+0x4c/0xc0 > [ 6.507536] pci_device_probe+0xb8/0x210 > [ 6.511470] really_probe.part.0+0xa4/0x2b0 > [ 6.515665] __driver_probe_device+0xa0/0x150 > [ 6.520033] driver_probe_device+0xb4/0x150 > [ 6.524228] __device_attach_driver+0xc4/0x130 > [ 6.528684] bus_for_each_drv+0x84/0xe0 > [ 6.532529] __device_attach+0xb0/0x1d0 > [ 6.536375] device_initial_probe+0x20/0x2c > [ 6.540569] bus_probe_device+0xac/0xb4 > [ 6.544414] deferred_probe_work_func+0x98/0xd4 > [ 6.548956] process_one_work+0x294/0x6d0 > [ 6.552979] worker_thread+0x80/0x460 > [ 6.556651] kthread+0x124/0x130 > [ 6.559887] ret_from_fork+0x10/0x20 > [ 6.563475] Code: a9bd7bfd 910003fd a90153f3 f9402422 (39444042) > > Disassembly of drivers/base/dd.c shows that dev->p is a NULL pointer, > and dev->p->dead goes right through it. How did we even get here... > device_private_init() should be called by device_add(). > > Curiously enough, mdio_device_create() only calls device_initialize(). > It's mdio_device_register() that calls device_add(). So after this > patch, we cannot call lynx_pcs_create() without calling > mdio_device_register(). OK, so presumably we need to call mdio_device_register after mdio_device_create. I suppose I should have caught this because patch 5 does exactly this. --Sean
On Tue, Jul 19, 2022 at 12:16:17PM -0400, Sean Anderson wrote: > > Curiously enough, mdio_device_create() only calls device_initialize(). > > It's mdio_device_register() that calls device_add(). So after this > > patch, we cannot call lynx_pcs_create() without calling > > mdio_device_register(). > > OK, so presumably we need to call mdio_device_register after mdio_device_create. > I suppose I should have caught this because patch 5 does exactly this. And that's only to not crash during boot, mind you. Networking is broken at this stage. I'll proceed to patch 5 to see what happens next.
diff --git a/drivers/net/dsa/ocelot/Kconfig b/drivers/net/dsa/ocelot/Kconfig index 220b0b027b55..cbb0ced3f37d 100644 --- a/drivers/net/dsa/ocelot/Kconfig +++ b/drivers/net/dsa/ocelot/Kconfig @@ -10,6 +10,7 @@ config NET_DSA_MSCC_FELIX select NET_DSA_TAG_OCELOT_8021Q select NET_DSA_TAG_OCELOT select FSL_ENETC_MDIO + select PCS select PCS_LYNX help This driver supports the VSC9959 (Felix) switch, which is embedded as @@ -25,6 +26,7 @@ config NET_DSA_MSCC_SEVILLE select MSCC_OCELOT_SWITCH_LIB select NET_DSA_TAG_OCELOT_8021Q select NET_DSA_TAG_OCELOT + select PCS select PCS_LYNX help This driver supports the VSC9953 (Seville) switch, which is embedded diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index 570d0204b7be..57634e2296c0 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -1094,7 +1094,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) continue; phylink_pcs = lynx_pcs_create(mdio_device); - if (!phylink_pcs) { + if (IS_ERR(phylink_pcs)) { mdio_device_free(mdio_device); continue; } diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index ea0649211356..8c52de5d0b02 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -1049,7 +1049,7 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot) continue; phylink_pcs = lynx_pcs_create(mdio_device); - if (!phylink_pcs) { + if (IS_ERR(phylink_pcs)) { mdio_device_free(mdio_device); continue; } diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig b/drivers/net/ethernet/freescale/dpaa2/Kconfig index d029b69c3f18..2648e9fb6e13 100644 --- a/drivers/net/ethernet/freescale/dpaa2/Kconfig +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig @@ -3,6 +3,7 @@ config FSL_DPAA2_ETH tristate "Freescale DPAA2 Ethernet" depends on FSL_MC_BUS && FSL_MC_DPIO select PHYLINK + select PCS select PCS_LYNX select FSL_XGMAC_MDIO select NET_DEVLINK diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index c9bee9a0c9b2..e82c0d23eeb5 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -268,10 +268,10 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac, return -EPROBE_DEFER; mac->pcs = lynx_pcs_create(mdiodev); - if (!mac->pcs) { + if (IS_ERR(mac->pcs)) { netdev_err(mac->net_dev, "lynx_pcs_create() failed\n"); put_device(&mdiodev->dev); - return -ENOMEM; + return PTR_ERR(mac->pcs); } return 0; diff --git a/drivers/net/ethernet/freescale/enetc/Kconfig b/drivers/net/ethernet/freescale/enetc/Kconfig index cdc0ff89388a..c7dcdeb9a333 100644 --- a/drivers/net/ethernet/freescale/enetc/Kconfig +++ b/drivers/net/ethernet/freescale/enetc/Kconfig @@ -5,6 +5,7 @@ config FSL_ENETC select FSL_ENETC_IERB select FSL_ENETC_MDIO select PHYLINK + select PCS select PCS_LYNX select DIMLIB help diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c index c4a0e836d4f0..8c923a93da88 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c @@ -859,9 +859,9 @@ static int enetc_imdio_create(struct enetc_pf *pf) } phylink_pcs = lynx_pcs_create(mdio_device); - if (!phylink_pcs) { + if (IS_ERR(phylink_pcs)) { mdio_device_free(mdio_device); - err = -ENOMEM; + err = PTR_ERR(phylink_pcs); dev_err(dev, "cannot create lynx pcs (%d)\n", err); goto unregister_mdiobus; } diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig index fed6264fdf33..a225176f92e8 100644 --- a/drivers/net/pcs/Kconfig +++ b/drivers/net/pcs/Kconfig @@ -25,9 +25,14 @@ config PCS_XPCS controllers. config PCS_LYNX - tristate + tristate "NXP Lynx PCS driver" + depends on PCS && MDIO_DEVICE help - This module provides helpers to phylink for managing the Lynx PCS - which is part of the Layerscape and QorIQ Ethernet SERDES. + This module provides driver support for the PCSs in Lynx 10g and 28g + SerDes devices. These devices are present in NXP QorIQ SoCs, + including the Layerscape series. + + If you want to use Ethernet on a QorIQ SoC, say "Y". If compiled as a + module, it will be called "pcs-lynx". endmenu diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c index fd3445374955..8272072698e4 100644 --- a/drivers/net/pcs/pcs-lynx.c +++ b/drivers/net/pcs/pcs-lynx.c @@ -1,11 +1,14 @@ -// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) -/* Copyright 2020 NXP +// SPDX-License-Identifier: GPL-2.0+ +/* Copyright (C) 2022 Sean Anderson <seanga2@gmail.com> + * Copyright 2020 NXP * Lynx PCS MDIO helpers */ #include <linux/mdio.h> -#include <linux/phylink.h> +#include <linux/of.h> +#include <linux/pcs.h> #include <linux/pcs-lynx.h> +#include <linux/phylink.h> #define SGMII_CLOCK_PERIOD_NS 8 /* PCS is clocked at 125 MHz */ #define LINK_TIMER_VAL(ns) ((u32)((ns) / SGMII_CLOCK_PERIOD_NS)) @@ -337,34 +340,74 @@ static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode, } static const struct phylink_pcs_ops lynx_pcs_phylink_ops = { + .owner = THIS_MODULE, .pcs_get_state = lynx_pcs_get_state, .pcs_config = lynx_pcs_config, .pcs_an_restart = lynx_pcs_an_restart, .pcs_link_up = lynx_pcs_link_up, }; -struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) +static int lynx_pcs_probe(struct mdio_device *mdio) { + struct device *dev = &mdio->dev; struct lynx_pcs *lynx; + int ret; - lynx = kzalloc(sizeof(*lynx), GFP_KERNEL); + lynx = devm_kzalloc(dev, sizeof(*lynx), GFP_KERNEL); if (!lynx) - return NULL; + return -ENOMEM; lynx->mdio = mdio; + lynx->pcs.dev = dev; lynx->pcs.ops = &lynx_pcs_phylink_ops; lynx->pcs.poll = true; - return lynx_to_phylink_pcs(lynx); + ret = devm_pcs_register(dev, &lynx->pcs); + if (ret) + return dev_err_probe(dev, ret, "could not register PCS\n"); + dev_info(dev, "probed\n"); + return 0; +} + +static const struct of_device_id lynx_pcs_of_match[] = { + { .compatible = "fsl,lynx-pcs" }, + { }, +}; +MODULE_DEVICE_TABLE(of, lynx_pcs_of_match); + +static struct mdio_driver lynx_pcs_driver = { + .probe = lynx_pcs_probe, + .mdiodrv.driver = { + .name = "lynx-pcs", + .of_match_table = of_match_ptr(lynx_pcs_of_match), + }, +}; +mdio_module_driver(lynx_pcs_driver); + +struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) +{ + struct device *dev = &mdio->dev; + int err; + + /* For compatibility with device trees lacking compatible strings, we + * bind the device manually here. + */ + err = device_driver_attach(&lynx_pcs_driver.mdiodrv.driver, dev); + if (err && err != -EBUSY) { + if (err == -EAGAIN) + err = -EPROBE_DEFER; + return ERR_PTR(err); + } + + return pcs_get_by_provider(&mdio->dev); } EXPORT_SYMBOL(lynx_pcs_create); void lynx_pcs_destroy(struct phylink_pcs *pcs) { - struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); - - kfree(lynx); + pcs_put(pcs); } EXPORT_SYMBOL(lynx_pcs_destroy); -MODULE_LICENSE("Dual BSD/GPL"); +MODULE_DESCRIPTION("NXP Lynx 10G/28G PCS driver"); +MODULE_LICENSE("GPL");
This converts the lynx PCS driver to a proper MDIO driver. This allows using a more conventional driver lifecycle (e.g. with a probe and remove). For compatibility with existing device trees lacking a compatible property, we bind the driver in lynx_pcs_create. This is intended only as a transitional method. After compatible properties are added to all existing device trees (and a reasonable amount of time has passed), then lynx_pcs_create can be removed, and users can be converted to pcs_get_fwnode. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/net/dsa/ocelot/Kconfig | 2 + drivers/net/dsa/ocelot/felix_vsc9959.c | 2 +- drivers/net/dsa/ocelot/seville_vsc9953.c | 2 +- drivers/net/ethernet/freescale/dpaa2/Kconfig | 1 + .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 4 +- drivers/net/ethernet/freescale/enetc/Kconfig | 1 + .../net/ethernet/freescale/enetc/enetc_pf.c | 4 +- drivers/net/pcs/Kconfig | 11 +++- drivers/net/pcs/pcs-lynx.c | 65 +++++++++++++++---- 9 files changed, 72 insertions(+), 20 deletions(-)