RE: [PATCH v2] net: fec: manage corner deferred probe condition

[Wei Fang]

> -----Original Message-----
> From: Pierluigi Passaro <pierluigi.p@xxxxxxxxxxxxx>
> Sent: 2023年1月17日 4:23
> To: Andrew Lunn <andrew@xxxxxxx>
> Cc: Pierluigi Passaro <pierluigi.passaro@xxxxxxxxx>; Wei Fang
> <wei.fang@xxxxxxx>; Shenwei Wang <shenwei.wang@xxxxxxx>; Clark Wang
> <xiaoning.wang@xxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> eran.m@xxxxxxxxxxxxx; Nate Drude <Nate.D@xxxxxxxxxxxxx>; Francesco
> Ferraro <francesco.f@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v2] net: fec: manage corner deferred probe condition
> 
> On Mon, Jan 16, 2023 at 4:32 PM Andrew Lunn <andrew@xxxxxxx> wrote:
> > > This is the setup of the corner case:
> > > - FEC0 is the owner of MDIO bus, but its own PHY rely on a "delayed"
> > > GPIO
> > > - FEC1 rely on FEC0 for MDIO communications The sequence is
> > > something like this
> > > - FEC0 probe start, but being the reset GPIO "delayed" it return
> > > EPROBE_DEFERRED
> > > - FEC1 is successfully probed: being the MDIO bus still not owned,
> > > the driver assume
> > >   that the ownership must be assigned to the 1st one successfully
> > > probed, but no
> > >   MDIO node is actually present and no communication takes place.
> >
> > So semantics of a phandle is that you expect what it points to, to
> > exists. So if phy-handle points to a PHY, when you follow that pointer
> > and find it missing, you should defer the probe. So this step should
> > not succeed.
> >
> I agree with you: the check is present, but the current logic is not consistent.
> Whenever the node owning the MDIO fails the probe due to
> EPROBE_DEFERRED, also the second node must defer the probe, otherwise no
> MDIO communication is possible.
> That's why the patch set the static variable wait_for_mdio_bus to track the
> status.
> > > - FEC0 is successfully probed, but MDIO bus is now assigned to FEC1
> > >   and cannot  and no communication takes place
> >

Have you tested that this issue also exists on the net tree? According to your
description, I simulated your situation on the net tree and tested it with imx6ul,
but the problem you mentioned does not exist. Below is is my test patch.

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 644f3c963730..e4f6937cdc3e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2284,6 +2284,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
        int err = -ENXIO;
        u32 mii_speed, holdtime;
        u32 bus_freq;
+       static bool test_flag;

        /*
         * The i.MX28 dual fec interfaces are not equal.
@@ -2388,7 +2389,12 @@ static int fec_enet_mii_init(struct platform_device *pdev)
        fep->mii_bus->priv = fep;
        fep->mii_bus->parent = &pdev->dev;

-       err = of_mdiobus_register(fep->mii_bus, node);
+       if (node && !test_flag) {
+               err = -EPROBE_DEFER;
+               test_flag = true;
+       } else {
+               err = of_mdiobus_register(fep->mii_bus, node);
+       }
        if (err)
                goto err_out_free_mdiobus;
        of_node_put(node);
@@ -4349,8 +4355,9 @@ fec_probe(struct platform_device *pdev)

        /* Decide which interrupt line is wakeup capable */
        fec_enet_get_wakeup_irq(pdev);
-
+       dev_info(&pdev->dev, "[FEC Debug] >> Start registering mii bus!\n");
        ret = fec_enet_mii_init(pdev);
+       dev_info(&pdev->dev, "[FEC Debug] >> Finish registering mii bus! ret:%d\n", ret);
        if (ret)
                goto failed_mii_init;

On the imx6ul platform, fec1 (ethernet@2188000) and fec2 (fec2: ethernet@20b4000) share
the same MDIO bus and external PHYs can only be configured by fec2. After applying the test
patch, the fec2 will be delayed to probe, the debug log shows as follows.

[    7.101569] fec 20b4000.ethernet: [FEC Debug] >> Start registering mii bus!
[    7.109386] fec 20b4000.ethernet: [FEC Debug] >> Finish registering mii bus! ret:-517
[    7.153045] fec 2188000.ethernet: [FEC Debug] >> Start registering mii bus!
[    7.343374] fec 2188000.ethernet: [FEC Debug] >> Finish registering mii bus! ret:0
[    8.742909] fec 20b4000.ethernet: [FEC Debug] >> Start registering mii bus!
[    8.769657] fec 20b4000.ethernet: [FEC Debug] >> Finish registering mii bus! ret:0

And the MDIO bus also can be accessed, please refer to the following log.

root@imx6ul7d:~# ./mdio eth0 1
read phy addr: 0x2  reg: 0x1   value : 0x786d

root@imx6ul7d:~# ./mdio eth1 1
read phy addr: 0x1  reg: 0x1   value : 0x786d

The only change is that after applying the test patch, the fec1 and fec2 exchange the ethernet
port names (before fec1: eth1 fec2: eth0, after fec1: eth0, fec2: eth1) because of the sequence
of probe. Of course, this is just a simulated situation on my side, maybe the actual situation on
your side is different from this which leads to different behaviors.

BTW, you'd better use the ./script/checkpatch.pl to check the patch before sending the patch.
There were some warnings and errors in the patch as follows.

WARNING: 'succesfully' may be misspelled - perhaps 'successfully'?
#14:
succesfully.
^^^^^^^^^^^

ERROR: do not initialise statics to false
#29: FILE: drivers/net/ethernet/freescale/fec_main.c:2287:
+       static bool wait_for_mdio_bus = false;

total: 1 errors, 1 warnings, 0 checks, 40 lines checked