| Message ID | 20210817120633.404790-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mscc: ocelot: Fix probe for vsc7514 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hi Horatiu, On Tue, Aug 17, 2021 at 02:06:33PM +0200, Horatiu Vultur wrote: > The check for parsing the 'phy-handle' was removed in the blamed commit. > Therefor it would try to create phylinks for each port and connect to > the phys. But on ocelot_pcb123 and ocelot_pcb120 not all the ports have > a phy, so this will failed. So the probe of the network driver will > fail. > > The fix consists in adding back the check for 'phy-handle' for vsc7514 > > Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink") > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > index 18aed504f45d..96ac64f13382 100644 > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > @@ -954,6 +954,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, > if (of_property_read_u32(portnp, "reg", ®)) > continue; > > + if (!of_parse_phandle(portnp, "phy-handle", 0)) > + continue; > + > port = reg; > if (port < 0 || port >= ocelot->num_phys_ports) { > dev_err(ocelot->dev, > -- > 2.31.1 > Thanks a lot for taking the time to test! What do you think about this alternative? It should not limit the driver to only having phy-handle (having a fixed-link is valid too): -----------------------------[ cut here ]----------------------------- >From 598f7795389fb127726de199bb77fd7ddf5df096 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: Tue, 17 Aug 2021 15:14:57 +0300 Subject: [PATCH] net: mscc: ocelot: allow probing to continue with ports that fail to register The existing ocelot device trees, like ocelot_pcb123.dts for example, have SERDES ports (ports 4 and higher) that do not have status = "disabled"; but on the other hand do not have a phy-handle or a fixed-link either. So from the perspective of phylink, they have broken DT bindings. Since the blamed commit, probing for the entire switch will fail when such a device tree binding is encountered on a port. There used to be this piece of code which skipped ports without a phy-handle: phy_node = of_parse_phandle(portnp, "phy-handle", 0); if (!phy_node) continue; but now it is gone. Anyway, fixed-link setups are a thing which should work out of the box with phylink, so it would not be in the best interest of the driver to add that check back. Instead, let's look at what other drivers do. Since commit 86f8b1c01a0a ("net: dsa: Do not make user port errors fatal"), DSA continues after a switch port fails to register, and works only with the ports that succeeded. We can achieve the same behavior in ocelot by unregistering the devlink port for ports where ocelot_port_phylink_create() failed (called via ocelot_probe_port), and clear the bit in devlink_ports_registered for that port. This will make the next iteration reconsider the port that failed to probe as an unused port, and re-register a devlink port of type UNUSED for it. No other cleanup should need to be performed, since ocelot_probe_port() should be self-contained when it fails. Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 18aed504f45d..291ae6817c26 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -978,14 +978,15 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, of_node_put(portnp); goto out_teardown; } - devlink_ports_registered |= BIT(port); err = ocelot_probe_port(ocelot, port, target, portnp); if (err) { - of_node_put(portnp); - goto out_teardown; + ocelot_port_devlink_teardown(ocelot, port); + continue; } + devlink_ports_registered |= BIT(port); + ocelot_port = ocelot->ports[port]; priv = container_of(ocelot_port, struct ocelot_port_private, port); -----------------------------[ cut here ]-----------------------------
The 08/17/2021 12:24, Vladimir Oltean wrote: Hi Vladimir, Thanks for the patch, it seems to fix the initial problem(the probe function failed) but then I start to get some warnings in the probe function. [ 2.178345] ------------[ cut here ]------------ [ 2.183249] WARNING: CPU: 0 PID: 1 at net/core/devlink.c:9275 ocelot_port_devlink_init+0x88/0xd0 [ 2.192333] Modules linked in: [ 2.195617] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc5-01260-g0f217b897eeb-dirty #113 [ 2.204414] Stack : 00000001 81d300e0 0000000f 8018ccc0 00000001 00000004 00000000 b7c1f2a2 [ 2.212876] 81455aec 80b142a8 80a30000 80a34663 809ce3d0 00000001 81455a90 81441800 [ 2.221331] 00000000 00000000 809ce3d0 81455930 00000001 81455944 00000000 0000076c [ 2.229783] 25784660 81455949 ffffffff 00000010 80a30000 809ce3d0 00000009 00000009 [ 2.238235] 00000000 00000006 00000001 81d300e0 00000000 805c9028 00000000 80b10000 [ 2.246687] ... [ 2.249163] Call Trace: [ 2.251626] [<80108b34>] show_stack+0x38/0x118 [ 2.256166] [<808d18e4>] dump_stack_lvl+0x68/0x94 [ 2.260937] [<808cea2c>] __warn+0xc0/0xe8 [ 2.264985] [<808ceac0>] warn_slowpath_fmt+0x6c/0xd0 [ 2.269993] [<8067ede0>] ocelot_port_devlink_init+0x88/0xd0 [ 2.275608] [<8067cfa0>] mscc_ocelot_probe+0x688/0x808 [ 2.280823] [<805df698>] platform_probe+0x68/0xf4 [ 2.285587] [<805dd3a8>] really_probe+0xc8/0x350 [ 2.290244] [<805dd6c4>] __driver_probe_device+0x94/0x10c [ 2.295686] [<805dd784>] driver_probe_device+0x48/0x110 [ 2.300954] [<805ddef8>] __driver_attach+0x7c/0x124 [ 2.305871] [<805db430>] bus_for_each_dev+0x7c/0xc8 [ 2.310822] [<805dbfa0>] bus_add_driver+0x1f4/0x204 [ 2.315739] [<805de9d8>] driver_register+0x84/0x15c [ 2.320660] [<80100158>] do_one_initcall+0x8c/0x1e4 [ 2.325579] [<80ac11cc>] kernel_init_freeable+0x268/0x30c [ 2.331049] [<808d53c4>] kernel_init+0x24/0x114 [ 2.335637] [<80102698>] ret_from_kernel_thread+0x14/0x1c [ 2.341081] [ 2.342910] ---[ end trace bf0038db663c1a65 ]--- [ 2.347793] ------------[ cut here ]------------ [ 2.352612] WARNING: CPU: 0 PID: 1 at net/core/devlink.c:9114 devlink_port_register+0x164/0x17c [ 2.361678] Modules linked in: [ 2.364809] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 5.14.0-rc5-01260-g0f217b897eeb-dirty #113 [ 2.374959] Stack : 00000001 81d300e0 0000000f 8018ccc0 00000001 00000004 00000000 b7c1f2a2 [ 2.383417] 81455abc 80b142a8 80a30000 80a34663 809ce3d0 00000001 81455a60 81441800 [ 2.391872] 00000000 00000000 809ce3d0 81455900 00000001 81455914 00000000 00000e10 [ 2.400324] f9a01ed8 81455919 ffffffff 00000010 80a30000 809ce3d0 00000009 00000009 [ 2.408777] 00000000 00000006 00000001 81d300e0 00000000 805c9028 00000000 80b10000 [ 2.417229] ... [ 2.419704] Call Trace: [ 2.422167] [<80108b34>] show_stack+0x38/0x118 [ 2.426689] [<808d18e4>] dump_stack_lvl+0x68/0x94 [ 2.431442] [<808cea2c>] __warn+0xc0/0xe8 [ 2.435488] [<808ceac0>] warn_slowpath_fmt+0x6c/0xd0 [ 2.440494] [<8073edd4>] devlink_port_register+0x164/0x17c [ 2.446026] [<8067edf0>] ocelot_port_devlink_init+0x98/0xd0 [ 2.451645] [<8067cfa0>] mscc_ocelot_probe+0x688/0x808 [ 2.456853] [<805df698>] platform_probe+0x68/0xf4 [ 2.461611] [<805dd3a8>] really_probe+0xc8/0x350 [ 2.466268] [<805dd6c4>] __driver_probe_device+0x94/0x10c [ 2.471709] [<805dd784>] driver_probe_device+0x48/0x110 [ 2.476976] [<805ddef8>] __driver_attach+0x7c/0x124 [ 2.481895] [<805db430>] bus_for_each_dev+0x7c/0xc8 [ 2.486828] [<805dbfa0>] bus_add_driver+0x1f4/0x204 [ 2.491746] [<805de9d8>] driver_register+0x84/0x15c [ 2.496666] [<80100158>] do_one_initcall+0x8c/0x1e4 [ 2.501586] [<80ac11cc>] kernel_init_freeable+0x268/0x30c [ 2.507041] [<808d53c4>] kernel_init+0x24/0x114 [ 2.511621] [<80102698>] ret_from_kernel_thread+0x14/0x1c [ 2.517067] [ 2.518811] ---[ end trace bf0038db663c1a66 ]--- These warnings are generated when trying to initialize unused devlink ports. To fix this warnings I have added the following line inside 'ocelot_port_devlink_teardown'. memset(dlp, 0x0, sizeof(struct devlink_port)); > > Hi Horatiu, > > On Tue, Aug 17, 2021 at 02:06:33PM +0200, Horatiu Vultur wrote: > > The check for parsing the 'phy-handle' was removed in the blamed commit. > > Therefor it would try to create phylinks for each port and connect to > > the phys. But on ocelot_pcb123 and ocelot_pcb120 not all the ports have > > a phy, so this will failed. So the probe of the network driver will > > fail. > > > > The fix consists in adding back the check for 'phy-handle' for vsc7514 > > > > Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink") > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > > index 18aed504f45d..96ac64f13382 100644 > > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c > > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > > @@ -954,6 +954,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, > > if (of_property_read_u32(portnp, "reg", ®)) > > continue; > > > > + if (!of_parse_phandle(portnp, "phy-handle", 0)) > > + continue; > > + > > port = reg; > > if (port < 0 || port >= ocelot->num_phys_ports) { > > dev_err(ocelot->dev, > > -- > > 2.31.1 > > > > Thanks a lot for taking the time to test! > > What do you think about this alternative? It should not limit the driver > to only having phy-handle (having a fixed-link is valid too): > > -----------------------------[ cut here ]----------------------------- > From 598f7795389fb127726de199bb77fd7ddf5df096 Mon Sep 17 00:00:00 2001 > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Date: Tue, 17 Aug 2021 15:14:57 +0300 > Subject: [PATCH] net: mscc: ocelot: allow probing to continue with ports that > fail to register > > The existing ocelot device trees, like ocelot_pcb123.dts for example, > have SERDES ports (ports 4 and higher) that do not have status = "disabled"; > but on the other hand do not have a phy-handle or a fixed-link either. > > So from the perspective of phylink, they have broken DT bindings. > > Since the blamed commit, probing for the entire switch will fail when > such a device tree binding is encountered on a port. There used to be > this piece of code which skipped ports without a phy-handle: > > phy_node = of_parse_phandle(portnp, "phy-handle", 0); > if (!phy_node) > continue; > > but now it is gone. > > Anyway, fixed-link setups are a thing which should work out of the box > with phylink, so it would not be in the best interest of the driver to > add that check back. > > Instead, let's look at what other drivers do. Since commit 86f8b1c01a0a > ("net: dsa: Do not make user port errors fatal"), DSA continues after a > switch port fails to register, and works only with the ports that > succeeded. > > We can achieve the same behavior in ocelot by unregistering the devlink > port for ports where ocelot_port_phylink_create() failed (called via > ocelot_probe_port), and clear the bit in devlink_ports_registered for > that port. This will make the next iteration reconsider the port that > failed to probe as an unused port, and re-register a devlink port of > type UNUSED for it. No other cleanup should need to be performed, since > ocelot_probe_port() should be self-contained when it fails. > > Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > index 18aed504f45d..291ae6817c26 100644 > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > @@ -978,14 +978,15 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, > of_node_put(portnp); > goto out_teardown; > } > - devlink_ports_registered |= BIT(port); > > err = ocelot_probe_port(ocelot, port, target, portnp); > if (err) { > - of_node_put(portnp); > - goto out_teardown; > + ocelot_port_devlink_teardown(ocelot, port); > + continue; > } > > + devlink_ports_registered |= BIT(port); > + > ocelot_port = ocelot->ports[port]; > priv = container_of(ocelot_port, struct ocelot_port_private, > port); > -----------------------------[ cut here ]-----------------------------
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 18aed504f45d..96ac64f13382 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -954,6 +954,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, if (of_property_read_u32(portnp, "reg", ®)) continue; + if (!of_parse_phandle(portnp, "phy-handle", 0)) + continue; + port = reg; if (port < 0 || port >= ocelot->num_phys_ports) { dev_err(ocelot->dev,
The check for parsing the 'phy-handle' was removed in the blamed commit. Therefor it would try to create phylinks for each port and connect to the phys. But on ocelot_pcb123 and ocelot_pcb120 not all the ports have a phy, so this will failed. So the probe of the network driver will fail. The fix consists in adding back the check for 'phy-handle' for vsc7514 Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink") Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 3 +++ 1 file changed, 3 insertions(+)