Hi, On Thu, Oct 31, 2013 at 04:02:20PM +0100, Johannes Stezenbach wrote: > On Wed, Oct 30, 2013 at 10:44:30PM +0100, Johannes Stezenbach wrote: > > On Wed, Oct 30, 2013 at 12:54:15PM -0500, Felipe Balbi wrote: > > > On Wed, Oct 30, 2013 at 03:17:46PM +0100, Johannes Stezenbach wrote: > > > > I'm testing dwc3 with g_zero and noticed that errors > > > > seem to stick even after SET_CONFIGURATION/SET_INTERFACE. > > > > > > > > In f_sourcesink.c, check_read_data() calls usb_ep_set_halt() > > > > if the data does not match the expected values, and > > > > sourcesink_set_alt() indirectly calls usb_ep_disable() > > > > and usb_ep_enable(). > > > > > > > > Now, include/linux/usb/gadget.h comments for usb_ep_enable() > > > > are not 100% clear wrt STALL, except "making it usable" > > > > sounds like it should clear it. However, usb_ep_set_halt() > > > > comments states: > > > > > > > > * Note that while an endpoint CLEAR_FEATURE will be invisible to the > > > > * gadget driver, a SET_INTERFACE will not be. To reset endpoints for the > > > > * current altsetting, see usb_ep_clear_halt(). When switching altsettings, > > > > * it's simplest to use usb_ep_enable() or usb_ep_disable() for the endpoints. > > > > > > > > That suggests to me that sourcesink_set_alt() is correct to > > > > simply call usb_ep_enable(), i.e. no explicit usb_ep_clear_halt() > > > > is needed? If so it would mean dwc3 ep_enable() is buggy. > > > > > > we clear those flags on usb_ep_disable(). You can't enable an endpoint > > > which is already enabled. And you can see that when you're going to > > > switch alt settings, f_sourcesink calls disable_source_sink() which will > > > call disable_endpoints() which will disable each and every endpoint, > > > thus clearing the such flags. > > > > > > Do you have any patches on f_sourcesink which might have caused this > > > bug ? > > > > No patches, but maybe out of date code. However, I'm looking > > at current git master, I see you clear dep->flags but I don't > > see any dwc3_send_gadget_ep_cmd(DWC3_DEPCMD_CLEARSTALL)? > > I just tried to add __dwc3_gadget_ep_set_halt(dep, false); > right before the call to __dwc3_gadget_ep_disable() > in dwc3_gadget_ep_disable(), it seems to fix the issue. > > It is easily reproducible using the attached Python script, it > should print: > > $ ./dwc3test.py > [Errno 32] Pipe error (expected) > OK > > but instead prints: > > $ ./dwc3test.py > [Errno 32] Pipe error (expected) > [Errno 32] Pipe error > > > Thanks, > Johannes > #!/usr/bin/env python2 > # g_zero speed test > # needs pyusb-1.0.0b1 from http://sourceforge.net/projects/pyusb/ > > import sys > sys.path.insert(0, "./pyusb-1.0.0b1") > > import usb.core > import usb.util > > # find Linux gadget zero > dev = usb.core.find(idVendor=0x0525, idProduct=0xa4a0) > if dev is None: > print 'Device not found' > sys.exit(1) > > # choose data sink + source configuration > dev.set_configuration(3) > > cfg = dev.get_active_configuration() > > # use first interface + alternate setting > intf = cfg[(0,0)] > > out_ep = usb.util.find_descriptor( > intf, > custom_match = lambda e: \ > usb.util.endpoint_direction(e.bEndpointAddress) == \ > usb.util.ENDPOINT_OUT) > > try: > out_ep.write("test") > out_ep.write("test") > except usb.core.USBError, e: > print e, "(expected)" > else: > print "error: didn't get the expected EPIPE" > > # SET_INTERFACE should clear STALL > intf.set_altsetting() > > try: > out_ep.write("\0" * 4) > except usb.core.USBError, e: > print e > else: > print "OK" can you send a patch to dwc3 so we can review and possibly apply ? -- balbi
Attachment:
signature.asc
Description: Digital signature
