On Fri, Jun 22, 2012 at 9:42 PM, Bjørn Mork <bjorn@xxxxxxx> wrote: > 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 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 -- 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
