| 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
| ||
|
Message-ID: <49416a44-6317-c3e5-dca6-d33f3d8c9c89@synopsys.com>
Date: Wed, 13 Nov 2019 03:55:30 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Michael Olbrich <m.olbrich@...gutronix.de>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
CC: Felipe Balbi <balbi@...nel.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more
robust
Hi,
Michael Olbrich wrote:
> Currently __dwc3_gadget_start_isoc must be called very shortly after
> XferNotReady. Otherwise the frame number is outdated and start transfer
> will fail, even with several retries.
>
> DSTS provides the lower 14 bit of the frame number. Use it in combination
> with the frame number provided by XferNotReady to guess the current frame
> number. This will succeed unless more than one 14 rollover has happened
> since XferNotReady.
>
> Start transfer might still fail if the frame number is increased
> immediately after DSTS is read. So retries are still needed.
> Don't drop the current request if this happens. This way it is not lost and
> can be used immediately to try again with the next frame number.
>
> With this change, __dwc3_gadget_start_isoc is still not successfully in all
> cases bit it increases the acceptable delay after XferNotReady
> significantly.
>
> Signed-off-by: Michael Olbrich <m.olbrich@...gutronix.de>
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
> 2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 3dd783b889cb..c5b223656e08 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -709,6 +709,7 @@ struct dwc3_ep {
> u8 type;
> u8 resource_index;
> u32 frame_number;
> + u32 last_frame_number;
There's no need to add a new field for last_frame_number. Just store the
value in a local variable in __dwc3_gadget_start_isoc().
> u32 interval;
>
> char name[20];
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f5329d3d9..ac4673d91939 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
> }
> }
>
> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
> {
> struct dwc3_gadget_ep_cmd_params params;
> struct dwc3_request *req;
> @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> }
>
> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
> - if (ret < 0) {
> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
> /*
> * FIXME we need to iterate over the list of requests
> * here and stop, unmap, free and del each of the linked
> @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> return ret;
> }
>
> - return 0;
> + return ret;
> }
>
> static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
> dep->start_cmd_status = 0;
> dep->combo_num = 0;
>
> - return __dwc3_gadget_kick_transfer(dep);
> + return __dwc3_gadget_kick_transfer(dep, false);
> }
>
> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> }
>
> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> + /*
> + * last_frame_number is set from XferNotReady and may be
> + * already out of date. DSTS only provides the lower 14 bit
> + * of the current frame number. So add the upper two bits of
> + * last_frame_number and handle a possible rollover.
> + * This will provide the correct frame_number unless more than
> + * rollover has happened since XferNotReady.
> + */
> + u32 frame = __dwc3_gadget_get_frame(dwc);
> +
> + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
> + if (frame < (dep->last_frame_number & 0x3fff))
> + dep->frame_number += 0x4000;
Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We
started using 0x3fff in multiple places now, can we create a macro for that?
Also, add an empty line here.
> + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
>
> - ret = __dwc3_gadget_kick_transfer(dep);
> + ret = __dwc3_gadget_kick_transfer(dep,
> + i + 1 < DWC3_ISOC_MAX_RETRIES);
> if (ret != -EAGAIN)
> break;
> }
> @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
> }
> }
>
> - return __dwc3_gadget_kick_transfer(dep);
> + return __dwc3_gadget_kick_transfer(dep, false);
> }
>
> static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
> @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>
> if (!dwc3_gadget_ep_request_completed(req) &&
> req->num_pending_sgs) {
> - __dwc3_gadget_kick_transfer(dep);
> + __dwc3_gadget_kick_transfer(dep, false);
> goto out;
> }
>
> @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
> const struct dwc3_event_depevt *event)
> {
> dep->frame_number = event->parameters;
> + dep->last_frame_number = event->parameters;
> }
>
> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
Other than the comments I provided, this patch looks fine to me.
BR,
Thinh
Powered by blists - more mailing lists