netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] net: fec: manage corner deferred probe condition
@ 2023-01-15 21:38 Pierluigi Passaro
  2023-01-15 21:56 ` Andrew Lunn
  2023-01-17 12:55 ` Paolo Abeni
  0 siblings, 2 replies; 12+ messages in thread
From: Pierluigi Passaro @ 2023-01-15 21:38 UTC (permalink / raw)
  To: andrew, wei.fang, shenwei.wang, xiaoning.wang, linux-imx, davem,
	edumazet, kuba, pabeni, netdev, linux-kernel
  Cc: eran.m, nate.d, francesco.f, pierluigi.p, pierluigi.passaro

For dual fec interfaces, external phys can only be configured by fec0.
When the function of_mdiobus_register return -EPROBE_DEFER, the driver
is lately called to manage fec1, which wrongly register its mii_bus as
fec0_mii_bus.
When fec0 retry the probe, the previous assignement prevent the MDIO bus
registration.
Use a static boolean to trace the orginal MDIO bus deferred probe and
prevent further registrations until the fec0 registration completed
succesfully.

Signed-off-by: Pierluigi Passaro <pierluigi.p@variscite.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 644f3c963730..b4ca3bd4283f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2284,6 +2284,18 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	int err = -ENXIO;
 	u32 mii_speed, holdtime;
 	u32 bus_freq;
+	static bool wait_for_mdio_bus = false;
+
+	bus_freq = 2500000; /* 2.5MHz by default */
+	node = of_get_child_by_name(pdev->dev.of_node, "mdio");
+	if (node) {
+		wait_for_mdio_bus = false;
+		of_property_read_u32(node, "clock-frequency", &bus_freq);
+		suppress_preamble = of_property_read_bool(node,
+							  "suppress-preamble");
+	}
+	if (wait_for_mdio_bus)
+		return -EPROBE_DEFER;
 
 	/*
 	 * The i.MX28 dual fec interfaces are not equal.
@@ -2311,14 +2323,6 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	bus_freq = 2500000; /* 2.5MHz by default */
-	node = of_get_child_by_name(pdev->dev.of_node, "mdio");
-	if (node) {
-		of_property_read_u32(node, "clock-frequency", &bus_freq);
-		suppress_preamble = of_property_read_bool(node,
-							  "suppress-preamble");
-	}
-
 	/*
 	 * Set MII speed (= clk_get_rate() / 2 * phy_speed)
 	 *
@@ -2389,6 +2393,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	fep->mii_bus->parent = &pdev->dev;
 
 	err = of_mdiobus_register(fep->mii_bus, node);
+	if (err == -EPROBE_DEFER)
+		wait_for_mdio_bus = true;
 	if (err)
 		goto err_out_free_mdiobus;
 	of_node_put(node);
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-15 21:38 [PATCH v2] net: fec: manage corner deferred probe condition Pierluigi Passaro
@ 2023-01-15 21:56 ` Andrew Lunn
  2023-01-15 22:23   ` Pierluigi Passaro
  2023-01-17 12:55 ` Paolo Abeni
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2023-01-15 21:56 UTC (permalink / raw)
  To: Pierluigi Passaro
  Cc: wei.fang, shenwei.wang, xiaoning.wang, linux-imx, davem,
	edumazet, kuba, pabeni, netdev, linux-kernel, eran.m, nate.d,
	francesco.f, pierluigi.passaro

On Sun, Jan 15, 2023 at 10:38:04PM +0100, Pierluigi Passaro wrote:
> For dual fec interfaces, external phys can only be configured by fec0.
> When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> is lately called to manage fec1, which wrongly register its mii_bus as
> fec0_mii_bus.
> When fec0 retry the probe, the previous assignement prevent the MDIO bus
> registration.
> Use a static boolean to trace the orginal MDIO bus deferred probe and
> prevent further registrations until the fec0 registration completed
> succesfully.

The real problem here seems to be that fep->dev_id is not
deterministic. I think a better fix would be to make the mdio bus name
deterministic. Use pdev->id instead of fep->dev_id + 1. That is what
most mdiobus drivers use.

	Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-15 21:56 ` Andrew Lunn
@ 2023-01-15 22:23   ` Pierluigi Passaro
  2023-01-16  0:01     ` Andrew Lunn
  2023-01-16  7:35     ` Alexander Stein
  0 siblings, 2 replies; 12+ messages in thread
From: Pierluigi Passaro @ 2023-01-15 22:23 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pierluigi Passaro, wei.fang, shenwei.wang, xiaoning.wang,
	linux-imx, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	eran.m, nate.d, francesco.f

On Sun, Jan 15, 2023 at 10:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Jan 15, 2023 at 10:38:04PM +0100, Pierluigi Passaro wrote:
> > For dual fec interfaces, external phys can only be configured by fec0.
> > When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> > is lately called to manage fec1, which wrongly register its mii_bus as
> > fec0_mii_bus.
> > When fec0 retry the probe, the previous assignement prevent the MDIO bus
> > registration.
> > Use a static boolean to trace the orginal MDIO bus deferred probe and
> > prevent further registrations until the fec0 registration completed
> > succesfully.
>
> The real problem here seems to be that fep->dev_id is not
> deterministic. I think a better fix would be to make the mdio bus name
> deterministic. Use pdev->id instead of fep->dev_id + 1. That is what
> most mdiobus drivers use.
>
Actually, the sequence is deterministic, fec0 and then fec1,
but sometimes the GPIO of fec0 is not yet available.
The EPROBE_DEFER does not prevent the second instance from being probed.
This is the origin of the problem.
>
>         Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-15 22:23   ` Pierluigi Passaro
@ 2023-01-16  0:01     ` Andrew Lunn
  2023-01-16  8:51       ` Pierluigi Passaro
  2023-01-16  7:35     ` Alexander Stein
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2023-01-16  0:01 UTC (permalink / raw)
  To: Pierluigi Passaro
  Cc: Pierluigi Passaro, wei.fang, shenwei.wang, xiaoning.wang,
	linux-imx, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	eran.m, nate.d, francesco.f

On Sun, Jan 15, 2023 at 11:23:51PM +0100, Pierluigi Passaro wrote:
> On Sun, Jan 15, 2023 at 10:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Sun, Jan 15, 2023 at 10:38:04PM +0100, Pierluigi Passaro wrote:
> > > For dual fec interfaces, external phys can only be configured by fec0.
> > > When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> > > is lately called to manage fec1, which wrongly register its mii_bus as
> > > fec0_mii_bus.
> > > When fec0 retry the probe, the previous assignement prevent the MDIO bus
> > > registration.
> > > Use a static boolean to trace the orginal MDIO bus deferred probe and
> > > prevent further registrations until the fec0 registration completed
> > > succesfully.
> >
> > The real problem here seems to be that fep->dev_id is not
> > deterministic. I think a better fix would be to make the mdio bus name
> > deterministic. Use pdev->id instead of fep->dev_id + 1. That is what
> > most mdiobus drivers use.
> >
> Actually, the sequence is deterministic, fec0 and then fec1,
> but sometimes the GPIO of fec0 is not yet available.
> The EPROBE_DEFER does not prevent the second instance from being probed.
> This is the origin of the problem.

Maybe i understood you wrongly, but it sounds like the second instance
takes the name space of the first? And when the first probes for the
second time, the name space is taken and the registration fails? To
me, this is indeterminate behaviour, the name fec0_mii_bus is not
determinate.

	Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-15 22:23   ` Pierluigi Passaro
  2023-01-16  0:01     ` Andrew Lunn
@ 2023-01-16  7:35     ` Alexander Stein
  2023-01-16 19:19       ` Pierluigi Passaro
  1 sibling, 1 reply; 12+ messages in thread
From: Alexander Stein @ 2023-01-16  7:35 UTC (permalink / raw)
  To: Andrew Lunn, Pierluigi Passaro
  Cc: Pierluigi Passaro, wei.fang, shenwei.wang, xiaoning.wang,
	linux-imx, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	eran.m, nate.d, francesco.f

Hi,

Am Sonntag, 15. Januar 2023, 23:23:51 CET schrieb Pierluigi Passaro:
> On Sun, Jan 15, 2023 at 10:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Sun, Jan 15, 2023 at 10:38:04PM +0100, Pierluigi Passaro wrote:
> > > For dual fec interfaces, external phys can only be configured by fec0.
> > > When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> > > is lately called to manage fec1, which wrongly register its mii_bus as
> > > fec0_mii_bus.
> > > When fec0 retry the probe, the previous assignement prevent the MDIO bus
> > > registration.
> > > Use a static boolean to trace the orginal MDIO bus deferred probe and
> > > prevent further registrations until the fec0 registration completed
> > > succesfully.
> > 
> > The real problem here seems to be that fep->dev_id is not
> > deterministic. I think a better fix would be to make the mdio bus name
> > deterministic. Use pdev->id instead of fep->dev_id + 1. That is what
> > most mdiobus drivers use.
> 
> Actually, the sequence is deterministic, fec0 and then fec1,
> but sometimes the GPIO of fec0 is not yet available.

Not in every case though. On i.MX6UL has the following memory map for FEC:
* FEC2: 0x020b4000
* FEC1: 0x02188000

Which essentially means that fec2 will be probed first.

> The EPROBE_DEFER does not prevent the second instance from being probed.
> This is the origin of the problem.

Is this the actual cause? There is also a problem in the case above if the 
MDIO controlling interface (fec2) is not probed first, e.g. using fec1 for 
MDIO access. But then again there is i.MX6ULG1 which only has fec1 
interface...

Best regards,
Alexander




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-16  0:01     ` Andrew Lunn
@ 2023-01-16  8:51       ` Pierluigi Passaro
  2023-01-16 15:32         ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Pierluigi Passaro @ 2023-01-16  8:51 UTC (permalink / raw)
  To: Andrew Lunn, Pierluigi Passaro
  Cc: wei.fang, shenwei.wang, xiaoning.wang, linux-imx, davem,
	edumazet, kuba, pabeni, netdev, linux-kernel, Eran Matityahu,
	Nate Drude, Francesco Ferraro

On Mon, Jan 16, 2023 at 1:01 AM Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Jan 15, 2023 at 11:23:51PM +0100, Pierluigi Passaro wrote:
> > On Sun, Jan 15, 2023 at 10:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Sun, Jan 15, 2023 at 10:38:04PM +0100, Pierluigi Passaro wrote:
> > > > For dual fec interfaces, external phys can only be configured by fec0.
> > > > When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> > > > is lately called to manage fec1, which wrongly register its mii_bus as
> > > > fec0_mii_bus.
> > > > When fec0 retry the probe, the previous assignement prevent the MDIO bus
> > > > registration.
> > > > Use a static boolean to trace the orginal MDIO bus deferred probe and
> > > > prevent further registrations until the fec0 registration completed
> > > > succesfully.
> > >
> > > The real problem here seems to be that fep->dev_id is not
> > > deterministic. I think a better fix would be to make the mdio bus name
> > > deterministic. Use pdev->id instead of fep->dev_id + 1. That is what
> > > most mdiobus drivers use.
> > >
> > Actually, the sequence is deterministic, fec0 and then fec1,
> > but sometimes the GPIO of fec0 is not yet available.
> > The EPROBE_DEFER does not prevent the second instance from being probed.
> > This is the origin of the problem.
>
> Maybe I understood you wrongly, but it sounds like the second instance
> takes the namespace of the first? And when the first probes for the
> second time, the name space is taken and the registration fails? To
> me, this is indeterminate behaviour, the name fec0_mii_bus is not
> determinate.
>
>         Andrew
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.
- FEC0 is successfully probed, but MDIO bus is now assigned to FEC1 and cannot  and no communication takes place

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-16  8:51       ` Pierluigi Passaro
@ 2023-01-16 15:32         ` Andrew Lunn
  2023-01-16 20:23           ` Pierluigi Passaro
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2023-01-16 15:32 UTC (permalink / raw)
  To: Pierluigi Passaro
  Cc: Pierluigi Passaro, wei.fang, shenwei.wang, xiaoning.wang,
	linux-imx, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	Eran Matityahu, Nate Drude, Francesco Ferraro

> 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.

> - FEC0 is successfully probed, but MDIO bus is now assigned to FEC1
>   and cannot  and no communication takes place

       Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-16  7:35     ` Alexander Stein
@ 2023-01-16 19:19       ` Pierluigi Passaro
  0 siblings, 0 replies; 12+ messages in thread
From: Pierluigi Passaro @ 2023-01-16 19:19 UTC (permalink / raw)
  To: Alexander Stein, Andrew Lunn, Pierluigi Passaro
  Cc: wei.fang, shenwei.wang, xiaoning.wang, linux-imx, davem,
	edumazet, kuba, pabeni, netdev, linux-kernel, Eran Matityahu,
	Nate Drude, Francesco Ferraro

On Mon, Jan 16, 2023 at 8:35 AM Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
> Hi,
>
> Am Sonntag, 15. Januar 2023, 23:23:51 CET schrieb Pierluigi Passaro:
> > On Sun, Jan 15, 2023 at 10:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Sun, Jan 15, 2023 at 10:38:04PM +0100, Pierluigi Passaro wrote:
> > > > For dual fec interfaces, external phys can only be configured by fec0.
> > > > When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> > > > is lately called to manage fec1, which wrongly register its mii_bus as
> > > > fec0_mii_bus.
> > > > When fec0 retry the probe, the previous assignement prevent the MDIO bus
> > > > registration.
> > > > Use a static boolean to trace the orginal MDIO bus deferred probe and
> > > > prevent further registrations until the fec0 registration completed
> > > > succesfully.
> > >
> > > The real problem here seems to be that fep->dev_id is not
> > > deterministic. I think a better fix would be to make the mdio bus name
> > > deterministic. Use pdev->id instead of fep->dev_id + 1. That is what
> > > most mdiobus drivers use.
> >
> > Actually, the sequence is deterministic, fec0 and then fec1,
> > but sometimes the GPIO of fec0 is not yet available.
>
> Not in every case though. On i.MX6UL has the following memory map for FEC:
> * FEC2: 0x020b4000
> * FEC1: 0x02188000
>
> Which essentially means that fec2 will be probed first.
>
This is actually the expected behaviour, by FEC0 I refer to the 1st instance of
FEC, no matter the alias used for it: apologizing for the misleading notation.
For iMX6UL, when both the FEC are present, the MDIO is owned by
fec@0x020b4000, which is the 1st instance of FEC.
>
> > The EPROBE_DEFER does not prevent the second instance from being probed.
> > This is the origin of the problem.
>
> Is this the actual cause? There is also a problem in the case above if the
> MDIO controlling interface (fec2) is not probed first, e.g. using fec1 for
> MDIO access. But then again there is i.MX6ULG1 which only has fec1
> interface...
>
I'm not familiar with iMX6ULG1, but I would expect that its device tree disables
one of the 2 fec: this patch is relevant only for dual FEC configuration.
>
> Best regards,
> Alexander

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-16 15:32         ` Andrew Lunn
@ 2023-01-16 20:23           ` Pierluigi Passaro
  2023-01-17  5:54             ` Wei Fang
  0 siblings, 1 reply; 12+ messages in thread
From: Pierluigi Passaro @ 2023-01-16 20:23 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Pierluigi Passaro, wei.fang, shenwei.wang, xiaoning.wang,
	linux-imx, davem, edumazet, kuba, pabeni, netdev, linux-kernel,
	Eran Matityahu, Nate Drude, Francesco Ferraro

On Mon, Jan 16, 2023 at 4:32 PM Andrew Lunn <andrew@lunn.ch> 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
>
>        Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-16 20:23           ` Pierluigi Passaro
@ 2023-01-17  5:54             ` Wei Fang
  2023-01-17 14:17               ` Andrew Lunn
  0 siblings, 1 reply; 12+ messages in thread
From: Wei Fang @ 2023-01-17  5:54 UTC (permalink / raw)
  To: Pierluigi Passaro, Andrew Lunn
  Cc: Pierluigi Passaro, Shenwei Wang, Clark Wang, dl-linux-imx, davem,
	edumazet, kuba, pabeni, netdev, linux-kernel, eran.m, Nate Drude,
	Francesco Ferraro

> -----Original Message-----
> From: Pierluigi Passaro <pierluigi.p@variscite.com>
> Sent: 2023年1月17日 4:23
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Pierluigi Passaro <pierluigi.passaro@gmail.com>; Wei Fang
> <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
> <xiaoning.wang@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> eran.m@variscite.com; Nate Drude <Nate.D@variscite.com>; Francesco
> Ferraro <francesco.f@variscite.com>
> Subject: Re: [PATCH v2] net: fec: manage corner deferred probe condition
> 
> On Mon, Jan 16, 2023 at 4:32 PM Andrew Lunn <andrew@lunn.ch> 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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-15 21:38 [PATCH v2] net: fec: manage corner deferred probe condition Pierluigi Passaro
  2023-01-15 21:56 ` Andrew Lunn
@ 2023-01-17 12:55 ` Paolo Abeni
  1 sibling, 0 replies; 12+ messages in thread
From: Paolo Abeni @ 2023-01-17 12:55 UTC (permalink / raw)
  To: Pierluigi Passaro, andrew, wei.fang, shenwei.wang, xiaoning.wang,
	linux-imx, davem, edumazet, kuba, netdev, linux-kernel
  Cc: eran.m, nate.d, francesco.f, pierluigi.passaro

Hello,

On Sun, 2023-01-15 at 22:38 +0100, Pierluigi Passaro wrote:
> For dual fec interfaces, external phys can only be configured by fec0.
> When the function of_mdiobus_register return -EPROBE_DEFER, the driver
> is lately called to manage fec1, which wrongly register its mii_bus as
> fec0_mii_bus.
> When fec0 retry the probe, the previous assignement prevent the MDIO bus
> registration.
> Use a static boolean to trace the orginal MDIO bus deferred probe and
> prevent further registrations until the fec0 registration completed
> succesfully.
> 
> Signed-off-by: Pierluigi Passaro <pierluigi.p@variscite.com>

Here there are a few formal/process-related mistakes:
- wait at least 24h before posting a new version
- please include the target tree name into the subj prefix

this looks like a fix, so it should possibly target net and include a
'Fixes' tag into the SoB area.

Please be sure to read and follow the process documentation and the
netdev specifics bit from the current kernel source.

Thanks,

Paolo


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] net: fec: manage corner deferred probe condition
  2023-01-17  5:54             ` Wei Fang
@ 2023-01-17 14:17               ` Andrew Lunn
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Lunn @ 2023-01-17 14:17 UTC (permalink / raw)
  To: Wei Fang
  Cc: Pierluigi Passaro, Pierluigi Passaro, Shenwei Wang, Clark Wang,
	dl-linux-imx, davem, edumazet, kuba, pabeni, netdev,
	linux-kernel, eran.m, Nate Drude, Francesco Ferraro

On Tue, Jan 17, 2023 at 05:54:21AM +0000, Wei Fang wrote:
> > -----Original Message-----
> > From: Pierluigi Passaro <pierluigi.p@variscite.com>
> > Sent: 2023年1月17日 4:23
> > To: Andrew Lunn <andrew@lunn.ch>
> > Cc: Pierluigi Passaro <pierluigi.passaro@gmail.com>; Wei Fang
> > <wei.fang@nxp.com>; Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang
> > <xiaoning.wang@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> > eran.m@variscite.com; Nate Drude <Nate.D@variscite.com>; Francesco
> > Ferraro <francesco.f@variscite.com>
> > Subject: Re: [PATCH v2] net: fec: manage corner deferred probe condition
> > 
> > On Mon, Jan 16, 2023 at 4:32 PM Andrew Lunn <andrew@lunn.ch> 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.

Hi Wei

Reading the emails from Pierluigi, i don't get the feeling he really
understands the problem and has got to the root cause. I've not seen a
really good, detailed explanation of what is going wrong.

       Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-01-17 14:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-15 21:38 [PATCH v2] net: fec: manage corner deferred probe condition Pierluigi Passaro
2023-01-15 21:56 ` Andrew Lunn
2023-01-15 22:23   ` Pierluigi Passaro
2023-01-16  0:01     ` Andrew Lunn
2023-01-16  8:51       ` Pierluigi Passaro
2023-01-16 15:32         ` Andrew Lunn
2023-01-16 20:23           ` Pierluigi Passaro
2023-01-17  5:54             ` Wei Fang
2023-01-17 14:17               ` Andrew Lunn
2023-01-16  7:35     ` Alexander Stein
2023-01-16 19:19       ` Pierluigi Passaro
2023-01-17 12:55 ` Paolo Abeni

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).