On Mon, Mar 21, 2022 at 02:10:27PM +0100, Andrew Lunn wrote:

There are two patterns in use at the moment:

1) The phy is attached in open() and detached in close(). There is no
   danger of the netdev disappearing at this time.

2) The PHY is attached during probe, and detached during release.

This second case is what is being used here in the USB code. This is
also a common pattern for complex devices. In probe, you get all the
components of a complex devices, stitch them together and then
register the composite device. During release, you unregister the
composite device, and then release all the components. Since this is a
natural model, i think it should work.

I've gone through all drivers and noticed that some of them use a variation
of pattern 2 which looks fishy:

On probe, they first attach the PHY, then register the netdev.
On remove, they detach the PHY, then unregister the netdev.

Is it legal to detach the PHY from a registered (potentially running)
netdev? It looks wrong to me.

Affected drivers:

          drivers/net/ethernet/actions/owl-emac.c
          drivers/net/ethernet/altera/altera_tse_main.c
          drivers/net/ethernet/apm/xgene-v2/mdio.c
          drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
          drivers/net/ethernet/arc/emac_main.c
          drivers/net/ethernet/asix/ax88796c_main.c
          drivers/net/ethernet/broadcom/tg3.c
          drivers/net/ethernet/cortina/gemini.c
          drivers/net/ethernet/dnet.c
          drivers/net/ethernet/ethoc.c
          drivers/net/ethernet/hisilicon/hip04_eth.c
          drivers/net/ethernet/lantiq_etop.c
          drivers/net/ethernet/marvell/pxa168_eth.c
          drivers/net/ethernet/toshiba/tc35815.c

Some of these use devm_register_netdev() on probe and disconnect the
PHY in their ->remove hook.  They're missing the fact that
__device_release_driver() calls the ->remove hook before
devres_release_all().

Thanks,

Lukas