On Sat, Jun 27, 2015 at 07:47:28PM -0400, Alan Stern wrote: > On Sun, 28 Jun 2015, Ruslan Bilovol wrote: > > > > Weren't you going to replace this loop with a simple list_del()? IIRC, > > > this is the third time I have asked you to make this change. > > > > I understand the improvement that replacing this loop with a list_del() > > may bring for us, but I disagree with doing it in this particular case. > > > > The reason is simple. The usb_gadget_unregister_driver() funciton is > > externally visible so we can get junk as input. Current implementation > > checks passed pointer and only after that does list_del(), or > > returns -EINVAL. Your variant will do list_del() unconditionally, that > > may cause a kernel crash or unexpected behavior in case of junk > > passed with *driver. The list_del_init() usage can't help here since > > there is no way to check that list_head structure is initialized with correct > > data or contains junk. > > That's right. > > > There is no noticeable performance loss with current implementation,just > > because current use case is pretty simple: one gadget driver per one UDC, > > and usually there is only one UDC per machine (or rare cases with few > > UDCs), thus number of pending gadget drivers is relatively small. > > We can return back to this discussion if someone needs to register > > many gadget drivers, and want to improve performance, because > > there are few existing places (not created by me) in this file that uses > > same approach of walking through list of registered gadget drivers. > > > > As a bottom line, choosing between stability and little performance > > improvement, I prefer stability. > > It's not really a question of code size or performance. As you say, > the difference in each is minimal. > > It _is_ a question of style. Adding unnecessary code to check for > something that shouldn't need to be checked looks bad. Other kernel > developers reading that code will notice it and wonder why it's there. > That's the argument for getting rid of the loop. yeah, if someone gives us junk as input, they deserve to oops. list_del() is enough. -- balbi
Attachment:
signature.asc
Description: Digital signature
