Messages in this thread | | | Date | Fri, 15 Dec 2023 23:45:23 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next v4 01/13] net: phy: Introduce ethernet link topology representation |
| |
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);
| |