Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752090AbbF0Whe (ORCPT ); Sat, 27 Jun 2015 18:37:34 -0400 Received: from mail-ie0-f171.google.com ([209.85.223.171]:35230 "EHLO mail-ie0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751817AbbF0Wh1 (ORCPT ); Sat, 27 Jun 2015 18:37:27 -0400 MIME-Version: 1.0 In-Reply-To: References: <1435010474-13419-6-git-send-email-ruslan.bilovol@gmail.com> Date: Sun, 28 Jun 2015 01:37:25 +0300 Message-ID: Subject: Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers From: Ruslan Bilovol To: Alan Stern Cc: "Balbi, Felipe" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Krzysztof Opasiak , Peter Chen , "gregkh@linuxfoundation.org" , Andrzej Pietrasiewicz , Maxime Ripard Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3302 Lines: 75 Hi Alan, On Tue, Jun 23, 2015 at 5:08 PM, Alan Stern wrote: > On Tue, 23 Jun 2015, Ruslan Bilovol wrote: > >> Change behavior during registration of gadgets and >> gadget drivers in udc-core. Instead of previous >> approach when for successful probe of usb gadget driver >> at least one usb gadget should be already registered >> use another one where gadget drivers and gadgets >> can be registered in udc-core independently. >> >> Independent registration of gadgets and gadget drivers >> is useful for built-in into kernel gadget and gadget >> driver case - because it's possible that gadget is >> really probed only on late_init stage (due to deferred >> probe) whereas gadget driver's probe is silently failed >> on module_init stage due to no any UDC added. >> >> Also it is useful for modules case - now there is no >> difference what module to insert first: gadget module >> or gadget driver one. >> >> Tested-by: Maxime Ripard >> Signed-off-by: Ruslan Bilovol > >> @@ -484,6 +507,16 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) >> break; >> } >> >> + if (ret) { >> + struct usb_gadget_driver *tmp; >> + >> + list_for_each_entry(tmp, &gadget_driver_pending_list, pending) >> + if (tmp == driver) { >> + list_del(&driver->pending); >> + ret = 0; >> + break; >> + } >> + } > > 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. 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. Best regards, Ruslan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/