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

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

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