LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Claudiu Manoil <claudiu.manoil@nxp.com>,
	"alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] net: mscc: ocelot: Fix probe for vsc7514
Date: Wed, 18 Aug 2021 09:32:48 +0200	[thread overview]
Message-ID: <20210818073248.as2mbc7ea2rttuxu@soft-dev3-1.localhost> (raw)
In-Reply-To: <20210817122412.6zjvjbfk3dnyh6uz@skbuf>

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", &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 ]-----------------------------

-- 
/Horatiu

      reply	other threads:[~2021-08-18  7:31 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17 12:06 Horatiu Vultur
2021-08-17 12:24 ` Vladimir Oltean
2021-08-18  7:32   ` Horatiu Vultur [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210818073248.as2mbc7ea2rttuxu@soft-dev3-1.localhost \
    --to=horatiu.vultur@microchip.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=claudiu.manoil@nxp.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vladimir.oltean@nxp.com \
    --subject='Re: [PATCH net-next] net: mscc: ocelot: Fix probe for vsc7514' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).