Re: ordering of call to unbind() in usbnet_disconnect

[Lukas Wunner <lukas@wunner.de>]

On Tue, Mar 15, 2022 at 06:44:03AM +0100, Oleksij Rempel wrote:
> On Mon, Mar 14, 2022 at 08:14:36PM +0100, Andrew Lunn wrote:
> > On Mon, Mar 14, 2022 at 07:42:34PM +0100, Lukas Wunner wrote:
> > > On Thu, Mar 10, 2022 at 12:38:20PM +0100, Oleksij Rempel wrote:
> > > > On Thu, Mar 10, 2022 at 12:25:08PM +0100, Oliver Neukum wrote:
> > > > > I got bug reports that 2c9d6c2b871d ("usbnet: run unbind() before
> > > > > unregister_netdev()") is causing regressions.
> > > > > Rather than simply reverting it,
> > > > > it seems to me that the call needs to be split. One in the old place
> > > > > and one in the place you moved it to.
> > > 
> > > I disagree.  The commit message claims that the change is necessary
> > > because phy_disconnect() fails if called with
> > > phydev->attached_dev == NULL.
> > 
> > The only place i see which sets phydev->attached_dev is
> > phy_attach_direct(). So if phydev->attached_dev is NULL, the PHY has
> > not been attached, and hence there is no need to call
> > phy_disconnect().
> 
> phydev->attached_dev is not NULL.

Right, I was mistaken, sorry.


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

Oleksij, the commit message of 2c9d6c2b871d says that disconnecting the
PHY "fails" in that situation.  Please elaborate what the failure looked
like.  Did you get a stacktrace?

Thanks,

Lukas