Re: MDIO over I2C driver driver probe dependency issue

On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:

Thanks for the responses - I now have a more clear picture of what's going on.
 (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
be similar to latest kernels, but I could be wrong.)

Hi Brian

macb_main has had a lot of changes with respect to PHYs. Please try
something modern, like 5.10.

	  Andrew

On 1/8/2021 1:04 PM, Andrew Lunn wrote:

On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:

quoted

Thanks for the responses - I now have a more clear picture of what's going on.
 (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
be similar to latest kernels, but I could be wrong.)

Hi Brian

macb_main has had a lot of changes with respect to PHYs. Please try
something modern, like 5.10.

It does not seem to me like 5.10 will be much better, because we have
the following in PHYLINK:

int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
                             u32 flags)
...
          phy_dev = of_phy_find_device(phy_node);
          /* We're done with the phy_node handle */
          of_node_put(phy_node);
          if (!phy_dev)
                  return -ENODEV;

Given Brian's configuration we should be returning -EPROBE_DEFER here,
but doing that would likely break a number of systems that do expect
-ENODEV to be returned. However there may be hope with fw_devlink to
create an appropriate graph of probing orders and solve the
consumer/provider order generically.

Up until now we did not really have a situation like this one where the
MDIO/PHY subsystem depended upon an another one to be available. The
problem does exist, however it is not clear to me yet how to best solve it.
-- 
Florian

Thanks - I will have to keep a local patch to fix this for my needs
for the time being.

If in the future if I'm able to do something better (e.g. with a
better fw_devlink, which looks interesting), I'll post a patch.  But I
generally lag the latest kernels by a couple years, so...  not any
time soon.

<and hopefully this time my post doesn't bounce as I've turned off HTML>



On Fri, Jan 8, 2021 at 5:11 PM Florian Fainelli [off-list ref] wrote:

On 1/8/2021 1:04 PM, Andrew Lunn wrote:

quoted

On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:

quoted

Thanks for the responses - I now have a more clear picture of what's going on.
 (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
be similar to latest kernels, but I could be wrong.)

Hi Brian

macb_main has had a lot of changes with respect to PHYs. Please try
something modern, like 5.10.

It does not seem to me like 5.10 will be much better, because we have
the following in PHYLINK:

int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
                             u32 flags)
...
          phy_dev = of_phy_find_device(phy_node);
          /* We're done with the phy_node handle */
          of_node_put(phy_node);
          if (!phy_dev)
                  return -ENODEV;

Given Brian's configuration we should be returning -EPROBE_DEFER here,
but doing that would likely break a number of systems that do expect
-ENODEV to be returned. However there may be hope with fw_devlink to
create an appropriate graph of probing orders and solve the
consumer/provider order generically.

Up until now we did not really have a situation like this one where the
MDIO/PHY subsystem depended upon an another one to be available. The
problem does exist, however it is not clear to me yet how to best solve it.
--
Florian

On Fri, Jan 08, 2021 at 02:11:31PM -0800, Florian Fainelli wrote:

On 1/8/2021 1:04 PM, Andrew Lunn wrote:

quoted

On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:

quoted

Thanks for the responses - I now have a more clear picture of what's going on.
 (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
be similar to latest kernels, but I could be wrong.)

Hi Brian

macb_main has had a lot of changes with respect to PHYs. Please try
something modern, like 5.10.

It does not seem to me like 5.10 will be much better, because we have
the following in PHYLINK:

int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
                             u32 flags)
...
          phy_dev = of_phy_find_device(phy_node);
          /* We're done with the phy_node handle */
          of_node_put(phy_node);
          if (!phy_dev)
                  return -ENODEV;

Given Brian's configuration we should be returning -EPROBE_DEFER here,
but doing that would likely break a number of systems that do expect
-ENODEV to be returned. However there may be hope with fw_devlink to
create an appropriate graph of probing orders and solve the
consumer/provider order generically.

Up until now we did not really have a situation like this one where the
MDIO/PHY subsystem depended upon an another one to be available. The
problem does exist, however it is not clear to me yet how to best solve it.

Hi Florian

But we do have situations where the MDIO bus driver is independent of
the MAC driver. And mdio-i2c is not that different to mdio-gpio, where
we need a GPIO driver to load before mdio-gpio is usable. Are all this
also broken?

     Andrew

On Fri, Jan 08, 2021 at 02:11:31PM -0800, Florian Fainelli wrote:

On 1/8/2021 1:04 PM, Andrew Lunn wrote:

quoted

On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:

quoted

Thanks for the responses - I now have a more clear picture of what's going on.
 (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
be similar to latest kernels, but I could be wrong.)

Hi Brian

macb_main has had a lot of changes with respect to PHYs. Please try
something modern, like 5.10.

It does not seem to me like 5.10 will be much better, because we have
the following in PHYLINK:

int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
                             u32 flags)
...
          phy_dev = of_phy_find_device(phy_node);
          /* We're done with the phy_node handle */
          of_node_put(phy_node);
          if (!phy_dev)
                  return -ENODEV;

Given Brian's configuration we should be returning -EPROBE_DEFER here,
but doing that would likely break a number of systems that do expect
-ENODEV to be returned.

I just looked through the current users of phylink_of_phy_connect().
Most simply do a netdev_err() and return the error code to higher
levels. So apart from the spurious netdev_err() is -EPROBE_DEFER is
returned, they should do the right thing.

mvneta actually looks broken, it prints the error, but keeps going,
plays with WOL setings on the phy and device_set_wake() then returns
the error code.

macb is a bit more complex, but if i'm understanding it correctly, it
should handle -EPROBE_DEFER already, but you will get a spurious
netdev_err() for the -EPROBE_DEFER.

So i think we can fix this, and we should probably do it before there
are more users.

Brian, can you run a modern kernel to test patches, or do you need to
use the Xilinx fork?

    Andrew