| 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
| ||
|
Date: Wed, 31 Mar 2021 00:41:15 +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 v4] usb: dwc3: add cancelled reasons for dwc3 requests
Ray Chi wrote:
> Currently, when dwc3 handles request cancelled, dwc3 just returns
> -ECONNRESET for all requests. It will cause USB function drivers
> can't know if the requests are cancelled by other reasons.
>
> This patch will replace DWC3_REQUEST_STATUS_CANCELLED with the
> reasons below.
> - DWC3_REQUEST_STATUS_DISCONNECTED
> - DWC3_REQUEST_STATUS_DEQUEUED
> - DWC3_REQUEST_STATUS_STALLED
>
> Signed-off-by: Ray Chi <raychi@...gle.com>
> ---
> Changelog since v3:
> - add error log and pass -ECONNRESET in default case
>
> Changelog since v2:
> - modify the changelog
> - remove theree definitions
> - replace DWC3_REQUEST_STATUS_CANCELLED with new cancelled reason
> - In dwc3_gadget_ep_cleanup_cancelled_requests(),
> add a switch case to give different error code base on the reason
>
> Changelog since v1:
> - Added new parameter to dwc3_gadget_move_cancelled_request()
> - Added three definitions for cancelled reasons
> - Passed the reason to req->request.status
>
> drivers/usb/dwc3/core.h | 12 +++++++-----
> drivers/usb/dwc3/gadget.c | 24 ++++++++++++++++++++----
> drivers/usb/dwc3/gadget.h | 6 ++++--
> 3 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4ca4b4be85e4..51a3eec2428a 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -908,11 +908,13 @@ struct dwc3_request {
> unsigned int remaining;
>
> unsigned int status;
> -#define DWC3_REQUEST_STATUS_QUEUED 0
> -#define DWC3_REQUEST_STATUS_STARTED 1
> -#define DWC3_REQUEST_STATUS_CANCELLED 2
> -#define DWC3_REQUEST_STATUS_COMPLETED 3
> -#define DWC3_REQUEST_STATUS_UNKNOWN -1
> +#define DWC3_REQUEST_STATUS_QUEUED 0
> +#define DWC3_REQUEST_STATUS_STARTED 1
> +#define DWC3_REQUEST_STATUS_DISCONNECTED 2
> +#define DWC3_REQUEST_STATUS_DEQUEUED 3
> +#define DWC3_REQUEST_STATUS_STALLED 4
> +#define DWC3_REQUEST_STATUS_COMPLETED 5
> +#define DWC3_REQUEST_STATUS_UNKNOWN -1
The -1 here is an weird... as the dwc3_request status is an unsigned int
type. We can make this fix in a separate patch, and what's done here
looks good.
>
> u8 epnum;
> struct dwc3_trb *trb;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e1442fc763e1..492086519539 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_STATUS_DEQUEUED);
>
> /* If ep isn't started, then there's no end transfer pending */
> if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> @@ -1729,10 +1729,25 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
> {
> struct dwc3_request *req;
> struct dwc3_request *tmp;
> + struct dwc3 *dwc = dep->dwc;
>
> list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) {
> dwc3_gadget_ep_skip_trbs(dep, req);
> - dwc3_gadget_giveback(dep, req, -ECONNRESET);
> + switch (req->status) {
> + case DWC3_REQUEST_STATUS_DISCONNECTED:
> + dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
> + break;
> + case DWC3_REQUEST_STATUS_DEQUEUED:
> + dwc3_gadget_giveback(dep, req, -ECONNRESET);
> + break;
> + case DWC3_REQUEST_STATUS_STALLED:
> + dwc3_gadget_giveback(dep, req, -EPIPE);
> + break;
> + default:
> + dev_err(dwc->dev, "request cancelled with wrong reason:%d\n", req->status);
> + dwc3_gadget_giveback(dep, req, -ECONNRESET);
> + break;
> + }
> }
> }
>
> @@ -1776,7 +1791,8 @@ 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_STATUS_DEQUEUED);
>
> dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>
> @@ -1848,7 +1864,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_STATUS_STALLED);
>
> 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..77df4b6d6c13 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -90,15 +90,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,
> + unsigned int reason)
> {
> struct dwc3_ep *dep = req->dep;
>
> - req->status = DWC3_REQUEST_STATUS_CANCELLED;
> + req->status = reason;
> list_move_tail(&req->list, &dep->cancelled_list);
> }
>
>
Reviewed-by: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Thanks for the patch,
Thinh
Powered by blists - more mailing lists