linux-kernel - Re: [PATCH] usb: dwc3: add cancelled reason for dwc3 requests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f3f6c78-a584-aaef-3f5d-7734620c433f@synopsys.com>
Date:   Thu, 25 Mar 2021 20:52:17 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Ray Chi <raychi@...gle.com>, "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "albertccwang@...gle.com" <albertccwang@...gle.com>
Subject: Re: [PATCH] usb: dwc3: add cancelled reason for dwc3 requests

Hi Ray,

Ray Chi wrote:
> Currently, when dwc3 handles request cancelled, dwc3 just returns
> -ECONNRESET for all requests. It will cause USB class drivers can't

class drivers -> gadget driver or function driver.

> know if the requests are cancelled by other reasons.
> 
> This patch will add the reason when the requests are cancelled.
> 
> Signed-off-by: Ray Chi <raychi@...gle.com>
> ---
>  drivers/usb/dwc3/gadget.c |  6 +++---
>  drivers/usb/dwc3/gadget.h | 10 +++++++++-
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e1442fc763e1..cc65fc064078 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1402,7 +1402,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>  		dwc3_stop_active_transfer(dep, true, true);
>  
>  		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
> -			dwc3_gadget_move_cancelled_request(req);
> +			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_DEQUEUED);
>  
>  		/* If ep isn't started, then there's no end transfer pending */
>  		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> @@ -1776,7 +1776,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  			 * cancelled.
>  			 */
>  			list_for_each_entry_safe(r, t, &dep->started_list, list)
> -				dwc3_gadget_move_cancelled_request(r);
> +				dwc3_gadget_move_cancelled_request(r, DWC3_REQUEST_DEQUEUED);
>  
>  			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>  
> @@ -1848,7 +1848,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>  		dwc3_stop_active_transfer(dep, true, true);
>  
>  		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
> -			dwc3_gadget_move_cancelled_request(req);
> +			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STALL);
>  
>  		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
>  			dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 0cd281949970..a23e85bd3933 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -56,6 +56,12 @@ struct dwc3;
>  
>  /* Frame/Microframe Number Mask */
>  #define DWC3_FRNUMBER_MASK		0x3fff
> +
> +/* Cancel reason for dwc3 request */
> +#define DWC3_REQUEST_DEQUEUED		-ECONNRESET  /* Request get dequeued */
> +#define DWC3_REQUEST_DISCONNECTED	-ESHUTDOWN   /* Device is disconnected/disabled */
> +#define DWC3_REQUEST_STALL		-EPIPE       /* Bus or protocol error */
> +
>  /* -------------------------------------------------------------------------- */
>  
>  #define to_dwc3_request(r)	(container_of(r, struct dwc3_request, request))
> @@ -90,15 +96,17 @@ static inline void dwc3_gadget_move_started_request(struct dwc3_request *req)
>  /**
>   * dwc3_gadget_move_cancelled_request - move @req to the cancelled_list
>   * @req: the request to be moved
> + * @reason: cancelled reason for the dwc3 request
>   *
>   * Caller should take care of locking. This function will move @req from its
>   * current list to the endpoint's cancelled_list.
>   */
> -static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request *req)
> +static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request *req, int reason)
>  {
>  	struct dwc3_ep		*dep = req->dep;
>  
>  	req->status = DWC3_REQUEST_STATUS_CANCELLED;

My suggestion previously was to replace DWC3_REQUEST_STATUS_CANCELLED
with more specific status:

DWC3_REQUEST_STATUS_DISCONNECTED
DWC3_REQUEST_STATUS_DEQUEUED
DWC3_REQUEST_STATUS_STALLED

and set req->status = reason;

> +	req->request.status = reason;

Not to update this here as this doesn't do anything. Update
dwc3_gadget_ep_cleanup_cancelled_requests() to have a switch case and
provide the appropriate giveback error code base on the status above.

>  	list_move_tail(&req->list, &dep->cancelled_list);
>  }
>  
> 

BR,
Thinh