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

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

[Thinh Nguyen]

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?

BR,
Thinh


> we would increase the num_trbs field on top of the previous stale value
> thus incorrectly representing the number of TRBs used. Fix this by invoking
> skip_trbs() in the ep disable path.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 578804d..b45e917 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -986,6 +986,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
>  	return 0;
>  }
>  
> +static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *req);
> +
>  void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
>  {
>  	struct dwc3_request		*req;
> @@ -1000,6 +1002,7 @@ void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
>  	while (!list_empty(&dep->started_list)) {
>  		req = next_request(&dep->started_list);
>  
> +		dwc3_gadget_ep_skip_trbs(dep, req);
>  		dwc3_gadget_giveback(dep, req, status);
>  	}
>  
> @@ -1012,6 +1015,7 @@ void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
>  	while (!list_empty(&dep->cancelled_list)) {
>  		req = next_request(&dep->cancelled_list);
>  
> +		dwc3_gadget_ep_skip_trbs(dep, req);
>  		dwc3_gadget_giveback(dep, req, status);
>  	}
>  }
> -- 
> 2.7.4
> 

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

[Elson Serrao]

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


> 
>> we would increase the num_trbs field on top of the previous stale value
>> thus incorrectly representing the number of TRBs used. Fix this by invoking
>> skip_trbs() in the ep disable path.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/dwc3/gadget.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 578804d..b45e917 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -986,6 +986,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
>>   	return 0;
>>   }
>>   
>> +static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *req);
>> +
>>   void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
>>   {
>>   	struct dwc3_request		*req;
>> @@ -1000,6 +1002,7 @@ void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
>>   	while (!list_empty(&dep->started_list)) {
>>   		req = next_request(&dep->started_list);
>>   
>> +		dwc3_gadget_ep_skip_trbs(dep, req);
>>   		dwc3_gadget_giveback(dep, req, status);
>>   	}
>>   
>> @@ -1012,6 +1015,7 @@ void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep, int status)
>>   	while (!list_empty(&dep->cancelled_list)) {
>>   		req = next_request(&dep->cancelled_list);
>>   
>> +		dwc3_gadget_ep_skip_trbs(dep, req);
>>   		dwc3_gadget_giveback(dep, req, status);
>>   	}
>>   }
>> -- 
>> 2.7.4

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

[Thinh Nguyen]

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

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

[Elson Serrao]

On 5/31/2023 6:30 PM, Thinh Nguyen wrote:
> 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.


Yes we can just reset num_trbs in dwc3_gadget_del_and_unmap_request. I 
had used skip trb function so that the trb_dequeue pointer and HWO field 
also gets modified accordingly. But we dont really care about it in the 
disconnect path as we reset that in the subsequent ep enable.
Thanks for this suggestion!
I will add a fix tag and re-upload the patch with above modification.

Thanks
Elson