Message-ID: <CANEJEGsYkxsbCj5O-O=QN8O0MEB-WY6FRJO6GFR0qt2sp4J8SA@mail.gmail.com>
Date:   Mon, 6 Mar 2023 22:22:06 -0800
From:   Grant Grundler <grundler@...omium.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Grant Grundler <grundler@...omium.org>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Pavel Skripkin <paskripkin@...il.com>,
        Lukas Wunner <lukas@...ner.de>,
        Eizan Miyamoto <eizan@...omium.org>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: asix: fix modprobe "sysfs: cannot create duplicate filename"

[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@...il.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@....umu.se>
> > Tested-by: Eizan Miyamoto <eizan@...omium.org>
> > Signed-off-by: Grant Grundler <grundler@...omium.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@...pel-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

Powered by blists - more mailing lists