Ming Lei <tom.leiming@xxxxxxxxx> writes: > On Fri, Jun 22, 2012 at 5:11 PM, Bjørn Mork <bjorn@xxxxxxx> 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 linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
