Re: [PATCH] usb: dwc3: Skip TRBs while removing requests in disconnect path

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Wed, May 31, 2023, Elson Serrao wrote:
> 
> 
> On 5/31/2023 4:20 PM, Thinh Nguyen wrote:
> > On Wed, May 31, 2023, Elson Roy Serrao wrote:
> > > Consider a scenario where cable disconnect happens when there is an active
> > > usb reqest queued to the UDC. As part of the disconnect we would issue an
> > > end transfer with no interrupt-on-completion before giving back this
> > > request. Since we are giving back the request without skipping TRBs the
> > > num_trbs field of dwc3_request still holds the stale value previously used.
> > > Function drivers re-use same request for a given bind-unbind session and
> > > hence their dwc3_request context gets preserved across cable
> > > disconnect/connect. When such a request gets re-queued after cable connect,
> > 
> > Why would we preserve the request after a disconnect? The request is
> > associated with an endpoint, and after disconnect, the endpoint is no
> > longer valid. Shouldn't the request be freed then?
> > 
> 
> 
> Function drivers generally allocate usb requests during bind when an
> endpoint is allocated to it (through usb_ep_autoconfig). These requests are
> freed when an unbind is called as the function is no longer associated with
> any end point. The function driver is free to re-use these requests
> throughout this bind-unbind session. The only restriction is that the
> function drivers wont be able to queue any requests as long as the endpoint

> is disabled. But that doesn't enforce function drivers to free the requests
> with ep_disable(). Even though the endpoint is disabled with cable
> disconnect, that endpoint is still associated with that particular function
> driver until that function is unbound.
> 
> As an example below is how f_ncm driver allocates and frees the requests
> during bind/unbind
> 
> Bind()
> ...
> ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_notify_desc);
> if (!ep)
> 	goto fail;
> ncm->notify = ep;
> 
> status = -ENOMEM;
> 
> /* allocate notification request and buffer */
> ncm->notify_req = usb_ep_alloc_request(ep, GFP_KERNEL);
> ...
> 
> The endpoint is enabled later when set_alt is received and disabled in
> ncm_disable when the connection goes down (cable disconnect scenario)
> 
> 
> Unbind()
> ....
> kfree(ncm->notify_req->buf);
> usb_ep_free_request(ncm->notify, ncm->notify_req);
> 
> I see similar implementation in other function drivers as well. That is,
> keep the usb requests allocated throughout the bind-unbind session and
> independent of ep_enable/ep_disable .
> 
> Thanks
> Elson
> 

Thanks for the clarification. Then you just need to reset the num_trbs
count when giving back the request. Can we do that in
dwc3_gadget_del_and_unmap_request()?

Please add a fix tag.

Thanks,
Thinh