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

On Sun, Jun 16, 2024 at 05:21:25PM +0200, Andrew Lunn wrote:

On Sun, Jun 16, 2024 at 06:02:31PM +0200, Maxime Chevallier wrote:

quoted

Hello Jakub,

On Thu, 13 Jun 2024 18:26:13 -0700
Jakub Kicinski [off-list ref] wrote:

quoted

On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:

quoted

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

It gets interesting with copper SFP. They contain a PHY, and that PHY
can physically disappear at any time. What i don't know is when the
logical representation of the PHY will disappear after the hotunplug
event.

On a SFP module unplug, the following upstream device methods will be
called in order:
1. link_down
2. module_stop
3. disconnect_phy

At this point, the PHY device will be removed (phy_device_remove()) and
freed (phy_device_free()), and shortly thereafter, the MDIO bus is
unregistered and thus destroyed.

In response to the above, phylink will, respectively for each method:

1. disable the netdev carrier and call mac_link_down()
2. call phy_stop() on the attached PHY
3. remove the PHY from phylink, and then call phy_disconnect(),
   disconnecting it from the netdev.

Thus, when a SFP PHY is being removed, phylib will see in order the
following calls:

	phy_disconnect()
	phy_device_remove()
	phy_device_free()

Provided the topology linkage is removed on phy_disconnect() which
disassociates the PHY from the netdev, SFP PHYs should be fine.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!