Re: [PATCH net-next v4 01/13] net: phy: Introduce ethernet link topology representation

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Dec 15, 2023 at 06:12:23PM +0100, Maxime Chevallier wrote:
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d8e9335d415c..89daaccc9276 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1491,6 +1500,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  
>  		if (phydev->sfp_bus_attached)
>  			dev->sfp_bus = phydev->sfp_bus;
> +
> +		err = phy_link_topo_add_phy(&dev->link_topo, phydev,
> +					    PHY_UPSTREAM_MAC, dev);
> +		if (err)
> +			goto error;
>  	}
>  
>  	/* Some Ethernet drivers try to connect to a PHY device before
> @@ -1816,6 +1830,7 @@ void phy_detach(struct phy_device *phydev)
>  	if (dev) {
>  		phydev->attached_dev->phydev = NULL;
>  		phydev->attached_dev = NULL;
> +		phy_link_topo_del_phy(&dev->link_topo, phydev);
>  	}
>  	phydev->phylink = NULL;
>  
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> new file mode 100644
> index 000000000000..22f6372d002c
> --- /dev/null
> +++ b/drivers/net/phy/phy_link_topology.c
> +int phy_link_topo_add_phy(struct phy_link_topology *topo,
> +			  struct phy_device *phy,
> +			  enum phy_upstream upt, void *upstream)
> +{
> +	struct phy_device_node *pdn;
> +	int ret;
> +
> +	/* Protects phy and upstream */
> +	ASSERT_RTNL();

Something to think for the PHY library maintainers. This is probably
the first time when the rtnl_lock() requirement is asserted at
phy_attach_direct() time.

I haven't done too much with the patch set yet, so I don't understand
exactly from the comment what this is protecting. But I get the
following assertion failure with DSA:

[    4.157160] ------------[ cut here ]------------
[    4.161805] RTNL: assertion failed at drivers/net/phy/phy_link_topology.c (35)
[    4.169124] WARNING: CPU: 0 PID: 26 at drivers/net/phy/phy_link_topology.c:35 phy_link_topo_add_phy+0x128/0x130
[    4.179263] Modules linked in:
[    4.209232] pc : phy_link_topo_add_phy+0x128/0x130
[    4.214040] lr : phy_link_topo_add_phy+0x128/0x130
[    4.293822] Call trace:
[    4.296271]  phy_link_topo_add_phy+0x128/0x130
[    4.300730]  phy_attach_direct+0xbc/0x3c4
[    4.304752]  phylink_fwnode_phy_connect+0xa8/0xf8
[    4.309473]  phylink_of_phy_connect+0x1c/0x28
[    4.313844]  dsa_user_create+0x318/0x5ac
[    4.317778]  dsa_port_setup+0x100/0x144
[    4.321626]  dsa_register_switch+0xe90/0x11f8
[    4.325997]  sja1105_probe+0x2bc/0x2e4
[    4.329759]  spi_probe+0xa4/0xc4
[    4.332995]  really_probe+0x16c/0x3fc
[    4.336669]  __driver_probe_device+0xa4/0x168
[    4.341041]  driver_probe_device+0x3c/0x220
[    4.345238]  __device_attach_driver+0x128/0x1cc
[    4.349784]  bus_for_each_drv+0xf4/0x14c
[    4.353719]  __device_attach+0xfc/0x1bc
[    4.357567]  device_initial_probe+0x14/0x20
[    4.361764]  bus_probe_device+0x94/0x100
[    4.385371] ---[ end trace 0000000000000000 ]---

Someone please correct me if I'm wrong, but at least up until now, calling
this unlocked has been quite harmless, because we call dsa_user_phy_setup()
before register_netdevice(), and thus, the net_device is pretty much
inaccessible to the world when we attach it to the PHY.

And, while having the phydev->attached_dev pointer populated technically
makes the net_device now accessible from the PHY, this is a moot point,
because no user space command targets the PHY directly. They all target
the netdev, and through that, netdev->phydev. The netdev is still
unregistered, so it's ok to not have rtnl_lock().

It is rather going to be something that concerns those drivers which call
phy_attach_direct() after registering, for example from ndo_open().

Interestingly, phylink_disconnect_phy() has an ASSERT_RTNL() in it
even though the phylink_attach_phy() derivatives do not. I'm unable
to ascertain whether a previous unregister_netdevice() call makes this
requirement redundant or not.

> +
> +	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> +	if (!pdn)
> +		return -ENOMEM;
> +
> +	pdn->phy = phy;
> +	switch (upt) {
> +	case PHY_UPSTREAM_MAC:
> +		pdn->upstream.netdev = (struct net_device *)upstream;
> +		if (phy_on_sfp(phy))
> +			pdn->parent_sfp_bus = pdn->upstream.netdev->sfp_bus;
> +		break;
> +	case PHY_UPSTREAM_PHY:
> +		pdn->upstream.phydev = (struct phy_device *)upstream;
> +		if (phy_on_sfp(phy))
> +			pdn->parent_sfp_bus = pdn->upstream.phydev->sfp_bus;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +	pdn->upstream_type = upt;
> +
> +	ret = xa_alloc_cyclic(&topo->phys, &phy->phyindex, pdn, xa_limit_32b,
> +			      &topo->next_phy_index, GFP_KERNEL);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	kfree(pdn);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);