On Mon, Jun 25, 2012 at 1:47 AM, Bjørn Mork <bjorn@xxxxxxx> wrote: > Oliver Neukum <oliver@xxxxxxxxxx> wrote: >>Am Sonntag, 24. Juni 2012, 11:34:19 schrieb Bjørn Mork: >> >>> Sorry, I did not understand what you meant we should do here. The >>extra >>> usb_set_intfdata(, NULL) in usbnet_disconnect() won't make any >>> difference for that piece of code, will it? >> >>The point is that if it may be set to NULL, we always want it to be set >>to >>NULL, so we catch bugs. >> >>> And the USB core ensures that intfdata is set to NULL before any >>> reprobing, so that will never be a problem. That's the reason why it >>> seems redundant setting it in usbnet_disconnect(). >> >>The point is that if there is a problem because intfdata is set to >>NULL, >>there is very likely a problem in form of a race condition, if intfdata >>were not set to NULL in usbnet's disconnect handler. I don't agree on the assumption. The current problem is caused by the set to NULL without any protection or sync mechanism on it, and it is really a bug. 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. > Thanks for explaining. Yes, that makes sense to me as well. > > So then the original patch against qmi_wwan should go in, and we should leave usbnet as it is. Are everyone comfortable with that? 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. 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. So it is only the sync mechanism that works on the race even the check is added in the patch. Putting usb_set_intfdata(, NULL) after driver_info->unbind should be OK, and it is a general solution for the 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
