Re: gadget: should usb_ep_enable() clear EP STALL?

[Johannes Stezenbach <js@xxxxxxxxx>]

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"