Re: [PATCH RFT] of: property: fw_devlink: Add support for the "phy-handle" binding

[Saravana Kannan]

On Wed, Oct 2, 2024 at 12:34 PM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote:
>
> On Tue, Oct 01, 2024 at 02:22:23PM GMT, Andrew Halaney wrote:
> > On Mon, Sep 30, 2024 at 05:12:42PM GMT, Saravana Kannan wrote:
> > > On Mon, Sep 30, 2024 at 2:28 PM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote:
> > > >
> > > > Add support for parsing the phy-handle binding so that fw_devlink can
> > > > enforce the dependency. This prevents MACs (that use this binding to
> > > > claim they're using the corresponding phy) from probing prior to the
> > > > phy, unless the phy is a child of the MAC (which results in a
> > > > dependency cycle) or similar.
> > > >
> > > > For some motivation, imagine a device topology like so:
> > > >
> > > >     &ethernet0 {
> > > >             phy-mode = "sgmii";
> > > >             phy-handle = <&sgmii_phy0>;
> > > >
> > > >             mdio {
> > > >                     compatible = "snps,dwmac-mdio";
> > > >                     sgmii_phy0: phy@8 {
> > > >                             compatible = "ethernet-phy-id0141.0dd4";
> > > >                             reg = <0x8>;
> > > >                             device_type = "ethernet-phy";
> > > >                     };
> > > >
> > > >                     sgmii_phy1: phy@a {
> > > >                             compatible = "ethernet-phy-id0141.0dd4";
> > > >                             reg = <0xa>;
> > > >                             device_type = "ethernet-phy";
> > > >                     };
> > > >             };
> > > >     };
> > > >
> > > >     &ethernet1 {
> > > >             phy-mode = "sgmii";
> > > >             phy-handle = <&sgmii_phy1>;
> > > >     };
> > > >
> > > > Here ethernet1 depends on sgmii_phy1 to function properly. In the below
> > > > link an issue is reported where ethernet1 is probed and used prior to
> > > > sgmii_phy1, resulting in a failure to get things working for ethernet1.
> > > > With this change in place ethernet1 doesn't probe until sgmii_phy1 is
> > > > ready, resulting in ethernet1 functioning properly.
> > > >
> > > > ethernet0 consumes sgmii_phy0, but this dependency isn't enforced
> > > > via the device_links backing fw_devlink since ethernet0 is the parent of
> > > > sgmii_phy0. Here's a log showing that in action:
> > > >
> > > >     [    7.000432] qcom-ethqos 23040000.ethernet: Fixed dependency cycle(s) with /soc@0/ethernet@23040000/mdio/phy@8
> > > >
> > > > With this change in place ethernet1's dependency is properly described,
> > > > and it doesn't probe prior to sgmii_phy1 being available.
> > > >
> > > > Link: https://lore.kernel.org/netdev/7723d4l2kqgrez3yfauvp2ueu6awbizkrq4otqpsqpytzp45q2@rju2nxmqu4ew/
> > > > Suggested-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > > > Signed-off-by: Andrew Halaney <ahalaney@xxxxxxxxxx>
> > > > ---
> > > > I've marked this as an RFT because when looking through old mailing
> > > > list discusssions and kernel tech talks on this subject, I was unable
> > > > to really understand why in the past phy-handle had been left out. There
> > > > were some loose references to circular dependencies (which seem more or
> > > > less handled by fw_devlink to me), and the fact that a lot of behavior
> > > > happens in ndo_open() (but I couldn't quite grok the concern there).
> > > >
> > > > I'd appreciate more testing by others and some feedback from those who
> > > > know this a bit better to indicate whether fw_devlink is ready to handle
> > > > this or not.
> > > >
> > > > At least in my narrow point of view, it's working well for me.
> > >
> > > I do want this to land and I'm fairly certain it'll break something.
> > > But it's been so long that I don't remember what it was. I think it
> > > has to do with the generic phy driver not working well with fw_devlink
> > > because it doesn't go through the device driver model.
> >
> > Let me see if I can hack something up on this board (which has a decent
> > dependency tree for testing this stuff) to use the generic phy driver
> > instead of the marvell one that it needs and see how that goes. It won't
> > *actually* work from a phy perspective, but it will at least test out
> > the driver core bits here I think.
> >
> > >
> > > But like you said, it's been a while and fw_devlink has improved since
> > > then (I think). So please go ahead and give this a shot. If you can
> > > help fix any issues this highlights, I'd really appreciate it and I'd
> > > be happy to guide you through what I think needs to happen. But I
> > > don't think I have the time to fix it myself.
> >
> > Sure, I tend to agree. Let me check the generic phy driver path for any
> > issues and if that test seems to go okay I too am of the opinion that
> > without any solid reasoning against this we enable it and battle through
> > (revert and fix after the fact if necessary) any newly identified issues
> > that prevent phy-handle and fw_devlink have with each other.
> >
>
> Hmmm, yes the generic phy driver path for this
> doesn't seem to work well. Its fine and dandy if there's
> no device_link (current situation), but if there is one
> (say with my patch and in my example above between ethernet1 and phy@a,
> you can ignore the ethernet0 relationship since its a cycle
> and therefore no device_link is created as mentioned in the patch)
> you run into problems with the generic phy driver.
>
> In my original test you can see I use the marvell driver
> for the phy. In that case things work well. In the generic phy
> case however, ethernet1's probe is actually delayed far past
> phy@a. Here's some logs that show that the device_link getting
> "relaxed" due to no driver being bound, which has fw_devlink
> thinking this supplier phy isn't going to get a driver ever,
> so it finally tries to unblock (probe) the consumer (ethernet1):
>
>     [   40.695570] platform 23000000.ethernet: Relaxing link with stmmac-0:0a
>     [   40.702274] CPU: 4 UID: 0 PID: 111 Comm: kworker/u34:1 Not tainted 6.12.0-rc1-next-20240930-00004-gb766c5527800-dirty #155
>     [   40.713605] Hardware name: Qualcomm SA8775P Ride (DT)
>     [   40.718789] Workqueue: events_unbound deferred_probe_work_func
>     [   40.724774] Call trace:
>     [   40.727295]  dump_backtrace+0x108/0x190
>     [   40.731233]  show_stack+0x24/0x38
>     [   40.734638]  dump_stack_lvl+0x40/0x88
>     [   40.738406]  dump_stack+0x18/0x28
>     [   40.741811]  fw_devlink_unblock_consumers+0x78/0xe8
>     [   40.746824]  device_add+0x290/0x3f8
>     [   40.750411]  phy_device_register+0x6c/0xd0
>     [   40.754615]  fwnode_mdiobus_phy_device_register+0xe8/0x178
>     [   40.760246]  fwnode_mdiobus_register_phy+0x214/0x268
>     [   40.765344]  __of_mdiobus_parse_phys+0x80/0x280
>     [   40.769995]  __of_mdiobus_register+0xd0/0x230
>     [   40.774465]  stmmac_mdio_register+0x220/0x3c8 [stmmac]
>     [   40.779755]  stmmac_dvr_probe+0x91c/0xd70 [stmmac]
>     [   40.784682]  devm_stmmac_pltfr_probe+0x54/0xe0 [stmmac_platform]
>     [   40.790846]  qcom_ethqos_probe+0x404/0x438 [dwmac_qcom_ethqos]
>     [   40.796830]  platform_probe+0x94/0xd8
>
> If I understand correctly that's because the generic phy driver
> is bound during a MAC's (like ethernet1 here) phylink_fwnode_phy_connect() call
> in ndo_open() currently.. here's another dump_stack() (yes I abuse that alot)
> showing when that happens:
>
>     [   42.980611] net end1: Before phylink_fwnode_phy_connect
>     [   42.986011] CPU: 4 UID: 0 PID: 310 Comm: NetworkManager Not tainted 6.12.0-rc1-next-20240930-00004-gb766c5527800-dirty #156
>     [   42.997436] Hardware name: Qualcomm SA8775P Ride (DT)
>     [   43.002632] Call trace:
>     [   43.005152]  dump_backtrace+0x108/0x190
>     [   43.009106]  show_stack+0x24/0x38
>     [   43.012518]  dump_stack_lvl+0x40/0x88
>     [   43.016290]  dump_stack+0x18/0x28
>     [   43.019701]  phy_attach_direct+0x2d4/0x3e0
>     [   43.023918]  phylink_fwnode_phy_connect+0xc4/0x178
>     [   43.028848]  __stmmac_open+0x698/0x6e0 [stmmac]
>     [   43.033534]  stmmac_open+0x54/0xe0 [stmmac]
>     [   43.037850]  __dev_open+0x110/0x228
>     [   43.041442]  __dev_change_flags+0xbc/0x1d0
>
>
> And here's the code for the binding of the generic phy driver:
>
>     /**
>      * phy_attach_direct - attach a network device to a given PHY device pointer
>      * @dev: network device to attach
>      * @phydev: Pointer to phy_device to attach
>      * @flags: PHY device's dev_flags
>      * @interface: PHY device's interface
>      *
>      * Description: Called by drivers to attach to a particular PHY
>      *     device. The phy_device is found, and properly hooked up
>      *     to the phy_driver.  If no driver is attached, then a
>      *     generic driver is used.  The phy_device is given a ptr to
>      *     the attaching device, and given a callback for link status
>      *     change.  The phy_device is returned to the attaching driver.
>      *     This function takes a reference on the phy device.
>      */
>     int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>                           u32 flags, phy_interface_t interface)
>     {
>             struct mii_bus *bus = phydev->mdio.bus;
>             struct device *d = &phydev->mdio.dev;
>             struct module *ndev_owner = NULL;
>             bool using_genphy = false;
>             int err;
>
>             /* For Ethernet device drivers that register their own MDIO bus, we
>              * will have bus->owner match ndev_mod, so we do not want to increment
>              * our own module->refcnt here, otherwise we would not be able to
>              * unload later on.
>              */
>             if (dev)
>                     ndev_owner = dev->dev.parent->driver->owner;
>             if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
>                     phydev_err(phydev, "failed to get the bus module\n");
>                     return -EIO;
>             }
>
>             get_device(d);
>
>             /* Assume that if there is no driver, that it doesn't
>              * exist, and we should use the genphy driver.
>              */
>             if (!d->driver) {
>                     if (phydev->is_c45)
>                             d->driver = &genphy_c45_driver.mdiodrv.driver;
>                     else
>                             d->driver = &genphy_driver.mdiodrv.driver;
>
>                     using_genphy = true;
>                     dump_stack();
>             }
>
>             if (!try_module_get(d->driver->owner)) {
>                     phydev_err(phydev, "failed to get the device driver module\n");
>                     err = -EIO;
>                     goto error_put_device;
>             }
>
>             if (using_genphy) {
>                     err = d->driver->probe(d);
>                     if (err >= 0)
>                             err = device_bind_driver(d);
>
>                     if (err)
>                             goto error_module_put;
>             }
>
>             ...
>     }
>
> Something will need to be done for the generic phy driver case before
> this patch could be considered acceptable as this would slow the boot time
> for the topology I described in the patch description if the generic phy
> driver was used.

Right. And the way to do that is to move the generic phy driver
matching to go through the normal probe model instead of doing a
direct driver attach or directly calling the probe function. It's
possible and I had a mental model a while ago, but didn't have the
time to get around to it. Basically, if we find that none of the
drivers match, we need to trigger something like -EPROBE_DEFER again
and then match it with the generic phy driver. Or figure out some
other way for the generic phy driver to NOT match if a better driver
is available. Once we do that, I think the rest should be easy to fix.

Let me know if there's anything I can do to help you move this forward.

-Saravana