usb: dwc3: ep0: Add implementation of ep0_dequeue separately

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(-)

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

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