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:
> > >
> > > ðernet0 {
> > > 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";
> > > };
> > > };
> > > };
> > >
> > > ðernet1 {
> > > 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.
