[PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Udipto Goswami]

A dequeue for ep0 need to adjust the handling based on the
data stage and status stage. Currently if ep0 is in data/status
stage the handling isn't that different, driver will try giveback
as part of dequeue process which might potentially lead to the
controller accessing invalid trbs.

Also for ep0 the requests aren't moved into the started_list,
which might potentially lead to the un-mapping of the request
buffers without sending endxfer.

Fix this by implementing a separate ep0 dequeue function where
if ep0 is still in data phase, driver will perform stall and
restart.

Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
 drivers/usb/dwc3/ep0.c    | 27 +++++++++++++++++++++++++++
 drivers/usb/dwc3/gadget.c |  3 +--
 drivers/usb/dwc3/gadget.h |  1 +
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 61de693461da..70b6df83d76e 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -1206,3 +1206,30 @@ void dwc3_ep0_interrupt(struct dwc3 *dwc,
 		break;
 	}
 }
+
+int dwc3_gadget_ep0_dequeue(struct usb_ep *ep, struct usb_request *request)
+{
+	struct dwc3_request		*req = to_dwc3_request(request);
+	struct dwc3_ep			*dep = to_dwc3_ep(ep);
+	struct dwc3			*dwc = dep->dwc;
+	unsigned long			flags;
+
+	trace_dwc3_ep_dequeue(req);
+	spin_lock_irqsave(&dwc->lock, flags);
+
+	if (dwc->ep0state != EP0_SETUP_PHASE) {
+		unsigned int dir;
+
+		dir = !!dwc->ep0_expect_in;
+		if (dwc->ep0state == EP0_DATA_PHASE)
+			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
+		else
+			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
+
+		dwc3_ep0_stall_and_restart(dwc);
+	}
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return 0;
+}
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5fe2d136dff5..3a8ca27eb5ee 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2058,7 +2058,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 	int				ret = 0;
 
 	trace_dwc3_ep_dequeue(req);
-
 	spin_lock_irqsave(&dwc->lock, flags);
 
 	list_for_each_entry(r, &dep->cancelled_list, list) {
@@ -2239,7 +2238,7 @@ static const struct usb_ep_ops dwc3_gadget_ep0_ops = {
 	.alloc_request	= dwc3_gadget_ep_alloc_request,
 	.free_request	= dwc3_gadget_ep_free_request,
 	.queue		= dwc3_gadget_ep0_queue,
-	.dequeue	= dwc3_gadget_ep_dequeue,
+	.dequeue	= dwc3_gadget_ep0_dequeue,
 	.set_halt	= dwc3_gadget_ep0_set_halt,
 	.set_wedge	= dwc3_gadget_ep_set_wedge,
 };
diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
index 55a56cf67d73..115321cb34b3 100644
--- a/drivers/usb/dwc3/gadget.h
+++ b/drivers/usb/dwc3/gadget.h
@@ -116,6 +116,7 @@ int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
 int dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
 int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
 		gfp_t gfp_flags);
+int dwc3_gadget_ep0_dequeue(struct usb_ep *ep, struct usb_request *request);
 int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol);
 void dwc3_ep0_send_delayed_status(struct dwc3 *dwc);
 void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt);
-- 
2.17.1

Re: [PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Thinh Nguyen]

On Thu, Nov 17, 2022, Udipto Goswami wrote:
> A dequeue for ep0 need to adjust the handling based on the
> data stage and status stage. Currently if ep0 is in data/status
> stage the handling isn't that different, driver will try giveback
> as part of dequeue process which might potentially lead to the
> controller accessing invalid trbs.
> 
> Also for ep0 the requests aren't moved into the started_list,
> which might potentially lead to the un-mapping of the request
> buffers without sending endxfer.

Maybe we need to track started_list for control endpoint? If the request
isn't prepared yet or that the transfer had completed, then there's no
need to issue End Tranfer command.

But I believe sending End Transfer for inactive endpoint should be fine
also. Then we maybe able to get away without checking the started list.
If you're planning to do that, please test and note it somewhere.

> 
> Fix this by implementing a separate ep0 dequeue function where
> if ep0 is still in data phase, driver will perform stall and
> restart.
> 
> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
>  drivers/usb/dwc3/ep0.c    | 27 +++++++++++++++++++++++++++
>  drivers/usb/dwc3/gadget.c |  3 +--
>  drivers/usb/dwc3/gadget.h |  1 +
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 61de693461da..70b6df83d76e 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1206,3 +1206,30 @@ void dwc3_ep0_interrupt(struct dwc3 *dwc,
>  		break;
>  	}
>  }
> +
> +int dwc3_gadget_ep0_dequeue(struct usb_ep *ep, struct usb_request *request)
> +{
> +	struct dwc3_request		*req = to_dwc3_request(request);
> +	struct dwc3_ep			*dep = to_dwc3_ep(ep);
> +	struct dwc3			*dwc = dep->dwc;
> +	unsigned long			flags;
> +

Need to check if the control transfer is active and the input request is
the valid request to dequeue. Return error code if it's not.

> +	trace_dwc3_ep_dequeue(req);
> +	spin_lock_irqsave(&dwc->lock, flags);
> +
> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
> +		unsigned int dir;
> +
> +		dir = !!dwc->ep0_expect_in;
> +		if (dwc->ep0state == EP0_DATA_PHASE)
> +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
> +		else
> +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);

What if the status stage is active? May need to check for active status
stage in other places to issue End Transfer too.

> +
> +		dwc3_ep0_stall_and_restart(dwc);
> +	}
> +
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	return 0;
> +}
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5fe2d136dff5..3a8ca27eb5ee 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2058,7 +2058,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  	int				ret = 0;
>  
>  	trace_dwc3_ep_dequeue(req);
> -

Can we keep this new line?

>  	spin_lock_irqsave(&dwc->lock, flags);
>  
>  	list_for_each_entry(r, &dep->cancelled_list, list) {
> @@ -2239,7 +2238,7 @@ static const struct usb_ep_ops dwc3_gadget_ep0_ops = {
>  	.alloc_request	= dwc3_gadget_ep_alloc_request,
>  	.free_request	= dwc3_gadget_ep_free_request,
>  	.queue		= dwc3_gadget_ep0_queue,
> -	.dequeue	= dwc3_gadget_ep_dequeue,
> +	.dequeue	= dwc3_gadget_ep0_dequeue,
>  	.set_halt	= dwc3_gadget_ep0_set_halt,
>  	.set_wedge	= dwc3_gadget_ep_set_wedge,
>  };
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 55a56cf67d73..115321cb34b3 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -116,6 +116,7 @@ int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
>  int dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
>  int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>  		gfp_t gfp_flags);
> +int dwc3_gadget_ep0_dequeue(struct usb_ep *ep, struct usb_request *request);
>  int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol);
>  void dwc3_ep0_send_delayed_status(struct dwc3 *dwc);
>  void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt);
> -- 
> 2.17.1
> 

Thanks,
Thinh

Re: [PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Udipto Goswami]

Hi Thinh

On 11/18/22 7:31 AM, Thinh Nguyen wrote:
> On Thu, Nov 17, 2022, Udipto Goswami wrote:
>> A dequeue for ep0 need to adjust the handling based on the
>> data stage and status stage. Currently if ep0 is in data/status
>> stage the handling isn't that different, driver will try giveback
>> as part of dequeue process which might potentially lead to the
>> controller accessing invalid trbs.
>>
>> Also for ep0 the requests aren't moved into the started_list,
>> which might potentially lead to the un-mapping of the request
>> buffers without sending endxfer.
> 
> Maybe we need to track started_list for control endpoint? If the request
> isn't prepared yet or that the transfer had completed, then there's no
> need to issue End Tranfer command.
> 
> But I believe sending End Transfer for inactive endpoint should be fine
> also. Then we maybe able to get away without checking the started list.
> If you're planning to do that, please test and note it somewhere.
> 
>>
thanks for the suggestion, sure i'll do some more experiments and 
confirm it.

>> Fix this by implementing a separate ep0 dequeue function where
>> if ep0 is still in data phase, driver will perform stall and
>> restart.
>>
>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
>> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
>> ---
>>   drivers/usb/dwc3/ep0.c    | 27 +++++++++++++++++++++++++++
>>   drivers/usb/dwc3/gadget.c |  3 +--
>>   drivers/usb/dwc3/gadget.h |  1 +
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 61de693461da..70b6df83d76e 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -1206,3 +1206,30 @@ void dwc3_ep0_interrupt(struct dwc3 *dwc,
>>   		break;
>>   	}
>>   }
>> +
>> +int dwc3_gadget_ep0_dequeue(struct usb_ep *ep, struct usb_request *request)
>> +{
>> +	struct dwc3_request		*req = to_dwc3_request(request);
>> +	struct dwc3_ep			*dep = to_dwc3_ep(ep);
>> +	struct dwc3			*dwc = dep->dwc;
>> +	unsigned long			flags;
>> +
> 
> Need to check if the control transfer is active and the input request is
> the valid request to dequeue. Return error code if it's not.
> 
sure will do that in v2.

>> +	trace_dwc3_ep_dequeue(req);
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +
>> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
>> +		unsigned int dir;
>> +
>> +		dir = !!dwc->ep0_expect_in;
>> +		if (dwc->ep0state == EP0_DATA_PHASE)
>> +			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
>> +		else
>> +			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
> 
> What if the status stage is active? May need to check for active status
> stage in other places to issue End Transfer too.
> 
okay will check this.
>> +
>> +		dwc3_ep0_stall_and_restart(dwc);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5fe2d136dff5..3a8ca27eb5ee 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2058,7 +2058,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>   	int				ret = 0;
>>   
>>   	trace_dwc3_ep_dequeue(req);
>> -
> 
> Can we keep this new line?
got it.
> 
>>   	spin_lock_irqsave(&dwc->lock, flags);
>>   
>>   	list_for_each_entry(r, &dep->cancelled_list, list) {
>> @@ -2239,7 +2238,7 @@ static const struct usb_ep_ops dwc3_gadget_ep0_ops = {
>>   	.alloc_request	= dwc3_gadget_ep_alloc_request,
>>   	.free_request	= dwc3_gadget_ep_free_request,
>>   	.queue		= dwc3_gadget_ep0_queue,
>> -	.dequeue	= dwc3_gadget_ep_dequeue,
>> +	.dequeue	= dwc3_gadget_ep0_dequeue,
>>   	.set_halt	= dwc3_gadget_ep0_set_halt,
>>   	.set_wedge	= dwc3_gadget_ep_set_wedge,
>>   };
>> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
>> index 55a56cf67d73..115321cb34b3 100644
>> --- a/drivers/usb/dwc3/gadget.h
>> +++ b/drivers/usb/dwc3/gadget.h
>> @@ -116,6 +116,7 @@ int __dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
>>   int dwc3_gadget_ep0_set_halt(struct usb_ep *ep, int value);
>>   int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>>   		gfp_t gfp_flags);
>> +int dwc3_gadget_ep0_dequeue(struct usb_ep *ep, struct usb_request *request);
>>   int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol);
>>   void dwc3_ep0_send_delayed_status(struct dwc3 *dwc);
>>   void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt);
>> -- 
>> 2.17.1
>>
> 
> Thanks,
> Thinh

Thanks,
-Udipto

Re: [PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Thinh Nguyen]

Hi Udipto,

On Fri, Nov 18, 2022, Udipto Goswami wrote:
> Hi Thinh
> 
> On 11/18/22 7:31 AM, Thinh Nguyen wrote:
> > On Thu, Nov 17, 2022, Udipto Goswami wrote:
> > > A dequeue for ep0 need to adjust the handling based on the
> > > data stage and status stage. Currently if ep0 is in data/status
> > > stage the handling isn't that different, driver will try giveback
> > > as part of dequeue process which might potentially lead to the
> > > controller accessing invalid trbs.
> > > 
> > > Also for ep0 the requests aren't moved into the started_list,
> > > which might potentially lead to the un-mapping of the request
> > > buffers without sending endxfer.
> > 
> > Maybe we need to track started_list for control endpoint? If the request
> > isn't prepared yet or that the transfer had completed, then there's no
> > need to issue End Tranfer command.
> > 
> > But I believe sending End Transfer for inactive endpoint should be fine
> > also. Then we maybe able to get away without checking the started list.
> > If you're planning to do that, please test and note it somewhere.
> > 
> > > 
> thanks for the suggestion, sure i'll do some more experiments and confirm
> it.
> 

Just curious, how do you hit/test this scenario?

For other endpoint types, I can see possible scenarios where a dequeue
may be needed, but I don't see one for control transfer.

The host can cancel the control transfer, and the controller will see
"setup_packet_pending" and handle accordingly. If there's a disconnect,
that's also handled separately by the controller driver also. So, where
does ep0_dequeue come into play here?

Thanks,
Thinh

Re: [PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Udipto Goswami]

Hi Thinh,


On 11/22/22 7:00 AM, Thinh Nguyen wrote:
> Hi Udipto,
> 
> On Fri, Nov 18, 2022, Udipto Goswami wrote:
>> Hi Thinh
>>
>> On 11/18/22 7:31 AM, Thinh Nguyen wrote:
>>> On Thu, Nov 17, 2022, Udipto Goswami wrote:
>>>> A dequeue for ep0 need to adjust the handling based on the
>>>> data stage and status stage. Currently if ep0 is in data/status
>>>> stage the handling isn't that different, driver will try giveback
>>>> as part of dequeue process which might potentially lead to the
>>>> controller accessing invalid trbs.
>>>>
>>>> Also for ep0 the requests aren't moved into the started_list,
>>>> which might potentially lead to the un-mapping of the request
>>>> buffers without sending endxfer.
>>>
>>> Maybe we need to track started_list for control endpoint? If the request
>>> isn't prepared yet or that the transfer had completed, then there's no
>>> need to issue End Tranfer command.
>>>
>>> But I believe sending End Transfer for inactive endpoint should be fine
>>> also. Then we maybe able to get away without checking the started list.
>>> If you're planning to do that, please test and note it somewhere.
>>>
>>>>
>> thanks for the suggestion, sure i'll do some more experiments and confirm
>> it.
>>
> 
> Just curious, how do you hit/test this scenario?
> 
> For other endpoint types, I can see possible scenarios where a dequeue
> may be needed, but I don't see one for control transfer.
> 
> The host can cancel the control transfer, and the controller will see
> "setup_packet_pending" and handle accordingly. If there's a disconnect,
> that's also handled separately by the controller driver also. So, where
> does ep0_dequeue come into play here?
> 
adding the reference to other thread [1]

[1]: https://www.spinics.net/lists/linux-usb/msg233862.html

was trying to address a race condition in the ffs driver where 
ep_dequeue was suggested, before freeing the request dequeue it.

as per the current code, since ep0 req isn't moved to started list
therefore it will exit from this in ep_dequeue:

list_for_each_entry(r, &dep->pending_list, list) {
                    if (r == req) {
                            dwc3_gadget_giveback(dep, req, -ECONNRESET);
                             goto out;
                     }
             }

but if the ep0 is in data/status phase technically it is still active.
We will unmap the buffer and giveback then the ep0 is in data/status stage.

This could potentially happen right?

The intent of a separate dequeue was to address that scenario when the 
data/status phase isn't completed.
Hope this give some clarity.

Thanks,
-Udipto

> Thanks,
> Thinh

Re: [PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Thinh Nguyen]

On Tue, Nov 22, 2022, Udipto Goswami wrote:
> Hi Thinh,
> 
> 
> On 11/22/22 7:00 AM, Thinh Nguyen wrote:
> > Hi Udipto,
> > 
> > On Fri, Nov 18, 2022, Udipto Goswami wrote:
> > > Hi Thinh
> > > 
> > > On 11/18/22 7:31 AM, Thinh Nguyen wrote:
> > > > On Thu, Nov 17, 2022, Udipto Goswami wrote:
> > > > > A dequeue for ep0 need to adjust the handling based on the
> > > > > data stage and status stage. Currently if ep0 is in data/status
> > > > > stage the handling isn't that different, driver will try giveback
> > > > > as part of dequeue process which might potentially lead to the
> > > > > controller accessing invalid trbs.
> > > > > 
> > > > > Also for ep0 the requests aren't moved into the started_list,
> > > > > which might potentially lead to the un-mapping of the request
> > > > > buffers without sending endxfer.
> > > > 
> > > > Maybe we need to track started_list for control endpoint? If the request
> > > > isn't prepared yet or that the transfer had completed, then there's no
> > > > need to issue End Tranfer command.
> > > > 
> > > > But I believe sending End Transfer for inactive endpoint should be fine
> > > > also. Then we maybe able to get away without checking the started list.
> > > > If you're planning to do that, please test and note it somewhere.
> > > > 
> > > > > 
> > > thanks for the suggestion, sure i'll do some more experiments and confirm
> > > it.
> > > 
> > 
> > Just curious, how do you hit/test this scenario?
> > 
> > For other endpoint types, I can see possible scenarios where a dequeue
> > may be needed, but I don't see one for control transfer.
> > 
> > The host can cancel the control transfer, and the controller will see
> > "setup_packet_pending" and handle accordingly. If there's a disconnect,
> > that's also handled separately by the controller driver also. So, where
> > does ep0_dequeue come into play here?
> > 
> adding the reference to other thread [1]
> 
> [1]: https://urldefense.com/v3/__https://www.spinics.net/lists/linux-usb/msg233862.html__;!!A4F2R9G_pg!Z6hsArtLfeqmaf08IqxTov5VyXdvLJuncb8wIXYHC5PW7Zk7WO6u_r8Ap1gR-TlrmzwmQEQ-cJElQ2ED_0deM8t49emcjw$
> 
> was trying to address a race condition in the ffs driver where ep_dequeue
> was suggested, before freeing the request dequeue it.
> 
> as per the current code, since ep0 req isn't moved to started list
> therefore it will exit from this in ep_dequeue:
> 
> list_for_each_entry(r, &dep->pending_list, list) {
>                    if (r == req) {
>                            dwc3_gadget_giveback(dep, req, -ECONNRESET);
>                             goto out;
>                     }
>             }
> 
> but if the ep0 is in data/status phase technically it is still active.
> We will unmap the buffer and giveback then the ep0 is in data/status stage.
> 
> This could potentially happen right?
> 
> The intent of a separate dequeue was to address that scenario when the
> data/status phase isn't completed.
> Hope this give some clarity.
> 

I'm still unclear how it can lead to this condition. Can you list the
sequence of events that can lead to this scenario.

For functionfs_unbind() to occur, shouldn't there be a disconnect call,
triggering soft-disconnect? When that happens, there are checks in dwc3
to ensure that any pending control transfer will be STALLed and given
back. Any new control request will not be queued, and no active control
transfer should happen at this point.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: ep0: Add implementation of ep0_dequeue separately

[Udipto Goswami]

Hi Thinh,

On 11/23/22 7:36 AM, Thinh Nguyen wrote:
> On Tue, Nov 22, 2022, Udipto Goswami wrote:
>> Hi Thinh,
>>
>>
>> On 11/22/22 7:00 AM, Thinh Nguyen wrote:
>>> Hi Udipto,
>>>
>>> On Fri, Nov 18, 2022, Udipto Goswami wrote:
>>>> Hi Thinh
>>>>
>>>> On 11/18/22 7:31 AM, Thinh Nguyen wrote:
>>>>> On Thu, Nov 17, 2022, Udipto Goswami wrote:
>>>>>> A dequeue for ep0 need to adjust the handling based on the
>>>>>> data stage and status stage. Currently if ep0 is in data/status
>>>>>> stage the handling isn't that different, driver will try giveback
>>>>>> as part of dequeue process which might potentially lead to the
>>>>>> controller accessing invalid trbs.
>>>>>>
>>>>>> Also for ep0 the requests aren't moved into the started_list,
>>>>>> which might potentially lead to the un-mapping of the request
>>>>>> buffers without sending endxfer.
>>>>>
>>>>> Maybe we need to track started_list for control endpoint? If the request
>>>>> isn't prepared yet or that the transfer had completed, then there's no
>>>>> need to issue End Tranfer command.
>>>>>
>>>>> But I believe sending End Transfer for inactive endpoint should be fine
>>>>> also. Then we maybe able to get away without checking the started list.
>>>>> If you're planning to do that, please test and note it somewhere.
>>>>>
>>>>>>
>>>> thanks for the suggestion, sure i'll do some more experiments and confirm
>>>> it.
>>>>
>>>
>>> Just curious, how do you hit/test this scenario?
>>>
>>> For other endpoint types, I can see possible scenarios where a dequeue
>>> may be needed, but I don't see one for control transfer.
>>>
>>> The host can cancel the control transfer, and the controller will see
>>> "setup_packet_pending" and handle accordingly. If there's a disconnect,
>>> that's also handled separately by the controller driver also. So, where
>>> does ep0_dequeue come into play here?
>>>
>> adding the reference to other thread [1]
>>
>> [1]: https://urldefense.com/v3/__https://www.spinics.net/lists/linux-usb/msg233862.html__;!!A4F2R9G_pg!Z6hsArtLfeqmaf08IqxTov5VyXdvLJuncb8wIXYHC5PW7Zk7WO6u_r8Ap1gR-TlrmzwmQEQ-cJElQ2ED_0deM8t49emcjw$
>>
>> was trying to address a race condition in the ffs driver where ep_dequeue
>> was suggested, before freeing the request dequeue it.
>>
>> as per the current code, since ep0 req isn't moved to started list
>> therefore it will exit from this in ep_dequeue:
>>
>> list_for_each_entry(r, &dep->pending_list, list) {
>>                     if (r == req) {
>>                             dwc3_gadget_giveback(dep, req, -ECONNRESET);
>>                              goto out;
>>                      }
>>              }
>>
>> but if the ep0 is in data/status phase technically it is still active.
>> We will unmap the buffer and giveback then the ep0 is in data/status stage.
>>
>> This could potentially happen right?
>>
>> The intent of a separate dequeue was to address that scenario when the
>> data/status phase isn't completed.
>> Hope this give some clarity.
>>
> 
> I'm still unclear how it can lead to this condition. Can you list the
> sequence of events that can lead to this scenario.
> 
> For functionfs_unbind() to occur, shouldn't there be a disconnect call,
> triggering soft-disconnect? When that happens, there are checks in dwc3
> to ensure that any pending control transfer will be STALLed and given
> back. Any new control request will not be queued, and no active control
> transfer should happen at this point.

Thanks for pointing this out. Yah i see the soft_disconnect already 
addresses what we are trying to do with a separate ep0_dequeue. I 
believe we don't need this change separately.

Thanks again for a clarifications.
-Udipto