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

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

[Grant Grundler]

"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:

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: e532a096be0e ("net: usb: asix: ax88772: add phylib support")
Reported-by: Anton Lundin <glance@acc.umu.se>
Link: https://lore.kernel.org/netdev/20220623063649.GD23685@pengutronix.de/T/
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(-)

V3: rebase against netdev/net.git
    remove "TEST" prefix in subject line
    added Link: tag for Reported-by tag

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

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 743cbf5d662c..538c84909913 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -666,8 +666,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;
 
@@ -679,7 +680,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)
@@ -690,6 +704,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;
 	}
 
@@ -926,6 +941,7 @@ static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
 	phylink_disconnect_phy(priv->phylink);
 	rtnl_unlock();
 	phylink_destroy(priv->phylink);
+	ax88772_mdio_unregister(priv);
 	asix_rx_fixup_common_free(dev->driver_priv);
 }
 
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Grant Grundler]

Make asix driver consistent with other drivers (e.g. tg3 and r8169) which
use mdiobus calls: setup and tear down be handled in one function each.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
 drivers/net/usb/asix_devices.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

V3: rebase against netdev/net.git
    add missing whitespace around "="

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 538c84909913..9a1e54ef4ff0 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -663,7 +663,7 @@ static int asix_resume(struct usb_interface *intf)
 	return usbnet_resume(intf);
 }
 
-static int ax88772_init_mdio(struct usbnet *dev)
+static int ax88772_mdio_register(struct usbnet *dev)
 {
 	struct asix_common_private *priv = dev->driver_priv;
 	int ret;
@@ -683,10 +683,22 @@ static int ax88772_init_mdio(struct usbnet *dev)
 	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;
+		goto mdio_register_err;
 	}
 
+	priv->phydev = mdiobus_get_phy(priv->mdio, priv->phy_addr);
+	if (!priv->phydev) {
+		netdev_err(dev->net, "Could not find PHY\n");
+		ret = -ENODEV;
+		goto mdio_phy_err;
+	}
+
+	return 0;
+
+mdio_phy_err:
+	mdiobus_unregister(priv->mdio);
+mdio_register_err:
+	mdiobus_free(priv->mdio);
 	return ret;
 }
 
@@ -701,13 +713,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");
-		ax88772_mdio_unregister(priv);
-		return -ENODEV;
-	}
-
 	ret = phylink_connect_phy(priv->phylink, priv->phydev);
 	if (ret) {
 		netdev_err(dev->net, "Could not connect PHY\n");
@@ -909,7 +914,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
 	priv->presvd_phy_bmcr = 0;
 	priv->presvd_phy_advertise = 0;
 
-	ret = ax88772_init_mdio(dev);
+	ret = ax88772_mdio_register(dev);
 	if (ret)
 		return ret;
 
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

Re: [PATCHv3 net 1/2] net: asix: fix modprobe "sysfs: cannot create duplicate filename"

[Jiri Pirko]

Wed, Mar 08, 2023 at 09:21:58PM CET, grundler@chromium.org 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:
>
>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: e532a096be0e ("net: usb: asix: ax88772: add phylib support")
>Reported-by: Anton Lundin <glance@acc.umu.se>
>Link: https://lore.kernel.org/netdev/20220623063649.GD23685@pengutronix.de/T/
>Tested-by: Eizan Miyamoto <eizan@chromium.org>
>Signed-off-by: Grant Grundler <grundler@chromium.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Jiri Pirko]

Wed, Mar 08, 2023 at 09:21:59PM CET, grundler@chromium.org wrote:
>Make asix driver consistent with other drivers (e.g. tg3 and r8169) which
>use mdiobus calls: setup and tear down be handled in one function each.
>
>Signed-off-by: Grant Grundler <grundler@chromium.org>

This is not fixing a bug. You should send it separatelly to net-next.

Re: [PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Grant Grundler]

On Thu, Mar 9, 2023 at 3:20 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Mar 08, 2023 at 09:21:59PM CET, grundler@chromium.org wrote:
> >Make asix driver consistent with other drivers (e.g. tg3 and r8169) which
> >use mdiobus calls: setup and tear down be handled in one function each.
> >
> >Signed-off-by: Grant Grundler <grundler@chromium.org>
>
> This is not fixing a bug. You should send it separatelly to net-next.

Jiro,
Thanks for reviewing both patches. Agreed that it's not fixing a bug.

The patch depends on the previous patch. It wouldn't apply on a
different branch.

I hope the maintainers can apply both to net-next and only apply the
first to net branch.

cheers,
grant

Re: [PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Andrew Lunn]

> I hope the maintainers can apply both to net-next and only apply the
> first to net branch.

Hi Grant

Please take a look at
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Please submit the first patch to net. Then wait a week for net to be
merged into net-next, and submit the second patch to net-next.

       Andrew

Re: [PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Grant Grundler]

On Thu, Mar 9, 2023 at 11:30 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > I hope the maintainers can apply both to net-next and only apply the
> > first to net branch.
>
> Hi Grant
>
> Please take a look at
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
>
> Please submit the first patch to net. Then wait a week for net to be
> merged into net-next, and submit the second patch to net-next.

Thanks Andrew!
I read maintainer-netdev.html when Jakub pointed me at it a few days
ago. He also instructed me to use "net" but didn't specify for the
second patch - so I assumed both patches.

I'll follow your instructions and repost to net-next once the first
patch has been merged.

cheers,
grant

>
>        Andrew

Re: [PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Jakub Kicinski]

On Thu, 9 Mar 2023 11:53:54 -0800 Grant Grundler wrote:
> On Thu, Mar 9, 2023 at 11:30 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > I hope the maintainers can apply both to net-next and only apply the
> > > first to net branch.  
> >
> > Hi Grant
> >
> > Please take a look at
> > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
> >
> > Please submit the first patch to net. Then wait a week for net to be
> > merged into net-next, and submit the second patch to net-next.  
> 
> Thanks Andrew!
> I read maintainer-netdev.html when Jakub pointed me at it a few days
> ago. He also instructed me to use "net" but didn't specify for the
> second patch - so I assumed both patches.

I did:

  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).

https://lore.kernel.org/all/20230307164736.37ecb2f9@kernel.org/

But the process is a bit confusing. I'll take patch 1 in now, please
repost patch with net-next on/after Friday March 17th.

Re: [PATCHv3 net 2/2] net: asix: init mdiobus from one function

[Jakub Kicinski]

On Thu, 9 Mar 2023 22:16:46 -0800 Jakub Kicinski wrote:
>  I'll take patch 1 in now,

Sorry, I need to take that back.

Don't the error paths in ax88772_bind() need to be updated to call
ax88772_mdio_unregister() now?