"modprobe asix ; rmmod asix ; modprobe asix" fails with:
   sysfs: cannot create duplicate filename \
   	'/devices/virtual/mdio_bus/usb-003:004'

Issue was originally reported by Anton Lundin on 2022-06-22 14:16 UTC:
   https://lore.kernel.org/netdev/20220623063649.GD23685@pengutronix.de/T/

Chrome OS team hit the same issue in Feb, 2023 when trying to find
work arounds for other issues with AX88172 devices.

The use of devm_mdiobus_register() with usbnet devices results in the
MDIO data being associated with the USB device. When the asix driver
is unloaded, the USB device continues to exist and the corresponding
"mdiobus_unregister()" is NOT called until the USB device is unplugged
or unauthorized. So the next "modprobe asix" will fail because the MDIO
phy sysfs attributes still exist.

The 'easy' (from a design PoV) fix is to use the non-devm variants of
mdiobus_* functions and explicitly manage this use in the asix_bind
and asix_unbind function calls. I've not explored trying to fix usbnet
initialization so devm_* stuff will work.

Reported-by: Anton Lundin <glance@acc.umu.se>
Tested-by: Eizan Miyamoto <eizan@chromium.org>
Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 drivers/net/usb/asix_devices.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 30e87389aefa1..f0a87b933062a 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -640,8 +640,9 @@ static int asix_resume(struct usb_interface *intf)
 static int ax88772_init_mdio(struct usbnet *dev)
 {
 	struct asix_common_private *priv = dev->driver_priv;
+	int ret;
 
-	priv->mdio = devm_mdiobus_alloc(&dev->udev->dev);
+	priv->mdio = mdiobus_alloc();
 	if (!priv->mdio)
 		return -ENOMEM;
 
@@ -653,7 +654,27 @@ static int ax88772_init_mdio(struct usbnet *dev)
 	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
 		 dev->udev->bus->busnum, dev->udev->devnum);
 
-	return devm_mdiobus_register(&dev->udev->dev, priv->mdio);
+	ret = mdiobus_register(priv->mdio);
+	if (ret) {
+		netdev_err(dev->net, "Could not register MDIO bus (err %d)\n", ret);
+		goto mdio_regerr;
+	}
+
+	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
+	if (priv->phydev)
+		return 0;
+
+	netdev_err(dev->net, "Could not find PHY\n");
+	mdiobus_unregister(priv->mdio);
+mdio_regerr:
+	mdiobus_free(priv->mdio);
+	return ret;
+}
+
+static void ax88772_release_mdio(struct asix_common_private *priv)
+{
+	mdiobus_unregister(priv->mdio);
+	mdiobus_free(priv->mdio);
 }
 
 static int ax88772_init_phy(struct usbnet *dev)
@@ -661,12 +682,6 @@ static int ax88772_init_phy(struct usbnet *dev)
 	struct asix_common_private *priv = dev->driver_priv;
 	int ret;
 
-	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
-	if (!priv->phydev) {
-		netdev_err(dev->net, "Could not find PHY\n");
-		return -ENODEV;
-	}
-
 	ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
 				 PHY_INTERFACE_MODE_INTERNAL);
 	if (ret) {
@@ -805,6 +820,7 @@ static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
 	struct asix_common_private *priv = dev->driver_priv;
 
 	phy_disconnect(priv->phydev);
+	ax88772_release_mdio(priv);
 	asix_rx_fixup_common_free(dev->driver_priv);
 }
 
-- 
2.39.2

On 3/6/2023 4:50 PM, Grant Grundler wrote:
> "modprobe asix ; rmmod asix ; modprobe asix" fails with:
>     sysfs: cannot create duplicate filename \
>     	'/devices/virtual/mdio_bus/usb-003:004'
> 
> Issue was originally reported by Anton Lundin on 2022-06-22 14:16 UTC:
>     https://lore.kernel.org/netdev/20220623063649.GD23685@pengutronix.de/T/
> 
> Chrome OS team hit the same issue in Feb, 2023 when trying to find
> work arounds for other issues with AX88172 devices.
> 
> The use of devm_mdiobus_register() with usbnet devices results in the
> MDIO data being associated with the USB device. When the asix driver
> is unloaded, the USB device continues to exist and the corresponding
> "mdiobus_unregister()" is NOT called until the USB device is unplugged
> or unauthorized. So the next "modprobe asix" will fail because the MDIO
> phy sysfs attributes still exist.
> 
> The 'easy' (from a design PoV) fix is to use the non-devm variants of
> mdiobus_* functions and explicitly manage this use in the asix_bind
> and asix_unbind function calls. I've not explored trying to fix usbnet
> initialization so devm_* stuff will work.
> 
> Reported-by: Anton Lundin <glance@acc.umu.se>
> Tested-by: Eizan Miyamoto <eizan@chromium.org>
> Signed-off-by: Grant Grundler <grundler@chromium.org>

Should we have a Fixes: tag here? One more question below

> ---
>   drivers/net/usb/asix_devices.c | 32 ++++++++++++++++++++++++--------
>   1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 30e87389aefa1..f0a87b933062a 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -640,8 +640,9 @@ static int asix_resume(struct usb_interface *intf)
>   static int ax88772_init_mdio(struct usbnet *dev)
>   {
>   	struct asix_common_private *priv = dev->driver_priv;
> +	int ret;
>   
> -	priv->mdio = devm_mdiobus_alloc(&dev->udev->dev);
> +	priv->mdio = mdiobus_alloc();
>   	if (!priv->mdio)
>   		return -ENOMEM;
>   
> @@ -653,7 +654,27 @@ static int ax88772_init_mdio(struct usbnet *dev)
>   	snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
>   		 dev->udev->bus->busnum, dev->udev->devnum);
>   
> -	return devm_mdiobus_register(&dev->udev->dev, priv->mdio);
> +	ret = mdiobus_register(priv->mdio);
> +	if (ret) {
> +		netdev_err(dev->net, "Could not register MDIO bus (err %d)\n", ret);
> +		goto mdio_regerr;
> +	}
> +
> +	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
> +	if (priv->phydev)
> +		return 0;

This was in ax88772_init_phy() before, why is this being moved here now?

> +
> +	netdev_err(dev->net, "Could not find PHY\n");
> +	mdiobus_unregister(priv->mdio);
> +mdio_regerr:
> +	mdiobus_free(priv->mdio);
> +	return ret;
> +}
> +
> +static void ax88772_release_mdio(struct asix_common_private *priv)
> +{
> +	mdiobus_unregister(priv->mdio);
> +	mdiobus_free(priv->mdio);
>   }
>   
>   static int ax88772_init_phy(struct usbnet *dev)
> @@ -661,12 +682,6 @@ static int ax88772_init_phy(struct usbnet *dev)
>   	struct asix_common_private *priv = dev->driver_priv;
>   	int ret;
>   
> -	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
> -	if (!priv->phydev) {
> -		netdev_err(dev->net, "Could not find PHY\n");
> -		return -ENODEV;
> -	}
> -
>   	ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
>   				 PHY_INTERFACE_MODE_INTERNAL);
>   	if (ret) {
> @@ -805,6 +820,7 @@ static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
>   	struct asix_common_private *priv = dev->driver_priv;
>   
>   	phy_disconnect(priv->phydev);
> +	ax88772_release_mdio(priv);
>   	asix_rx_fixup_common_free(dev->driver_priv);
>   }
>   

-- 
Florian

[dropping Aton Lundin <glance@...> since it's bouncing ... and
replying in "text only"]

On Mon, Mar 6, 2023 at 7:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 3/6/2023 4:50 PM, Grant Grundler wrote:
> > "modprobe asix ; rmmod asix ; modprobe asix" fails with:
> >     sysfs: cannot create duplicate filename \
> >       '/devices/virtual/mdio_bus/usb-003:004'
> >
> > Issue was originally reported by Anton Lundin on 2022-06-22 14:16 UTC:
> >     https://lore.kernel.org/netdev/20220623063649.GD23685@pengutronix.de/T/
> >
> > Chrome OS team hit the same issue in Feb, 2023 when trying to find
> > work arounds for other issues with AX88172 devices.
> >
> > The use of devm_mdiobus_register() with usbnet devices results in the
> > MDIO data being associated with the USB device. When the asix driver
> > is unloaded, the USB device continues to exist and the corresponding
> > "mdiobus_unregister()" is NOT called until the USB device is unplugged
> > or unauthorized. So the next "modprobe asix" will fail because the MDIO
> > phy sysfs attributes still exist.
> >
> > The 'easy' (from a design PoV) fix is to use the non-devm variants of
> > mdiobus_* functions and explicitly manage this use in the asix_bind
> > and asix_unbind function calls. I've not explored trying to fix usbnet
> > initialization so devm_* stuff will work.
> >
> > Reported-by: Anton Lundin <glance@acc.umu.se>
> > Tested-by: Eizan Miyamoto <eizan@chromium.org>
> > Signed-off-by: Grant Grundler <grundler@chromium.org>
>
> Should we have a Fixes: tag here? One more question below

I have no idea which change/patch caused this problem. I'm happy to
add whatever Fixes: tag folks suggest.

Looking at git blame, looks like the devm_mdiobus_* usage was
introduced with e532a096be0e5:

commit e532a096be0e5e570b383e71d4560e7f04384e0f
Author: Oleksij Rempel <linux@rempel-privat.de>
Date:   Mon Jun 7 10:27:23 2021 +0200

    net: usb: asix: ax88772: add phylib support

>
> > ---
> >   drivers/net/usb/asix_devices.c | 32 ++++++++++++++++++++++++--------
> >   1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> > index 30e87389aefa1..f0a87b933062a 100644
> > --- a/drivers/net/usb/asix_devices.c
> > +++ b/drivers/net/usb/asix_devices.c
> > @@ -640,8 +640,9 @@ static int asix_resume(struct usb_interface *intf)
> >   static int ax88772_init_mdio(struct usbnet *dev)
> >   {
> >       struct asix_common_private *priv = dev->driver_priv;
> > +     int ret;
> >
> > -     priv->mdio = devm_mdiobus_alloc(&dev->udev->dev);
> > +     priv->mdio = mdiobus_alloc();
> >       if (!priv->mdio)
> >               return -ENOMEM;
> >
> > @@ -653,7 +654,27 @@ static int ax88772_init_mdio(struct usbnet *dev)
> >       snprintf(priv->mdio->id, MII_BUS_ID_SIZE, "usb-%03d:%03d",
> >                dev->udev->bus->busnum, dev->udev->devnum);
> >
> > -     return devm_mdiobus_register(&dev->udev->dev, priv->mdio);
> > +     ret = mdiobus_register(priv->mdio);
> > +     if (ret) {
> > +             netdev_err(dev->net, "Could not register MDIO bus (err %d)\n", ret);
> > +             goto mdio_regerr;
> > +     }
> > +
> > +     priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
> > +     if (priv->phydev)
> > +             return 0;
>
> This was in ax88772_init_phy() before, why is this being moved here now?

1) To be consistent with other drivers (e.g. tg3 and r8169) which call
 mdiobus from one function.
2) So the functions called from ax88172_bind and ax88172_unbind are
"symmetric".  I am now thinking the two functions should be renamed to
ax88172_mdio_register and ..._unregister.

Thanks for looking Florian!

cheers,
grant

>
> > +
> > +     netdev_err(dev->net, "Could not find PHY\n");
> > +     mdiobus_unregister(priv->mdio);
> > +mdio_regerr:
> > +     mdiobus_free(priv->mdio);
> > +     return ret;
> > +}
> > +
> > +static void ax88772_release_mdio(struct asix_common_private *priv)
> > +{
> > +     mdiobus_unregister(priv->mdio);
> > +     mdiobus_free(priv->mdio);
> >   }
> >
> >   static int ax88772_init_phy(struct usbnet *dev)
> > @@ -661,12 +682,6 @@ static int ax88772_init_phy(struct usbnet *dev)
> >       struct asix_common_private *priv = dev->driver_priv;
> >       int ret;
> >
> > -     priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
> > -     if (!priv->phydev) {
> > -             netdev_err(dev->net, "Could not find PHY\n");
> > -             return -ENODEV;
> > -     }
> > -
> >       ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
> >                                PHY_INTERFACE_MODE_INTERNAL);
> >       if (ret) {
> > @@ -805,6 +820,7 @@ static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
> >       struct asix_common_private *priv = dev->driver_priv;
> >
> >       phy_disconnect(priv->phydev);
> > +     ax88772_release_mdio(priv);
> >       asix_rx_fixup_common_free(dev->driver_priv);
> >   }
> >
>
> --
> Florian

Hi Grant,

On Mon, Mar 06, 2023 at 10:22:06PM -0800, Grant Grundler wrote:
...
> [dropping Aton Lundin <glance@...> since it's bouncing ... and
> replying in "text only"]
> > Should we have a Fixes: tag here? One more question below
> 
> I have no idea which change/patch caused this problem. I'm happy to
> add whatever Fixes: tag folks suggest.
> 
> Looking at git blame, looks like the devm_mdiobus_* usage was
> introduced with e532a096be0e5:
> 
> commit e532a096be0e5e570b383e71d4560e7f04384e0f
> Author: Oleksij Rempel <linux@rempel-privat.de>
> Date:   Mon Jun 7 10:27:23 2021 +0200
> 
>     net: usb: asix: ax88772: add phylib support

Yes, this is the right commit.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |