"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/[email protected]/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 <[email protected]>
Tested-by: Eizan Miyamoto <[email protected]>
Signed-off-by: Grant Grundler <[email protected]>
---
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/[email protected]/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 <[email protected]>
> Tested-by: Eizan Miyamoto <[email protected]>
> Signed-off-by: Grant Grundler <[email protected]>
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 <[email protected]> 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/[email protected]/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 <[email protected]>
> > Tested-by: Eizan Miyamoto <[email protected]>
> > Signed-off-by: Grant Grundler <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> 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 |
On Mon, Mar 06, 2023 at 10:10:09PM -0800, Grant Grundler wrote:
> On Mon, Mar 6, 2023 at 7:46???PM Florian Fainelli <[email protected]> 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 <[email protected]> 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 <[email protected]> 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