On Wed, Sep 08, 2021 at 08:21:05PM -0700, Saravana Kannan wrote:

quoted

But with that fixed, it
still does not work. This is too late, the mdio busses have already
been registered and probed, the PHYs have been found on the busses,
and the PHYs would of been probed, if not for fw_devlink.

Sigh... looks like some drivers register their mdio bus in their
dsa_switch_ops->setup while others do it in their actual probe
function (which actually makes more sense to me).

If it makes more sense to you for of_mdiobus_register to be placed in
the probe function and not in ->setup, then please be aware of my
previous email pointing out that DSA defers probe due to other reasons
too before calling ->setup, like of_find_net_device_by_node not finding
the DSA master. If the MDIO bus was registered by then, it will be
destroyed by the unwind path and the device links will not be created a
second time, effectively defeating fw_devlink.

quoted

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 53f034fc2ef7..7ecd910f7fb8 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

@@ -525,6 +525,9 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
            NULL == bus->read || NULL == bus->write)
                return -EINVAL;

+       if (bus->parent && bus->parent->of_node)
+               bus->parent->of_node->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT;
+
        BUG_ON(bus->state != MDIOBUS_ALLOCATED &&
               bus->state != MDIOBUS_UNREGISTERED);

So basically saying all MDIO busses potentially have a problem.

I also don't like the name FWNODE_FLAG_BROKEN_PARENT. The parents are
not broken, they work fine, if fw_devlink gets out of the way and
allows them to do their job.

The parent assuming the child will be probed as soon as it's added is
a broken expectation/assumption. fw_devlink is just catching them
immediately.

It's not really a broken expectation when you come to think of the fact
that synchronous probing is requested, and this is the internal PHY of
the switch we are talking about, not just any PHY off the street, with
random dependencies. It is known beforehand, and so are the dependencies.
All dependencies that the internal PHY has should be provided by the
switch driver by the time it registers the MDIO bus.

The system is not prepared to handle an -EPROBE_DEFER simply because
there is no good reason why it should happen.

I see fw_devlink as injecting a fault in this case. Maybe we should
treat it, but in any case it is adding a pointless -EPROBE_DEFER when
the PHY driver could have probed immediately. This will slow down the
boot time when we treat it properly eventually.

Having said that, this is not the hill either of us should choose to
die on. So, how about something like:
FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD

If that works, I can clean up the series with this and the MDIO fix
you mentioned.

I'm okay with the "needs child bound on add" name.

quoted

You also asked about why the component framework is not used. DSA has
been around for a while, the first commit dates back to October
2008. Russell Kings first commit for the component framework is
January 2014. The plain driver model has worked for the last 13 years,
so there has not been any need to change.

Thanks for the history on why it couldn't have been used earlier.

In the long run, I'd still like to fix this so that the
dsa_tree_setup() doesn't need the flag above. I have some ideas using
device links that'll be much simpler to understand and maintain than
using the component framework. I'll send out patches for that (not
meant for 5.15) later and we can go with the MDIO bus hammer for 5.15.

Details?

I am not too fond of using the component framework because I am not
convinced we should be fabricating struct devices we do not need, just
to comply with an API that solves a fabricated problem.