| Message ID | CACVXFVPkC8NtvOzTsOyNcftZy60fDjtEkV_Da1Z32iYpHFHcUw@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Ming Lei <tom.leiming@gmail.com> writes: > On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote: >> usbnet_disconnect() will set intfdata to NULL before calling >> the minidriver unbind function. The cdc_wdm subdriver cannot >> know that it is disconnecting until the qmi_wwan unbind >> function has called its disconnect function. This means that >> we must be able to support the cdc_wdm subdriver operating >> normally while usbnet_disconnect() is running, and in >> particular that intfdata may be NULL. >> >> The only place this matters is in qmi_wwan_cdc_wdm_manage_power >> which is called from cdc_wdm. Simply testing for NULL >> intfdata there is sufficient to allow it to continue working >> at all times. >> >> Fixes this Oops where a cdc-wdm device was closed while the >> USB device was disconnecting, causing wdm_release to call >> qmi_wwan_cdc_wdm_manage_power after intfdata was set to >> NULL by usbnet_disconnect: > > In fact, it should be a general problem in usbnet, there are races > between usbnet_disconnect and .open/.close. Considered that > unregister_netdev, which will sync with .open/.close, is called in > usbnet_disconnect, the simplest fix is to move usb_set_intfdata(NULL) > after unregister_netdev. Is there really a race there? The usbnet .open/.close don't use the intfdata, do they? I looked briefly through usbnet for related potentional problems while fixing this in qmi_wwan, but could only find suspend/resume. Which I believe are protected against running on disconnect. So I think usbnet in general is OK. > Considered that usb_set_intfdata(NULL) will be called after > executing .disconnect in unbind path, it can be removed simply. > > So how about the below? > > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index e92c057..2eb9e1e 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -1286,7 +1286,6 @@ void usbnet_disconnect (struct usb_interface *intf) > struct net_device *net; > > dev = usb_get_intfdata(intf); > - usb_set_intfdata(intf, NULL); > if (!dev) > return; I believe that call is there to prevent disconnect from running twice for the common two-interface CDC ethernet model, like e.g. cdc_ether. So I don't think it can be removed. Not without touching the minidrivers depending on that behaviour, anyway. Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 22, 2012 at 9:42 PM, Bjørn Mork <bjorn@mork.no> wrote: > Ming Lei <tom.leiming@gmail.com> writes: > >> On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@mork.no> wrote: >>> usbnet_disconnect() will set intfdata to NULL before calling >>> the minidriver unbind function. The cdc_wdm subdriver cannot >>> know that it is disconnecting until the qmi_wwan unbind >>> function has called its disconnect function. This means that >>> we must be able to support the cdc_wdm subdriver operating >>> normally while usbnet_disconnect() is running, and in >>> particular that intfdata may be NULL. >>> >>> The only place this matters is in qmi_wwan_cdc_wdm_manage_power >>> which is called from cdc_wdm. Simply testing for NULL >>> intfdata there is sufficient to allow it to continue working >>> at all times. >>> >>> Fixes this Oops where a cdc-wdm device was closed while the >>> USB device was disconnecting, causing wdm_release to call >>> qmi_wwan_cdc_wdm_manage_power after intfdata was set to >>> NULL by usbnet_disconnect: >> >> In fact, it should be a general problem in usbnet, there are races >> between usbnet_disconnect and .open/.close. Considered that >> unregister_netdev, which will sync with .open/.close, is called in >> usbnet_disconnect, the simplest fix is to move usb_set_intfdata(NULL) >> after unregister_netdev. > > Is there really a race there? The usbnet .open/.close don't use the > intfdata, do they? I looked briefly through usbnet for related Suppose intfdata is not used in .open/.close, there are no the race. > potentional problems while fixing this in qmi_wwan, but could only find > suspend/resume. Which I believe are protected against running on > disconnect. > > So I think usbnet in general is OK. Yes. Looks I understand the problem incorrectly, your problem is that qmi_wwan_cdc_wdm_manage_power is called from wdm_open, not from usbnet_open. It is a problem crossing 2 class drivers. > >> Considered that usb_set_intfdata(NULL) will be called after >> executing .disconnect in unbind path, it can be removed simply. >> >> So how about the below? >> >> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c >> index e92c057..2eb9e1e 100644 >> --- a/drivers/net/usb/usbnet.c >> +++ b/drivers/net/usb/usbnet.c >> @@ -1286,7 +1286,6 @@ void usbnet_disconnect (struct usb_interface *intf) >> struct net_device *net; >> >> dev = usb_get_intfdata(intf); >> - usb_set_intfdata(intf, NULL); >> if (!dev) >> return; > > I believe that call is there to prevent disconnect from running twice > for the common two-interface CDC ethernet model, like e.g. cdc_ether. No, intfdata is per interface, and .unbind will clear it for each interface. > So I don't think it can be removed. Not without touching the It can be removed, but not necessary since it can't fix your problem. Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > Looks I understand the problem incorrectly, your problem is > that qmi_wwan_cdc_wdm_manage_power is called from > wdm_open, not from usbnet_open. It is a problem crossing > 2 class drivers. That's correct. Trying to use two drivers at once will necessarily lead to the situation where one drivers shutdown is called from the other drivers shutdown. And the fact that one of the drivers is a usbnet minidriver complicates this a bit. But I believe most of it should be OK now. I tried my best to let the two drivers share as little as possible. The only shared state is in the power management code, which have to access the active/idle state of both drivers. >>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c >>> index e92c057..2eb9e1e 100644 >>> --- a/drivers/net/usb/usbnet.c >>> +++ b/drivers/net/usb/usbnet.c >>> @@ -1286,7 +1286,6 @@ void usbnet_disconnect (struct usb_interface *intf) >>> struct net_device *net; >>> >>> dev = usb_get_intfdata(intf); >>> - usb_set_intfdata(intf, NULL); >>> if (!dev) >>> return; >> >> I believe that call is there to prevent disconnect from running twice >> for the common two-interface CDC ethernet model, like e.g. cdc_ether. > > No, intfdata is per interface, and .unbind will clear it for each interface. Yes, intfdata is per interface, but in the case of cdc_ether (and probably other similar minidrivers) there are two interfaces pointing to the *same* usbnet private data. What about the situation where disconnect is called simultaneously for both interfaces? Or can't that happen? If it can, then we'll do driver->disconnect(intf1) driver->disconnect(intf2) dev = usb_get_intfdata(intf1) dev = usb_get_intfdata(intf2) dev->driver_info->unbind() dev->driver_info->unbind() net = dev->net net = dev->net free_netdev(net) free_netdev(dev->net) where "dev" and "net" will be pointing to the same private data and netdevice. I assume that is what the code above is trying to protect against. But I could be wrong. That has happened before, believe it or not :-) Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 23, 2012 at 12:18 AM, Bjørn Mork <bjorn@mork.no> wrote: > Ming Lei <tom.leiming@gmail.com> writes: > Yes, intfdata is per interface, but in the case of cdc_ether (and > probably other similar minidrivers) there are two interfaces pointing to > the *same* usbnet private data. Yes, the priv pointers of both interface points to the usbnet instance, but the same pointer is stored into two places. > > What about the situation where disconnect is called simultaneously for > both interfaces? Or can't that happen? If it can, then we'll do It can't happen because both parent lock and its lock need to be held in hotplug situation or unbind situation. > > driver->disconnect(intf1) driver->disconnect(intf2) > dev = usb_get_intfdata(intf1) dev = usb_get_intfdata(intf2) > dev->driver_info->unbind() dev->driver_info->unbind() > net = dev->net net = dev->net > free_netdev(net) free_netdev(dev->net) > > where "dev" and "net" will be pointing to the same private data and > netdevice. Suppose driver->disconnect(intf1) is called first, .ubind() inside .disconnect will clear intfdata of another interface(intf2) and call usb_driver_release_interface(intf2), which will cause .disconnect(intf2) called, but it will return immediately. > > I assume that is what the code above is trying to protect against. But I don't see the protection in the code, :-) > I could be wrong. That has happened before, believe it or not :-) Could you provide a link or commit about it? Thanks,
Ming Lei <tom.leiming@gmail.com> writes: > On Sat, Jun 23, 2012 at 12:18 AM, Bjørn Mork <bjorn@mork.no> wrote: >> Ming Lei <tom.leiming@gmail.com> writes: > >> Yes, intfdata is per interface, but in the case of cdc_ether (and >> probably other similar minidrivers) there are two interfaces pointing to >> the *same* usbnet private data. > > Yes, the priv pointers of both interface points to the usbnet instance, but > the same pointer is stored into two places. > >> >> What about the situation where disconnect is called simultaneously for >> both interfaces? Or can't that happen? If it can, then we'll do > > It can't happen because both parent lock and its lock need to be held in > hotplug situation or unbind situation. OK, then it can probably go away. But then again it doesn't do any harm for any other minidriver. qmi_wwan is special because of the cooperation with cdc_wdm. >> driver->disconnect(intf1) driver->disconnect(intf2) >> dev = usb_get_intfdata(intf1) dev = usb_get_intfdata(intf2) >> dev->driver_info->unbind() dev->driver_info->unbind() >> net = dev->net net = dev->net >> free_netdev(net) free_netdev(dev->net) >> >> where "dev" and "net" will be pointing to the same private data and >> netdevice. > > Suppose driver->disconnect(intf1) is called first, .ubind() inside > .disconnect will clear intfdata of another interface(intf2) and call > usb_driver_release_interface(intf2), which will cause .disconnect(intf2) > called, but it will return immediately. Yes, if that's the only possible call sequence then I agree that removing the call won't be a problem. But I don't see much gain either. And it has been there since the beginning of git history, if that counts for anything around here. >> I assume that is what the code above is trying to protect against. But > > I don't see the protection in the code, :-) I am trying my best to be positive :-) Bjørn -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index e92c057..2eb9e1e 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1286,7 +1286,6 @@ void usbnet_disconnect (struct usb_interface *intf) struct net_device *net; dev = usb_get_intfdata(intf); - usb_set_intfdata(intf, NULL); if (!dev) return;