On Mon, Jun 25, 2012 at 2:15 PM, Oliver Neukum <oliver@xxxxxxxxxx> wrote: > Am Montag, 25. Juni 2012, 05:37:20 schrieb Ming Lei: >> The current problem is caused by the set to NULL without any >> protection or sync mechanism on it, and it is really a bug. > > Minidrivers can test for NULL. > That may not be enough and locking may be needed. Any locking isn't needed if the set to NULL is put after driver_info->unbind, since ->unbind will call subdriver->disconnect, which will hold the open/disconnect lock of wdm_mutex. > >> Also we didn't say the set to NULL will be cancelled, just delay >> the clear until it is safe to do it, eg. after complete of unregister_netdev() >> and driver_info->unbind, when the previous .open/.close has been >> completed already or the later ones will notice the disconnection >> early and won't touch usb_get_intfdata() any more. > > We can move to after unregister_netdev() > I am unhappy with it going after unbind. > Could you let us know the reason? I think it may let the patch not necessary. >> I don't see any races caused by just removing usb_set_intfdata(, NULL) >> or moving it after driver_info->unbind simply in usbnet_disconnect. > > They are not caused. They are harder to detect. > >> In fact, suppose that .open/.close and .disconnect are run on different CPUs, >> there are no any guarantee that .open/.close can always see the clear action >> immediately. Also, the clear of intfdata may not be observed in .manage_power >> since usb_set_intfdata(, NULL) may be completed after the lock wdm_mutex >> operation. > > Sure, it is a debugging aid. It has the drawback that minidrivers have > to be able to deal with intfdata being NULL. That is not hard to do. The check isn't needed if the set to NULL is put after driver_info->unbind in usbnet_disconnect. I think we can add the comment on it to avoid the unnecessary check. 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
