"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 <[email protected]>
Link: https://lore.kernel.org/netdev/[email protected]/T/
Tested-by: Eizan Miyamoto <[email protected]>
Signed-off-by: Grant Grundler <[email protected]>
---
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
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 <[email protected]>
---
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
Wed, Mar 08, 2023 at 09:21:58PM CET, [email protected] 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 <[email protected]>
>Link: https://lore.kernel.org/netdev/[email protected]/T/
>Tested-by: Eizan Miyamoto <[email protected]>
>Signed-off-by: Grant Grundler <[email protected]>
Reviewed-by: Jiri Pirko <[email protected]>
Wed, Mar 08, 2023 at 09:21:59PM CET, [email protected] 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 <[email protected]>
This is not fixing a bug. You should send it separatelly to net-next.
On Thu, Mar 9, 2023 at 3:20 AM Jiri Pirko <[email protected]> wrote:
>
> Wed, Mar 08, 2023 at 09:21:59PM CET, [email protected] 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 <[email protected]>
>
> 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
> 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
On Thu, Mar 9, 2023 at 11:30 AM Andrew Lunn <[email protected]> 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
On Thu, 9 Mar 2023 11:53:54 -0800 Grant Grundler wrote:
> On Thu, Mar 9, 2023 at 11:30 AM Andrew Lunn <[email protected]> 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/[email protected]/
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.
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?