Re: [PATCH net-next] net: mscc: ocelot: Fix probe for vsc7514
From: Horatiu Vultur
Date: Wed Aug 18 2021 - 03:31:51 EST
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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxx>
> 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@xxxxxxx>
> ---
> 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 ]-----------------------------
--
/Horatiu