Re: [PATCH net-next v13 05/13] net: ethtool: Allow passing a phy index for some commands

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Jakub,

On Thu, 13 Jun 2024 18:26:13 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > +			struct nlattr *phy_id;
> > +
> > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > +			phydev = phy_link_topo_get_phy(dev,
> > +						       nla_get_u32(phy_id));  
> 
> Sorry for potentially repeating question (please put the answer in the
> commit message) - are phys guaranteed not to disappear, even if the
> netdev gets closed? this has no rtnl protection

I'll answer here so that people can correct me if I'm wrong, but I'll
also add it in the commit logs as well (and possibly with some fixes
depending on how this discussion goes)

While a PHY can be attached to/detached from a netdevice at open/close,
the phy_device itself will keep on living, as its lifetime is tied to
the underlying mdio_device (however phy_attach/detach take a ref on the
phy_device, preventing it from vanishing while it's attached to a
netdev)

I think the worst that could happen is that phy_detach() gets
called (at ndo_close() for example, but that's not the only possible
call site for that), and right after we manually unbind the PHY, which
will drop its last refcount, while we hold a pointer to it :

			phydev = phy_link_topo_get_phy()
 phy_detach(phydev)
 unbind on phydev
			/* access phydev */
			
PHY device lifetime is, from my understanding, not protected by
rtnl() so should a lock be added, I don't think rtnl_lock() would be
the one to use.

Maybe instead we should grab a reference to the phydev when we add it
to the topology ?

> 
> > +			if (!phydev) {
> > +				NL_SET_BAD_ATTR(extack, phy_id);
> > +				return -ENODEV;
> > +			}
> > +		} else {
> > +			/* If we need a PHY but no phy index is specified, fallback
> > +			 * to dev->phydev  
> 
> please double check the submission for going over 80 chars, this one
> appears to be particularly pointlessly over 80 chars...

Arg yes sorry about this one...
 
> > +			 */
> > +			phydev = dev->phydev;  

Thanks,

Maxime