Message-ID: <20231218101131.04160623@device-28.home>
Date: Mon, 18 Dec 2023 10:11:31 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Russell King
 <linux@...linux.org.uk>, linux-arm-kernel@...ts.infradead.org, Christophe
 Leroy <christophe.leroy@...roup.eu>, Herve Codina
 <herve.codina@...tlin.com>, Florian Fainelli <f.fainelli@...il.com>, Heiner
 Kallweit <hkallweit1@...il.com>, Köry Maincent
 <kory.maincent@...tlin.com>, Jesse Brandeburg <jesse.brandeburg@...el.com>,
 Jonathan Corbet <corbet@....net>, Marek Behún
 <kabel@...nel.org>, Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>, Nicolò Veronese
 <nicveronese@...il.com>
Subject: Re: [PATCH net-next v4 01/13] net: phy: Introduce ethernet link
 topology representation

Hello Vlad, PHy maintainers,

On Fri, 15 Dec 2023 23:45:23 +0200
Vladimir Oltean <vladimir.oltean@....com> wrote:

> 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.

Ok so I'll give it more thought on that part, and analyze better the
access paths that ca be problematic. I'll update the doc accordingly,
as this is non-trivial. I haven't been able to test it on a DSA setup,
nor on a !phylink mac, so thanks a lot for testing :)
> 
> 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().

In that case this is fine, right ? As ndo_open runs with rtnl held,
phy-targetting netlink commands (that indeed goes through the netdev)
will be also serialized ? I might be missing the point though :(

> 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);  

Powered by blists - more mailing lists