| Message ID | 20230307005028.2065800-1-grundler@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | 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 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); > } >
[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
On Mon, Mar 06, 2023 at 10:10:09PM -0800, Grant Grundler wrote: > 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: > > > + 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? > > Because other drivers I looked at (e.g. tg3 and r8169) do all the mdiobus_* > calls in one function and I wanted to have some "symmetry" > with ax88772_release_mdio() function I added below. I'd suggest moving this cleanup to a separate commit so that you keep the fix itself as small as possible and thus minimize the potential of introducing regressions in stable kernels that will receive the fix. Also, per convention please use the if-clause to catch the error case, not the success case. It doesn't matter if you need two or three more lines, readability is more important IMO. Thanks, Lukas
On Tue, Mar 7, 2023 at 2:29 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Mon, Mar 06, 2023 at 10:10:09PM -0800, Grant Grundler wrote: > > 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: > > > > + 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? > > > > Because other drivers I looked at (e.g. tg3 and r8169) do all the mdiobus_* > > calls in one function and I wanted to have some "symmetry" > > with ax88772_release_mdio() function I added below. > > I'd suggest moving this cleanup to a separate commit so that you keep > the fix itself as small as possible and thus minimize the potential of > introducing regressions in stable kernels that will receive the fix. Ok - will do. > Also, per convention please use the if-clause to catch the error case, > not the success case. It doesn't matter if you need two or three more > lines, readability is more important IMO. Sure. No problem. The code I wrote seemed easier to read but I'm familiar with the convention and have no objection to that. I'll add the Fixes: tag as well (thanks for confirming Oleksij!) cheers, grant > > Thanks, > > Lukas
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); }