[PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
on both Ethernet controllers. Currently device trees for such setups
have to make assumptions regarding the probe order of the controllers:

For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for
the PHYs of both fec1 and fec2. The reason is that fec2 has a lower
address than fec1 and is thus loaded first, so the bus is already
available when fec1 is probed.

Besides being confusing, this limitation also makes it impossible to use
the same device tree for variants of the i.MX6UL with one Ethernet
controller (which have to use the MDIO of fec1, as fec2 does not exist)
and variants with two controllers (which have to use fec2 because of the
load order).

To fix this, defer the probe of the Ethernet controller when the PHY is
not on our own MDIO bus and not available.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 47a6fc702ac7..dc070dd216e8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
 		goto failed_stop_mode;
 
 	phy_node = of_parse_phandle(np, "phy-handle", 0);
-	if (!phy_node && of_phy_is_fixed_link(np)) {
+	if (phy_node) {
+		struct device_node *mdio_parent =
+			of_get_next_parent(of_get_parent(phy_node));
+
+		ret = 0;
+
+		/* Skip PHY availability check for our own MDIO bus to avoid
+		 * cyclic dependency
+		 */
+		if (mdio_parent != np) {
+			struct phy_device *phy = of_phy_find_device(phy_node);
+
+			if (phy)
+				put_device(&phy->mdio.dev);
+			else
+				ret = -EPROBE_DEFER;
+		}
+
+		of_node_put(mdio_parent);
+		if (ret)
+			goto failed_phy;
+	} else if (of_phy_is_fixed_link(np)) {
 		ret = of_phy_register_fixed_link(np);
 		if (ret < 0) {
 			dev_err(&pdev->dev,
-- 
2.17.1

RE: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Joakim Zhang]

Hi Matthias,

> -----Original Message-----
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Sent: 2021年10月14日 19:31
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com>
> Subject: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not
> available
> 
> On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
> on both Ethernet controllers. Currently device trees for such setups have to
> make assumptions regarding the probe order of the controllers:
> 
> For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for the
> PHYs of both fec1 and fec2. The reason is that fec2 has a lower address than
> fec1 and is thus loaded first, so the bus is already available when fec1 is
> probed.

It's not correct, I think, we have board designed to use fec1(which is lower address) to controller MDIO interface, such as,
https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8qm-mek.dts?h=lf-5.10.y#n948
that means our driver can handle these cases, not related to the order.

> Besides being confusing, this limitation also makes it impossible to use the
> same device tree for variants of the i.MX6UL with one Ethernet controller
> (which have to use the MDIO of fec1, as fec2 does not exist) and variants with
> two controllers (which have to use fec2 because of the load order).

Generally speaking, you should only include imx6ul.dtsi for your board design to cover SoC definition,
and imx6ul-14x14-evk.dtsi/ imx6ul-14x14-evk.dts is for our 14x14 EVK board. So do we really need this
defer probe?

> 
> To fix this, defer the probe of the Ethernet controller when the PHY is not on
> our own MDIO bus and not available.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 47a6fc702ac7..dc070dd216e8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_stop_mode;
> 
>  	phy_node = of_parse_phandle(np, "phy-handle", 0);
> -	if (!phy_node && of_phy_is_fixed_link(np)) {
> +	if (phy_node) {
> +		struct device_node *mdio_parent =
> +			of_get_next_parent(of_get_parent(phy_node));
> +
> +		ret = 0;
> +
> +		/* Skip PHY availability check for our own MDIO bus to avoid
> +		 * cyclic dependency
> +		 */
> +		if (mdio_parent != np) {
> +			struct phy_device *phy = of_phy_find_device(phy_node);
> +
> +			if (phy)
> +				put_device(&phy->mdio.dev);
> +			else
> +				ret = -EPROBE_DEFER;
> +		}
> +
> +		of_node_put(mdio_parent);
> +		if (ret)
> +			goto failed_phy;
> +	} else if (of_phy_is_fixed_link(np)) {
>  		ret = of_phy_register_fixed_link(np);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev,
> --
> 2.17.1

Best Regards,
Joakim Zhang

RE: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On Mon, 2021-10-18 at 10:20 +0000, Joakim Zhang wrote:
> Hi Matthias,
> 
> > -----Original Message-----
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > Sent: 2021年10月14日 19:31
> > To: Joakim Zhang <qiangqing.zhang@nxp.com>; David S. Miller
> > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com>
> > Subject: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not
> > available
> > 
> > On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
> > on both Ethernet controllers. Currently device trees for such setups have to
> > make assumptions regarding the probe order of the controllers:
> > 
> > For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for the
> > PHYs of both fec1 and fec2. The reason is that fec2 has a lower address than
> > fec1 and is thus loaded first, so the bus is already available when fec1 is
> > probed.
> 
> It's not correct, I think, we have board designed to use fec1(which is lower address) to controller MDIO interface, such as,
> https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm64/boot/dts/freescale/imx8qm-mek.dts?h=lf-5.10.y#n948
> that means our driver can handle these cases, not related to the order.

Yes, not all SoC have FEC2 at the lower address, but this is the case
on i.MX6UL. As far as I can tell my patch should not hurt when the
order is already correct, as the added code will never retern
EPROBE_DEFER in these cases.

Obviously all existing Device Trees in the mainline kernel are defined
in a way that already works with the existing code, by using the FEC2
MDIO on i.MX6UL-based designs, or their Ethernet would not work
correctly.


> 
> > Besides being confusing, this limitation also makes it impossible to use the
> > same device tree for variants of the i.MX6UL with one Ethernet controller
> > (which have to use the MDIO of fec1, as fec2 does not exist) and variants with
> > two controllers (which have to use fec2 because of the load order).
> 
> Generally speaking, you should only include imx6ul.dtsi for your board design to cover SoC definition,
> and imx6ul-14x14-evk.dtsi/ imx6ul-14x14-evk.dts is for our 14x14 EVK board. So do we really need this
> defer probe?

I only mentioned imx6ul-14x14-evk.dtsi as an example. The issue affects
the TQ-Systems MBa6ULx board, which I'm currently preparing for
mainline submission.

The bootloader disables the non-existing FEC2 node on MCIMX6G1, but
when the MDIO of FEC2 is used for both interfacse, this will also break
FEC1, so a separate Device Tree is currently needed for the MCIMX6G1.
We would prefer to use the same Device Tree for both variants, which is
possible by using the MDIO of FEC1 and applying this patch.


> 
> > 
> > To fix this, defer the probe of the Ethernet controller when the PHY is not on
> > our own MDIO bus and not available.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 47a6fc702ac7..dc070dd216e8 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
> >  		goto failed_stop_mode;
> > 
> >  	phy_node = of_parse_phandle(np, "phy-handle", 0);
> > -	if (!phy_node && of_phy_is_fixed_link(np)) {
> > +	if (phy_node) {
> > +		struct device_node *mdio_parent =
> > +			of_get_next_parent(of_get_parent(phy_node));
> > +
> > +		ret = 0;
> > +
> > +		/* Skip PHY availability check for our own MDIO bus to avoid
> > +		 * cyclic dependency
> > +		 */
> > +		if (mdio_parent != np) {
> > +			struct phy_device *phy = of_phy_find_device(phy_node);
> > +
> > +			if (phy)
> > +				put_device(&phy->mdio.dev);
> > +			else
> > +				ret = -EPROBE_DEFER;
> > +		}
> > +
> > +		of_node_put(mdio_parent);
> > +		if (ret)
> > +			goto failed_phy;
> > +	} else if (of_phy_is_fixed_link(np)) {
> >  		ret = of_phy_register_fixed_link(np);
> >  		if (ret < 0) {
> >  			dev_err(&pdev->dev,
> > --
> > 2.17.1
> 
> Best Regards,
> Joakim Zhang

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Andrew Lunn]

On Thu, Oct 14, 2021 at 01:30:43PM +0200, Matthias Schiffer wrote:
> On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
> on both Ethernet controllers. Currently device trees for such setups
> have to make assumptions regarding the probe order of the controllers:
> 
> For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for
> the PHYs of both fec1 and fec2. The reason is that fec2 has a lower
> address than fec1 and is thus loaded first, so the bus is already
> available when fec1 is probed.
> 
> Besides being confusing, this limitation also makes it impossible to use
> the same device tree for variants of the i.MX6UL with one Ethernet
> controller (which have to use the MDIO of fec1, as fec2 does not exist)
> and variants with two controllers (which have to use fec2 because of the
> load order).
> 
> To fix this, defer the probe of the Ethernet controller when the PHY is
> not on our own MDIO bus and not available.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 47a6fc702ac7..dc070dd216e8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_stop_mode;
>  
>  	phy_node = of_parse_phandle(np, "phy-handle", 0);
> -	if (!phy_node && of_phy_is_fixed_link(np)) {
> +	if (phy_node) {
> +		struct device_node *mdio_parent =
> +			of_get_next_parent(of_get_parent(phy_node));
> +
> +		ret = 0;
> +
> +		/* Skip PHY availability check for our own MDIO bus to avoid
> +		 * cyclic dependency
> +		 */
> +		if (mdio_parent != np) {
> +			struct phy_device *phy = of_phy_find_device(phy_node);
> +
> +			if (phy)
> +				put_device(&phy->mdio.dev);
> +			else
> +				ret = -EPROBE_DEFER;
> +		}

I've not looked at the details yet, just back from vacation. But this
seems wrong. I would of expected phylib to of returned -EPRODE_DEFER
at some point, when asked for a PHY which does not exist yet. All the
driver should need to do is make sure it returns the
-EPRODE_DEFER.

       Andrew

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On Tue, 2021-10-19 at 16:12 +0200, Andrew Lunn wrote:
> On Thu, Oct 14, 2021 at 01:30:43PM +0200, Matthias Schiffer wrote:
> > On some SoCs like i.MX6UL it is common to use the same MDIO bus for PHYs
> > on both Ethernet controllers. Currently device trees for such setups
> > have to make assumptions regarding the probe order of the controllers:
> > 
> > For example in imx6ul-14x14-evk.dtsi, the MDIO bus of fec2 is used for
> > the PHYs of both fec1 and fec2. The reason is that fec2 has a lower
> > address than fec1 and is thus loaded first, so the bus is already
> > available when fec1 is probed.
> > 
> > Besides being confusing, this limitation also makes it impossible to use
> > the same device tree for variants of the i.MX6UL with one Ethernet
> > controller (which have to use the MDIO of fec1, as fec2 does not exist)
> > and variants with two controllers (which have to use fec2 because of the
> > load order).
> > 
> > To fix this, defer the probe of the Ethernet controller when the PHY is
> > not on our own MDIO bus and not available.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > index 47a6fc702ac7..dc070dd216e8 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3820,7 +3820,28 @@ fec_probe(struct platform_device *pdev)
> >  		goto failed_stop_mode;
> >  
> >  	phy_node = of_parse_phandle(np, "phy-handle", 0);
> > -	if (!phy_node && of_phy_is_fixed_link(np)) {
> > +	if (phy_node) {
> > +		struct device_node *mdio_parent =
> > +			of_get_next_parent(of_get_parent(phy_node));
> > +
> > +		ret = 0;
> > +
> > +		/* Skip PHY availability check for our own MDIO bus to avoid
> > +		 * cyclic dependency
> > +		 */
> > +		if (mdio_parent != np) {
> > +			struct phy_device *phy = of_phy_find_device(phy_node);
> > +
> > +			if (phy)
> > +				put_device(&phy->mdio.dev);
> > +			else
> > +				ret = -EPROBE_DEFER;
> > +		}
> 
> I've not looked at the details yet, just back from vacation. But this
> seems wrong. I would of expected phylib to of returned -EPRODE_DEFER
> at some point, when asked for a PHY which does not exist yet. All the
> driver should need to do is make sure it returns the
> -EPRODE_DEFER.

This is what I expected as well, however there are a few complications:

- At the moment the first time the driver does anything with the PHY is
  in fec_enet_open(), not in fec_probe() - way too late to defer
  anything

- phylib doesn't know about EPROBE_DEFER, or error returns in general,
  everything just returns NULL. There is a fairly long chain of
  functions that just return NULL here (which might be okay, as they
  don't have a way to distinguish different errors anyways AFAICT):
  of_phy_find_device() -> fwnode_phy_find_device() ->
  fwnode_phy_find_device() -> fwnode_mdio_find_device() ->
  bus_find_device_by_fwnode() -> bus_find_device()

- Even if we implement the EPROBE_DEFER return somewhere in phylib,
  there needs to be special handling for the internal MDIO case, where
  the MDIO device is provided by the same driver that uses it. We can't
  wait with the check until we registered the MDIO bus, as it is not
  allowed to return EPROBE_DEFER after any devices have been
  registered. Splitting out the MDIO bus to be probed separately does
  not seem feasible, but I might be wrong?


So I have a few ideas, but I'm not sure which approach to pursue:

1. Make of_phy_find_device() return -EPROBE_DEFER (with or without
   touching more of the call chain). Doesn't seem too convincing to me,
   as it will just replace every case where of_phy_find_device()
   return NULL with -EPROBE_DEFER, making it more complicated to use
   for no gain.

2. Create a helper in phylib ("of_phy_device_available()") or something
   that encapsulates some of the code of this patch in a reuseable way,
   returning 0 or -EPROBE_DEFER.

 2a. Move just the code in "if (mdio_parent != np) {"
 2b. Also include the check for the MDIO parent for special handling of
     the internal MDIO. Not sure if this is approach is too specific
     to the node structure for a generic helper, or if the structure is
     common enough across different drivers.

3. Create a wrapper for of_parse_phandle() in phylib that does
   everything from 2b. 
  - Change the driver to hold a reference to a phy_device instead of
    its device_node
  - Might require further work for the fixed-link case
  - Will allow for an API similar to regulator_get[_optional]()
  - I have no idea how to solve the internal MDIO case with this
    approach nicely, as we don't be able to get a phy_device before the
    MDIO bus is registered


Matthias



> 
>        Andrew

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Andrew Lunn]

> > I've not looked at the details yet, just back from vacation. But this
> > seems wrong. I would of expected phylib to of returned -EPRODE_DEFER
> > at some point, when asked for a PHY which does not exist yet. All the
> > driver should need to do is make sure it returns the
> > -EPRODE_DEFER.
> 
> This is what I expected as well, however there are a few complications:
> 
> - At the moment the first time the driver does anything with the PHY is
>   in fec_enet_open(), not in fec_probe() - way too late to defer
>   anything

O.K. Right. Are you using NFS root? For normal user space opening of
the interface, this has all been sorted out by the time user space
does anything. The NFS root changes the time in a big way.

Anyway, i would say some bits of code need moving from open to probe
so EPROBE_DEFER can be used.

We already have:

        phy_node = of_parse_phandle(np, "phy-handle", 0);
        if (!phy_node && of_phy_is_fixed_link(np)) {
                ret = of_phy_register_fixed_link(np);
                if (ret < 0) {
                        dev_err(&pdev->dev,
                                "broken fixed-link specification\n");
                        goto failed_phy;
                }
                phy_node = of_node_get(np);
        }
        fep->phy_node = phy_node;

Go one step further. If fep->phy_node is not NULL, we know there
should be a PHY. So call of_phy_find_device(). If it returns NULL,
then -EPROBE_DEFER. Otherwise store the phydev into fep, and use it in
open.

You will need to move the call to fec_enet_mii_init(pdev) earlier, so
the MDIO bus is available.

    Andrew

Re: (EXT) Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On Wed, 2021-10-20 at 20:50 +0200, Andrew Lunn wrote:
> > > I've not looked at the details yet, just back from vacation. But this
> > > seems wrong. I would of expected phylib to of returned -EPRODE_DEFER
> > > at some point, when asked for a PHY which does not exist yet. All the
> > > driver should need to do is make sure it returns the
> > > -EPRODE_DEFER.
> > 
> > This is what I expected as well, however there are a few complications:
> > 
> > - At the moment the first time the driver does anything with the PHY is
> >   in fec_enet_open(), not in fec_probe() - way too late to defer
> >   anything
> 
> O.K. Right. Are you using NFS root? For normal user space opening of
> the interface, this has all been sorted out by the time user space
> does anything. The NFS root changes the time in a big way.

NFS root is one of our usecases.

> 
> Anyway, i would say some bits of code need moving from open to probe
> so EPROBE_DEFER can be used.
> 
> We already have:
> 
>         phy_node = of_parse_phandle(np, "phy-handle", 0);
>         if (!phy_node && of_phy_is_fixed_link(np)) {
>                 ret = of_phy_register_fixed_link(np);
>                 if (ret < 0) {
>                         dev_err(&pdev->dev,
>                                 "broken fixed-link specification\n");
>                         goto failed_phy;
>                 }
>                 phy_node = of_node_get(np);
>         }
>         fep->phy_node = phy_node;
> 
> Go one step further. If fep->phy_node is not NULL, we know there
> should be a PHY. So call of_phy_find_device(). If it returns NULL,
> then -EPROBE_DEFER. Otherwise store the phydev into fep, and use it in
> open.
> 
> You will need to move the call to fec_enet_mii_init(pdev) earlier, so
> the MDIO bus is available.

I would love to do this, but driver-api/driver-model/driver.rst
contains the following warning:

      -EPROBE_DEFER must not be returned if probe() has already created
      child devices, even if those child devices are removed again
      in a cleanup path. If -EPROBE_DEFER is returned after a child
      device has been registered, it may result in an infinite loop of
      .probe() calls to the same driver.

My understanding of this is that there is simply no way to return
-EPROBE_DEFER after fec_enet_mii_init(pdev).



> 
>     Andrew

Re: (EXT) Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Andrew Lunn]

> I would love to do this, but driver-api/driver-model/driver.rst
> contains the following warning:
> 
>       -EPROBE_DEFER must not be returned if probe() has already created
>       child devices, even if those child devices are removed again
>       in a cleanup path. If -EPROBE_DEFER is returned after a child
>       device has been registered, it may result in an infinite loop of
>       .probe() calls to the same driver.
> 
> My understanding of this is that there is simply no way to return
> -EPROBE_DEFER after fec_enet_mii_init(pdev).

It might say that, but lots of network drivers actually do this. I've
not seen an endless loop.

    Andrew

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On Thu, 2021-10-21 at 14:50 +0200, Andrew Lunn wrote:
> > I would love to do this, but driver-api/driver-model/driver.rst
> > contains the following warning:
> > 
> >       -EPROBE_DEFER must not be returned if probe() has already created
> >       child devices, even if those child devices are removed again
> >       in a cleanup path. If -EPROBE_DEFER is returned after a child
> >       device has been registered, it may result in an infinite loop of
> >       .probe() calls to the same driver.
> > 
> > My understanding of this is that there is simply no way to return
> > -EPROBE_DEFER after fec_enet_mii_init(pdev).
> 
> It might say that, but lots of network drivers actually do this. I've
> not seen an endless loop.
> 
>     Andrew


Hmm, lots of network drivers? I tried to find an example, but all
drivers that generate -EPROBE_DEFER for missing PHYs at all don't have
an internal MDIO bus and thus avoid the circular dependency.

Matthias

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Andrew Lunn]

> Hmm, lots of network drivers? I tried to find an example, but all
> drivers that generate -EPROBE_DEFER for missing PHYs at all don't have
> an internal MDIO bus and thus avoid the circular dependency.

Try drivers/net/dsa.

These often have mdio busses which get registered and then
unregistered. There are also IRQ drivers which do the same.

	Andrew

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On Fri, 2021-10-22 at 14:56 +0200, Andrew Lunn wrote:
> > Hmm, lots of network drivers? I tried to find an example, but all
> > drivers that generate -EPROBE_DEFER for missing PHYs at all don't have
> > an internal MDIO bus and thus avoid the circular dependency.
> 
> Try drivers/net/dsa.
> 
> These often have mdio busses which get registered and then
> unregistered. There are also IRQ drivers which do the same.
> 
> 	Andrew


All drivers I looked at were careful to register the MDIO bus in the
last part of the probe function, so that the only errors that could 
happen after that (and thus require to unregister the bus device again)
would not be -EPROBE_DEFER.

The documented infinite loop is easy to reproduce: You just need two
separate device instances with misbehaving probe() functions that
return -EPROBE_DEFER after registering and unregistering some
subdevice. If the missing device that causes the deferral never appears
(for example because its driver is not available), the two devices will
be probed ad infinitum.

I agree with the documentation that a driver should not do this, and
thus we need another way to deal with the cyclic dependency between an
Ethernet interface and a PHY on its internal MDIO bus.

While I think that a generic solution would be theoretically possible
by ensuring that the probing loop makes some kind of "progress", I
think this would set a precedent for "expensive" operations happening
before a -EPROBE_DEFER return, which should be avoided even when no
infinite loop results.

Matthias

Re: [PATCH] net: fec: defer probe if PHY on external MDIO bus is not available

[Matthias Schiffer]

On Tue, 2021-10-26 at 13:54 +0200, Matthias Schiffer wrote:
> On Fri, 2021-10-22 at 14:56 +0200, Andrew Lunn wrote:
> > > Hmm, lots of network drivers? I tried to find an example, but all
> > > drivers that generate -EPROBE_DEFER for missing PHYs at all don't have
> > > an internal MDIO bus and thus avoid the circular dependency.
> > 
> > Try drivers/net/dsa.
> > 
> > These often have mdio busses which get registered and then
> > unregistered. There are also IRQ drivers which do the same.
> > 
> > 	Andrew
> 
> 
> All drivers I looked at were careful to register the MDIO bus in the
> last part of the probe function, so that the only errors that could 
> happen after that (and thus require to unregister the bus device again)
> would not be -EPROBE_DEFER.
> 
> The documented infinite loop is easy to reproduce: You just need two
> separate device instances with misbehaving probe() functions that
> return -EPROBE_DEFER after registering and unregistering some
> subdevice. If the missing device that causes the deferral never appears
> (for example because its driver is not available), the two devices will
> be probed ad infinitum.
> 
> I agree with the documentation that a driver should not do this, and
> thus we need another way to deal with the cyclic dependency between an
> Ethernet interface and a PHY on its internal MDIO bus.
> 
> While I think that a generic solution would be theoretically possible
> by ensuring that the probing loop makes some kind of "progress", I
> think this would set a precedent for "expensive" operations happening
> before a -EPROBE_DEFER return, which should be avoided even when no
> infinite loop results.
> 
> Matthias

Does anyone have a suggestion how to proceed with this issue?