Re: [PATCH] USB: DWC3: Restart setup phase correctly if gadget halts the transfer

[Felipe Balbi <balbi@xxxxxx>]

Hi,

On Mon, Jun 25, 2012 at 07:16:56PM +0530, Pratyush Anand wrote:
> If a gadget halts control transfer stage, then that stage is stalled,
> but core should be ready to receive next setup packet.
> 
> This patch restarts control transfer along with other software correct
> flag settings.
> 
> Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx>
> Signed-off-by: Michel Sanches <michel.sanches@xxxxxx>

have you actually experienced any issues ? I ask this because if you
look at dwc3_ep0_stall_and_restart() you will see that this is properly
handled:

208 static void dwc3_ep0_stall_and_restart(struct dwc3 *dwc)
209 {
210         struct dwc3_ep          *dep = dwc->eps[0];
211
212         /* stall is always issued on EP0 */
213         __dwc3_gadget_ep_set_halt(dep, 1);
214         dep->flags = DWC3_EP_ENABLED;
215         dwc->delayed_status = false;
216
217         if (!list_empty(&dep->request_list)) {
218                 struct dwc3_request     *req;
219
220                 req = next_request(&dep->request_list);
221                 dwc3_gadget_giveback(dep, req, -ECONNRESET);
222         }
223
224         dwc->ep0state = EP0_SETUP_PHASE;
225         dwc3_ep0_out_start(dwc);
226 }

We could even apply this patch:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d34ed40..835c918 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1210,15 +1210,6 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value)
 	memset(&params, 0x00, sizeof(params));
 
 	if (value) {
-		if (dep->number == 0 || dep->number == 1) {
-			/*
-			 * Whenever EP0 is stalled, we will restart
-			 * the state machine, thus moving back to
-			 * Setup Phase
-			 */
-			dwc->ep0state = EP0_SETUP_PHASE;
-		}
-
 		ret = dwc3_send_gadget_ep_cmd(dwc, dep->number,
 			DWC3_DEPCMD_SETSTALL, &params);
 		if (ret)

because that part if also handled dwc3_ep0_stall_and_restart().

All in all, I fail to see the need for this patch. I can see, however,
the patch causing a problem because we will be issuing two Start
Transfer commands to ep0 by way of dwc3_ep0_out_start()

If you have found any issues, I don't think this is the failing
function, but I would need to see logs and an easy way to reproduce the
failure ;-)

-- 
balbi

Attachment: signature.asc
Description: Digital signature