| Message ID | 20230307200502.2263655-1-grundler@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv2,1/2] TEST:net: asix: fix modprobe "sysfs: cannot create duplicate filename" | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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
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 --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); }
"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