diff mbox series

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

Message ID 20230307200502.2263655-1-grundler@chromium.org
State New
Headers
Series [PATCHv2,1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename" |

Commit Message

Grant Grundler March 7, 2023, 8:05 p.m. UTC 
  "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

Comments

Jakub Kicinski March 8, 2023, 12:47 a.m. UTC | #1 
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
Grant Grundler March 8, 2023, 6:34 p.m. UTC | #2 
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
diff mbox series

Patch

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);
 }