From patchwork Thu Aug 20 15:35:53 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Felipe Balbi X-Patchwork-Id: 7045431 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 661F29F373 for ; Thu, 20 Aug 2015 15:39:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4F201205D4 for ; Thu, 20 Aug 2015 15:39:15 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DA600205D3 for ; Thu, 20 Aug 2015 15:39:13 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZSRtH-0001Ge-8R; Thu, 20 Aug 2015 15:36:31 +0000 Received: from arroyo.ext.ti.com ([192.94.94.40]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZSRtD-0001F9-CW; Thu, 20 Aug 2015 15:36:28 +0000 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by arroyo.ext.ti.com (8.13.7/8.13.7) with ESMTP id t7KFZtIR031569; Thu, 20 Aug 2015 10:35:55 -0500 Received: from DFLE72.ent.ti.com (dfle72.ent.ti.com [128.247.5.109]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id t7KFZtTs014176; Thu, 20 Aug 2015 10:35:55 -0500 Received: from dlep33.itg.ti.com (157.170.170.75) by DFLE72.ent.ti.com (128.247.5.109) with Microsoft SMTP Server id 14.3.224.2; Thu, 20 Aug 2015 10:35:55 -0500 Received: from localhost (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep33.itg.ti.com (8.14.3/8.13.8) with ESMTP id t7KFZrLp004049; Thu, 20 Aug 2015 10:35:54 -0500 Date: Thu, 20 Aug 2015 10:35:53 -0500 From: Felipe Balbi To: Robert Baldyga Subject: Re: [PATCH v5 01/46] usb: gadget: encapsulate endpoint claiming mechanism Message-ID: <20150820153553.GD1639@saruman.tx.rr.com> References: <1438351258-31578-1-git-send-email-r.baldyga@samsung.com> <1438351258-31578-2-git-send-email-r.baldyga@samsung.com> MIME-Version: 1.0 In-Reply-To: <1438351258-31578-2-git-send-email-r.baldyga@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150820_083627_574048_274AC021 X-CRM114-Status: GOOD ( 21.66 ) X-Spam-Score: -7.5 (-------) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: balbi@ti.com Cc: nicolas.ferre@atmel.com, robert.jarzmik@free.fr, m.szyprowski@samsung.com, devel@driverdev.osuosl.org, cernekee@gmail.com, michal.simek@xilinx.com, stern@rowland.harvard.edu, Peter.Chen@freescale.com, dahlmann.thomas@arcor.de, leoli@freescale.com, johnyoun@synopsys.com, linux-geode@lists.infradead.org, haojian.zhuang@gmail.com, andrzej.p@samsung.com, linux-omap@vger.kernel.org, petr.cvek@tul.cz, linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, balbi@ti.com, linuxppc-dev@lists.ozlabs.org, daniel@zonque.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On Fri, Jul 31, 2015 at 04:00:13PM +0200, Robert Baldyga wrote: > So far it was necessary for usb functions to set ep->driver_data in > endpoint obtained from autoconfig to non-null value, to indicate that > endpoint is claimed by function (in autoconfig it was checked if endpoint > has set this field to non-null value, and if it has, it was assumed that > it is claimed). It could cause bugs because if some function doesn't > set this field autoconfig could return the same endpoint more than one > time. > > To help to avoid such bugs this patch adds claimed flag to struct usb_ep, > and encapsulates endpoint claiming mechanism inside usb_ep_autoconfig_ss() > and usb_ep_autoconfig_reset(), so now usb functions don't need to perform > any additional actions to mark endpoint obtained from autoconfig as claimed. > > Signed-off-by: Robert Baldyga just letting you know that this regresses all gadget drivers making them try to disable previously disabled endpoints and enable previously enabled endpoints. I have a possible fix (see below) but then it shows a problem on the host side when using with g_zero (see further below): commit 3b8932100aacb6cfbffe288ca93025d8b8430c00 Author: Felipe Balbi Date: Wed Aug 19 18:05:27 2015 -0500 usb: gadget: fix ep->claimed lifetime In order to fix a regression introduced by commit cc476b42a39d ("usb: gadget: encapsulate endpoint claiming mechanism") we have to introduce a simple helper to check if a particular is enabled or not. After that, we need to move ep->claimed lifetime to usb_ep_enable() and usb_ep_disable() since those are the only functions which actually enable and disable endpoints. A follow-up patch will come to drop all driver_data checks from function drivers, since those are, now, pointless. Fixes: cc476b42a39d ("usb: gadget: encapsulate endpoint claiming mechanism") Cc: Robert Baldyga Signed-off-by: Felipe Balbi [ 73.290345] WARNING: CPU: 0 PID: 300 at lib/kobject.c:240 kobject_add_internal+0x25c/0x2d8() [ 73.299172] kobject_add_internal failed for ep_81 with -EEXIST, don't try to register things with the same name in the same directory. [ 73.311825] Modules linked in: usbtest usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore joydev dwc3 udc_core usb_common m25p80 evdev spi_nor omapfb cpufreq_dt cfbfillrect snd_soc_simple_card cfbimgblt thermal_sys cfbcopyarea hwmon leds_gpio matrix_keypad led_class matrix_keymap panel_dpi pwm_bl snd_soc_tlv320aic3x snd_soc_davinci_mcasp snd_soc_edma snd_soc_omap snd_soc_core omapdss snd_compress snd_pcm_dmaengine snd_pcm dwc3_omap extcon snd_timer pwm_tiecap snd lis3lv02d_i2c lis3lv02d soundcore input_polldev spi_ti_qspi tps65218_pwrbutton rtc_omap omap_wdt phy_omap_usb2 autofs4 [ 73.367114] CPU: 0 PID: 300 Comm: testusb Tainted: G W 4.2.0-rc7-next-20150819-00002-g3ccb6c8b6305 #1080 [ 73.378236] Hardware name: Generic AM43 (Flattened Device Tree) [ 73.384439] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 73.392566] [] (show_stack) from [] (dump_stack+0x84/0x9c) [ 73.400164] [] (dump_stack) from [] (warn_slowpath_common+0x78/0xb4) [ 73.408649] [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x30/0x40) [ 73.417783] [] (warn_slowpath_fmt) from [] (kobject_add_internal+0x25c/0x2d8) [ 73.427063] [] (kobject_add_internal) from [] (kobject_add+0x4c/0x98) [ 73.435641] [] (kobject_add) from [] (device_add+0xd4/0x56c) [ 73.443466] [] (device_add) from [] (usb_create_ep_devs+0x60/0xac [usbcore]) [ 73.452788] [] (usb_create_ep_devs [usbcore]) from [] (create_intf_ep_devs+0x48/0x70 [usbcore]) [ 73.463801] [] (create_intf_ep_devs [usbcore]) from [] (usb_set_interface+0x1c4/0x2f4 [usbcore]) [ 73.474891] [] (usb_set_interface [usbcore]) from [] (usbtest_ioctl+0x840/0x14c8 [usbtest]) [ 73.485659] [] (usbtest_ioctl [usbtest]) from [] (usbdev_ioctl+0x1728/0x17fc [usbcore]) [ 73.495947] [] (usbdev_ioctl [usbcore]) from [] (do_vfs_ioctl+0x3f4/0x6c4) [ 73.504989] [] (do_vfs_ioctl) from [] (SyS_ioctl+0x6c/0x7c) [ 73.512662] [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x54) [ 73.520605] ---[ end trace 4576b2ea698fb13f ]--- diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c index 978435a51038..ad45070cd76f 100644 --- a/drivers/usb/gadget/epautoconf.c +++ b/drivers/usb/gadget/epautoconf.c @@ -126,7 +126,6 @@ found_ep: ep->address = desc->bEndpointAddress; ep->desc = NULL; ep->comp_desc = NULL; - ep->claimed = true; return ep; } EXPORT_SYMBOL_GPL(usb_ep_autoconfig_ss); @@ -182,11 +181,6 @@ EXPORT_SYMBOL_GPL(usb_ep_autoconfig); */ void usb_ep_autoconfig_reset (struct usb_gadget *gadget) { - struct usb_ep *ep; - - list_for_each_entry (ep, &gadget->ep_list, ep_list) { - ep->claimed = false; - } gadget->in_epnum = 0; gadget->out_epnum = 0; } diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index c14a69b36d27..9b3d60c1cf9f 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -243,6 +243,22 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep, } /** + * usb_ep_enabled - is endpoint enabled ? + * @ep: the endpoint being checked. may not be the endpoint named "ep0". + * + * Whenever a function driver wants to check if a particular endpoint is + * enabled or not, it must check using this helper function. This will + * encapsulate details about how the endpoint is checked, saving the function + * driver from using private methods for doing so. + * + * return true if endpoint is enabled, false otherwise. + */ +static inline bool usb_ep_enabled(struct usb_ep *ep) +{ + return ep->claimed; +} + +/** * usb_ep_enable - configure endpoint, making it usable * @ep:the endpoint being configured. may not be the endpoint named "ep0". * drivers discover endpoints through the ep_list of a usb_gadget. @@ -264,7 +280,18 @@ static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep, */ static inline int usb_ep_enable(struct usb_ep *ep) { - return ep->ops->enable(ep, ep->desc); + int ret; + + if (usb_ep_enabled(ep)) + return 0; + + ret = ep->ops->enable(ep, ep->desc); + if (ret) + return ret; + + ep->claimed = true; + + return 0; } /** @@ -281,7 +308,18 @@ static inline int usb_ep_enable(struct usb_ep *ep) */ static inline int usb_ep_disable(struct usb_ep *ep) { - return ep->ops->disable(ep); + int ret; + + if (!usb_ep_enabled(ep)) + return 0; + + ret = ep->ops->disable(ep); + if (ret) + return ret; + + ep->claimed = false; + + return 0; } /**