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, Andrew, Russell,

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

After scratching my head maybe a bit too hard and re-reading the
replies from Andrew and Russell, I think there's indeed a problem. The
SFP case as described by Russell, from my understanding, leads me to
believe that the way PHY's are tracked by phy_link_topology is correct,
but that doesn't mean that what I try do to in this exact patch is
right.

After the phydev has been retrieved from the topology and stored in the
req_info, nothing guarantees that the PHY won't vanish between the
moment we get it here and the moment we use it in the ethnl command
handling (SFP removal being a good example, and probably(?) the only
problematic case).

A solution would be, as Russell says, to make sure we get the PHY and
do whatever we need to do with it with rtnl held. Fortunately that
shouldn't require significant rework of individual netlink commands
that use the phydev, as they already manipulate it while holding rtnl().

So, I'll ditch this idea of storing the phydev pointer in
the req_info, I'll just store the phy_index (if it was passed by user)
and grab the phy whenever we need to.

Let me know if you find some flaw in my analysis, and thanks for
spotting this.

Best regards,

Maxime