On Thu, Mar 17, 2022 at 04:53:34PM +0100, Oliver Neukum wrote:

On 15.03.22 14:28, Andrew Lunn wrote:

quoted

quoted

quoted

quoted

It was linked to unregistered/freed
netdev. This is why my patch changing the order to call phy_disconnect()
first and then unregister_netdev().

Unregistered yes, but freed no.  Here's the order before 2c9d6c2b871d:

  usbnet_disconnect()
    unregister_netdev()
    ax88772_unbind()
      phy_disconnect()
    free_netdev()

Is it illegal to disconnect a PHY from an unregistered, but not yet freed
net_device?

There are drivers which unregistering and then calling
phy_disconnect. In general that should be a valid pattern. But more
MAC drivers actually connect the PHY on open and disconnect it on
close. So it is less well used.

this is an interesting discussion, but what practical conclusion
do we draw from it? Is it necessary to provide both orders
of notifying the subdriver, or isn't it?

I see two possible solutions:

(1) The pattern of unregistering a net_device and then detaching the PHY
    is made legal by setting attached_dev = NULL on unregister and adding
    NULL pointer checks to phylib.  I'll wait for the phylib maintainers'
    comments whether they find that acceptable.

(2) Affected drivers (asix_devices.c, smsc95xx.c, possibly others) are
    amended with ->ndo_uninit() callbacks, which call phy_disconnect().
    That's basically your (Oliver's) idea to split usbnet ->unbind,
    but without actually having to split it.  (Just use the existing
    ->ndo_uninit.)

By the way: 2c9d6c2b871d caused breakage in smsc95xx.c which was
subsequently fixed by a049a30fc27c.  That in turn required another
fix, 0bf3885324a8.  Some of these code changes will have to be
rolled back or adjusted after reverting 2c9d6c2b871d.  It's a giant mess.
It's possible that more drivers saw fixes due to 2c9d6c2b871d,
I haven't checked that yet.

Oliver, why did you want to revert 2c9d6c2b871d, i.e. in which drivers
have your users reported breakage?  Do you have bugzilla links?

Thanks,

Lukas