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 <r.baldyga@xxxxxxxxxxx>
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 <balbi@xxxxxx>
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 <r.baldyga@xxxxxxxxxxx>
Signed-off-by: Felipe Balbi <balbi@xxxxxx>
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;
}
/**
[ 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] [<c0016f14>] (unwind_backtrace) from [<c001339c>] (show_stack+0x10/0x14)
[ 73.392566] [<c001339c>] (show_stack) from [<c0636cec>] (dump_stack+0x84/0x9c)
[ 73.400164] [<c0636cec>] (dump_stack) from [<c003db48>] (warn_slowpath_common+0x78/0xb4)
[ 73.408649] [<c003db48>] (warn_slowpath_common) from [<c003dbb4>] (warn_slowpath_fmt+0x30/0x40)
[ 73.417783] [<c003dbb4>] (warn_slowpath_fmt) from [<c03473e4>] (kobject_add_internal+0x25c/0x2d8)
[ 73.427063] [<c03473e4>] (kobject_add_internal) from [<c03474ac>] (kobject_add+0x4c/0x98)
[ 73.435641] [<c03474ac>] (kobject_add) from [<c03d8a58>] (device_add+0xd4/0x56c)
[ 73.443466] [<c03d8a58>] (device_add) from [<bf2c17b0>] (usb_create_ep_devs+0x60/0xac [usbcore])
[ 73.452788] [<bf2c17b0>] (usb_create_ep_devs [usbcore]) from [<bf2bb210>] (create_intf_ep_devs+0x48/0x70 [usbcore])
[ 73.463801] [<bf2bb210>] (create_intf_ep_devs [usbcore]) from [<bf2bbae0>] (usb_set_interface+0x1c4/0x2f4 [usbcore])
[ 73.474891] [<bf2bbae0>] (usb_set_interface [usbcore]) from [<bf33d370>] (usbtest_ioctl+0x840/0x14c8 [usbtest])
[ 73.485659] [<bf33d370>] (usbtest_ioctl [usbtest]) from [<bf2c569c>] (usbdev_ioctl+0x1728/0x17fc [usbcore])
[ 73.495947] [<bf2c569c>] (usbdev_ioctl [usbcore]) from [<c0185764>] (do_vfs_ioctl+0x3f4/0x6c4)
[ 73.504989] [<c0185764>] (do_vfs_ioctl) from [<c0185aa0>] (SyS_ioctl+0x6c/0x7c)
[ 73.512662] [<c0185aa0>] (SyS_ioctl) from [<c000f540>] (ret_fast_syscall+0x0/0x54)
[ 73.520605] ---[ end trace 4576b2ea698fb13f ]---
--
balbi
Attachment:
signature.asc
Description: Digital signature
