Re: gadget: should usb_ep_enable() clear EP STALL?
Hi, On Fri, Nov 01, 2013 at 09:26:41AM +0100, Johannes Stezenbach wrote: Hi, On Thu, Oct 31, 2013 at 10:21:28AM -0500, Felipe Balbi wrote: 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: 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 can you send a patch to dwc3 so we can review and possibly apply ? I could create a patch, but I can't test it with the current upstream version because I'm stuck with an old 3.4.x kernel and dwc3 version (not under my control). So my idea was to make it reproducible for you so you can create the patch and test it. If you want you can add Reported-by: or Suggested-by:. All right, I'll add to my todo for this week ;-) -- balbi signature.asc Description: Digital signature
Re: gadget: should usb_ep_enable() clear EP STALL?
Hi, On Thu, Oct 31, 2013 at 10:21:28AM -0500, Felipe Balbi wrote: 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: 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 can you send a patch to dwc3 so we can review and possibly apply ? I could create a patch, but I can't test it with the current upstream version because I'm stuck with an old 3.4.x kernel and dwc3 version (not under my control). So my idea was to make it reproducible for you so you can create the patch and test it. If you want you can add Reported-by: or Suggested-by:. Thanks, Johannes -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadget: should usb_ep_enable() clear EP STALL?
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
Re: gadget: should usb_ep_enable() clear EP STALL?
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 signature.asc Description: Digital signature
gadget: should usb_ep_enable() clear EP STALL?
Hi, 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. Thanks, Johannes -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadget: should usb_ep_enable() clear EP STALL?
Hi, On Wed, Oct 30, 2013 at 03:17:46PM +0100, Johannes Stezenbach wrote: Hi, 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 ? -- balbi signature.asc Description: Digital signature
Re: gadget: should usb_ep_enable() clear EP STALL?
Hi, 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)? Thanks, Johannes -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
