Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

[Felipe Balbi]

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