[PATCHv2 1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename"

[PATCHv2 1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename"

[Grant Grundler]

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

Fixes: e532a096be0e5 ("net: usb: asix: ax88772: add phylib support")
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 | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

V2: moved mdiobus_get_phy() call back into ax88772_init_phy()
   Lukas Wunner is entirely correct this patch is much easier
   to backport without this patch hunk.
   Added "Fixes:" tag per request from Florian Fainelli

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 30e87389aefa1..21845b88a64b9 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,20 @@ 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);
+		mdiobus_free(priv->mdio);
+		priv->mdio = NULL;
+	}
+
+	return ret;
+}
+
+static void ax88772_mdio_unregister(struct asix_common_private *priv)
+{
+	mdiobus_unregister(priv->mdio);
+	mdiobus_free(priv->mdio);
 }
 
 static int ax88772_init_phy(struct usbnet *dev)
@@ -664,6 +678,7 @@ static int ax88772_init_phy(struct usbnet *dev)
 	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
 	if (!priv->phydev) {
 		netdev_err(dev->net, "Could not find PHY\n");
+		ax88772_mdio_unregister(priv);
 		return -ENODEV;
 	}
 
@@ -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_mdio_unregister(priv);
 	asix_rx_fixup_common_free(dev->driver_priv);
 }
 
-- 
2.39.2

[PATCHv2 2/2] net: asix: init mdiobus from one function

[Grant Grundler]

Make asix driver consistent with other drivers (e.g. tg3 and r8169) which
use mdiobus calls: setup and tear down be handled in one function each.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 drivers/net/usb/asix_devices.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 21845b88a64b9..d7caab4493d15 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -637,7 +637,7 @@ static int asix_resume(struct usb_interface *intf)
 	return usbnet_resume(intf);
 }
 
-static int ax88772_init_mdio(struct usbnet *dev)
+static int ax88772_mdio_register(struct usbnet *dev)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 	int ret;
@@ -657,10 +657,22 @@ static int ax88772_init_mdio(struct usbnet *dev)
 	ret = mdiobus_register(priv->mdio);
 	if (ret) {
 		netdev_err(dev->net, "Could not register MDIO bus (err %d)\n", ret);
-		mdiobus_free(priv->mdio);
-		priv->mdio = NULL;
+		goto mdio_register_err;
 	}
 
+	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
+	if (!priv->phydev) {
+		netdev_err(dev->net, "Could not find PHY\n");
+		ret=-ENODEV;
+		goto mdio_phy_err;
+	}
+
+	return 0;
+
+mdio_phy_err:
+	mdiobus_unregister(priv->mdio);
+mdio_register_err:
+	mdiobus_free(priv->mdio);
 	return ret;
 }
 
@@ -675,13 +687,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");
-		ax88772_mdio_unregister(priv);
-		return -ENODEV;
-	}
-
 	ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
 				 PHY_INTERFACE_MODE_INTERNAL);
 	if (ret) {
@@ -799,7 +804,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
 	priv->presvd_phy_bmcr = 0;
 	priv->presvd_phy_advertise = 0;
 
-	ret = ax88772_init_mdio(dev);
+	ret = ax88772_mdio_register(dev);
 	if (ret)
 		return ret;
 
-- 
2.39.2

Re: [PATCHv2 2/2] net: asix: init mdiobus from one function

[Grant Grundler]

On Tue, Mar 7, 2023 at 12:05 PM Grant Grundler <grundler@chromium.org> wrote:
>
> Make asix driver consistent with other drivers (e.g. tg3 and r8169) which
> use mdiobus calls: setup and tear down be handled in one function each.
>
> Signed-off-by: Grant Grundler <grundler@chromium.org>
> ---
>  drivers/net/usb/asix_devices.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 21845b88a64b9..d7caab4493d15 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -637,7 +637,7 @@ static int asix_resume(struct usb_interface *intf)
>         return usbnet_resume(intf);
>  }
>
> -static int ax88772_init_mdio(struct usbnet *dev)
> +static int ax88772_mdio_register(struct usbnet *dev)
>  {
>         struct asix_common_private *priv = dev->driver_priv;
>         int ret;
> @@ -657,10 +657,22 @@ static int ax88772_init_mdio(struct usbnet *dev)
>         ret = mdiobus_register(priv->mdio);
>         if (ret) {
>                 netdev_err(dev->net, "Could not register MDIO bus (err %d)\n", ret);
> -               mdiobus_free(priv->mdio);
> -               priv->mdio = NULL;
> +               goto mdio_register_err;
>         }
>
> +       priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
> +       if (!priv->phydev) {
> +               netdev_err(dev->net, "Could not find PHY\n");
> +               ret=-ENODEV;

My apologies: checkpatch wants spaces around "=" and especially
because it should be "ret = -ENODEV".

I can resend - but is this trivial enough to fix up by maintainer?

cheers,
grant

> +               goto mdio_phy_err;
> +       }
> +
> +       return 0;
> +
> +mdio_phy_err:
> +       mdiobus_unregister(priv->mdio);
> +mdio_register_err:
> +       mdiobus_free(priv->mdio);
>         return ret;
>  }
>
> @@ -675,13 +687,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");
> -               ax88772_mdio_unregister(priv);
> -               return -ENODEV;
> -       }
> -
>         ret = phy_connect_direct(dev->net, priv->phydev, &asix_adjust_link,
>                                  PHY_INTERFACE_MODE_INTERNAL);
>         if (ret) {
> @@ -799,7 +804,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
>         priv->presvd_phy_bmcr = 0;
>         priv->presvd_phy_advertise = 0;
>
> -       ret = ax88772_init_mdio(dev);
> +       ret = ax88772_mdio_register(dev);
>         if (ret)
>                 return ret;
>
> --
> 2.39.2
>

Re: [PATCHv2 1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename"

[Jakub Kicinski]

On Tue,  7 Mar 2023 12:05:01 -0800 Grant Grundler wrote:
> Subject: [PATCHv2 1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename"

Why the "TEST:" prefix?

The patch doesn't apply cleanly, it needs to go via this tree:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
so rebase it onto that, please, and put [PATCH net] in the subject
rather than just [PATCH].

Keep patch 2 locally for about a week (we merge fixes and cleanup
branches once a week around Thu, and the two patches depend on each
other).

Please look thru at least the tl;dr of our doc:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Re: [PATCHv2 1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename"

[Grant Grundler]

On Tue, Mar 7, 2023 at 4:47 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue,  7 Mar 2023 12:05:01 -0800 Grant Grundler wrote:
> > Subject: [PATCHv2 1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename"
>
> Why the "TEST:" prefix?

Sorry - that's left over from how I mark the change for testing with
chromeos-5.15 kernel branch:
   https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4313619

I should have removed that.  I upload the change to gerrit so partners
can easily test the same code  (e.g. coworker Eizan who is in
Australia).

If you follow the link above, you can see I'm testing a bunch of
additional backports as well and have additional fields in the commit
message required by chromium.org.

> The patch doesn't apply cleanly, it needs to go via this tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
> so rebase it onto that, please, and put [PATCH net] in the subject
> rather than just [PATCH].

Ok - thanks! Wil repost v3 against netdev/net.git/ shortly. No problem.

> Keep patch 2 locally for about a week (we merge fixes and cleanup
> branches once a week around Thu, and the two patches depend on each
> other).

Awesome! Sounds good.

> Please look thru at least the tl;dr of our doc:
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Thanks!

Because I've been "randomly" contributing to netdev for 20+ years,
I've not looked for documentation (beyond SubmittingPatches). But I am
quite willing to read and follow it - makes life easier for everyone.

cheers,
grant