Am Montag, 25. Juni 2012, 05:37:20 schrieb Ming Lei: > 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. Minidrivers can test for NULL. That may not be enough and locking may be needed. > 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. > > 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. 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. Regards Oliver -- 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
