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

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

Hi Russell,

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

I'm replying to your mail to summarize what the topology code does,
which I think is correct according to these explanations.

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

Patch 03 adds a phy_sfp_connect_phy() / phy_sfp_disconnect_phy() set of
helpers that new PHY drivers supporting downstream SFP busses must
specify in their sfp_upstream_ops, which will add/remove the SFP phy
to/from the topology. I realize now that I need to add some clear
documentation so that new drivers get that right.

That is because in this situation, the SFP PHY won't be attached to the
netdev (as the media-converter PHY already is), so relying on the
phy_attach / phy_detach paths won't cover these cases.

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

And here in that case, there's a hook in phy_detach() to remove the phy
from the topology as well, which deals with the case where phylink
deals with the sfp_upstream_ops. I think this covers all cases :)

Thanks,

Maxime