On 14.04.22 17:01, Andy Shevchenko wrote: > > Good question. Isn't it the commit 2c9d6c2b871d ("usbnet: run unbind() > before unregister_netdev()") which changed the ordering of the > interface shutdown and basically makes this race happen? I don't see > how we can guarantee that IOCTL won't be called until we quiescence > the network device — my understanding that on device surprise removal True. The best we could do is introduce a mutex for ioctl() and disconnect(). That seems the least preferable solution to me. > we have to first shutdown what it created and then unbind the device. > If I understand the original issue correctly then the problem is in > usbnet->unbind and it should actually be split to two hooks, otherwise > it seems every possible IOCTL callback must have some kind of > reference counting and keep an eye on the surprise removal. > > Johan, can you correct me if my understanding is wrong? > It seems to me that fundamentally the order of actions to handle a hotunplug must mirror the order in a hotplug. We can add more hooks if that turns out to be necessary for some drivers, but the basic reverse mirrored order must be supported and I very much favor restoring it as default. So I am afraid I have to ask again, whether anybody sees a fundamental issue with the attached patch, as opposed to it not being an elegant solution? It looks to me that we are in a fundamental disagreement on the correct order in this question and there is no productive way forward other than offering both ways. Regards Oliver
From 2e07ccbd1769889963d129ec474909bdcaa4c64a Mon Sep 17 00:00:00 2001 From: Oliver Neukum <oneukum@xxxxxxxx> Date: Thu, 10 Mar 2022 13:18:38 +0100 Subject: [PATCH] usbnet: split unbind callback Some devices need to be informed of a disconnect before the generic layer is informed, others need their notification later to avoid race conditions. Hence we provide two callbacks. Signed-off-by: Oliver Neukum <oneukum@xxxxxxxx> --- drivers/net/usb/asix_devices.c | 8 ++++---- drivers/net/usb/smsc95xx.c | 4 ++-- drivers/net/usb/usbnet.c | 7 +++++-- include/linux/usb/usbnet.h | 3 +++ 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c index 6ea44e53713a..e6cfa9a39a87 100644 --- a/drivers/net/usb/asix_devices.c +++ b/drivers/net/usb/asix_devices.c @@ -808,7 +808,7 @@ static int ax88772_stop(struct usbnet *dev) return 0; } -static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf) +static void ax88772_disable(struct usbnet *dev, struct usb_interface *intf) { struct asix_common_private *priv = dev->driver_priv; @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info = { static const struct driver_info ax88772_info = { .description = "ASIX AX88772 USB 2.0 Ethernet", .bind = ax88772_bind, - .unbind = ax88772_unbind, + .unbind = ax88772_disable, .status = asix_status, .reset = ax88772_reset, .stop = ax88772_stop, @@ -1226,7 +1226,7 @@ static const struct driver_info ax88772_info = { static const struct driver_info ax88772b_info = { .description = "ASIX AX88772B USB 2.0 Ethernet", .bind = ax88772_bind, - .unbind = ax88772_unbind, + .unbind = ax88772_disable, .status = asix_status, .reset = ax88772_reset, .stop = ax88772_stop, @@ -1262,7 +1262,7 @@ static const struct driver_info ax88178_info = { static const struct driver_info hg20f9_info = { .description = "HG20F9 USB 2.0 Ethernet", .bind = ax88772_bind, - .unbind = ax88772_unbind, + .unbind = ax88772_disable, .status = asix_status, .reset = ax88772_reset, .flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_LINK_INTR | diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 5567220e9d16..62db57021f5f 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -1211,7 +1211,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf) return ret; } -static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf) +static void smsc95xx_disable(struct usbnet *dev, struct usb_interface *intf) { struct smsc95xx_priv *pdata = dev->driver_priv; @@ -1985,7 +1985,7 @@ static int smsc95xx_manage_power(struct usbnet *dev, int on) static const struct driver_info smsc95xx_info = { .description = "smsc95xx USB 2.0 Ethernet", .bind = smsc95xx_bind, - .unbind = smsc95xx_unbind, + .unbind = smsc95xx_disable, .link_reset = smsc95xx_link_reset, .reset = smsc95xx_reset, .check_connect = smsc95xx_start_phy, diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index b1f93810a6f3..5249a7d7efa5 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1641,8 +1641,8 @@ void usbnet_disconnect (struct usb_interface *intf) xdev->bus->bus_name, xdev->devpath, dev->driver_info->description); - if (dev->driver_info->unbind) - dev->driver_info->unbind(dev, intf); + if (dev->driver_info->disable) + dev->driver_info->disable(dev, intf); net = dev->net; unregister_netdev (net); @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf) usb_scuttle_anchored_urbs(&dev->deferred); + if (dev->driver_info->unbind) + dev->driver_info->unbind (dev, intf); + usb_kill_urb(dev->interrupt); usb_free_urb(dev->interrupt); kfree(dev->padding_pkt); diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h index 8336e86ce606..4d2407f1ae93 100644 --- a/include/linux/usb/usbnet.h +++ b/include/linux/usb/usbnet.h @@ -129,6 +129,9 @@ struct driver_info { /* cleanup device ... can sleep, but can't fail */ void (*unbind)(struct usbnet *, struct usb_interface *); + /* disable device ... can sleep, but can't fail */ + void (*disable)(struct usbnet *, struct usb_interface *); + /* reset device ... can sleep */ int (*reset)(struct usbnet *); -- 2.34.1