From patchwork Thu Apr 14 10:18:09 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Roger Quadros X-Patchwork-Id: 65783 Delivered-To: patch@linaro.org Received: by 10.140.93.198 with SMTP id d64csp507480qge; Thu, 14 Apr 2016 03:18:30 -0700 (PDT) X-Received: by 10.28.127.80 with SMTP id a77mr16108369wmd.84.1460629110751; Thu, 14 Apr 2016 03:18:30 -0700 (PDT) Return-Path: Received: from theia.denx.de (theia.denx.de. [85.214.87.163]) by mx.google.com with ESMTP id fz5si44522763wjb.64.2016.04.14.03.18.30; Thu, 14 Apr 2016 03:18:30 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of u-boot-bounces@lists.denx.de designates 85.214.87.163 as permitted sender) client-ip=85.214.87.163; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of u-boot-bounces@lists.denx.de designates 85.214.87.163 as permitted sender) smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id A104AA7606; Thu, 14 Apr 2016 12:18:29 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id yM0kOlHqIWNO; Thu, 14 Apr 2016 12:18:29 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 05F63A7498; Thu, 14 Apr 2016 12:18:29 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 64A8C4B624 for ; Thu, 14 Apr 2016 12:18:24 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id blm6nNHc3NjC for ; Thu, 14 Apr 2016 12:18:24 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from devils.ext.ti.com (devils.ext.ti.com [198.47.26.153]) by theia.denx.de (Postfix) with ESMTPS id ABEEEA7498 for ; Thu, 14 Apr 2016 12:18:20 +0200 (CEST) Received: from dflxv15.itg.ti.com ([128.247.5.124]) by devils.ext.ti.com (8.13.7/8.13.7) with ESMTP id u3EAIDCx011305; Thu, 14 Apr 2016 05:18:13 -0500 Received: from DLEE71.ent.ti.com (dlee71.ent.ti.com [157.170.170.114]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id u3EAIDw9030332; Thu, 14 Apr 2016 05:18:13 -0500 Received: from dflp32.itg.ti.com (10.64.6.15) by DLEE71.ent.ti.com (157.170.170.114) with Microsoft SMTP Server id 14.3.224.2; Thu, 14 Apr 2016 05:18:12 -0500 Received: from [192.168.2.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp32.itg.ti.com (8.14.3/8.13.8) with ESMTP id u3EAI9Bt016440; Thu, 14 Apr 2016 05:18:10 -0500 To: Sam Protsenko References: <1460548862-27625-1-git-send-email-semen.protsenko@linaro.org> <570E3C67.6080200@ti.com> From: Roger Quadros Message-ID: <570F6E61.10206@ti.com> Date: Thu, 14 Apr 2016 13:18:09 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Cc: Marek Vasut , Steve Rae , Tom Rini , Praneeth Bajjuri , U-Boot Mailing List Subject: Re: [U-Boot] [PATCH] fastboot: Fix OUT transaction length alignment X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.15 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" Hi, On 13/04/16 19:56, Sam Protsenko wrote: > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros wrote: >> Hi, >> >> On 13/04/16 15:01, Semen Protsenko wrote: >>> From: Sam Protsenko >>> >>> Some UDC controllers may require buffer size to be aligned to >>> wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size >>> field being set to "true" (in UDC driver code). In that case >>> rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on >>> transaction will happen. For example, it's required by DWC3 controller >>> data manual: >>> >>> section 8.2.3.3 Buffer Size Rules and Zero-Length Packets: >>> >>> For OUT endpoints, the following rules apply: >>> - The BUFSIZ field must be ≥ 1 byte. >>> - The total size of a Buffer Descriptor must be a multiple of >>> MaxPacketSize >>> - A received zero-length packet still requires a MaxPacketSize buffer. >>> Therefore, if the expected amount of data to be received is a multiple >>> of MaxPacketSize, software should add MaxPacketSize bytes to the buffer >>> to sink a possible zero-length packet at the end of the transfer. >>> >>> But other UDC controllers don't need such alignment, so mentioned field >>> is set to "false". If buffer size is aligned to wMaxPacketSize, those >>> controllers may stuck on transaction. The example is DWC2. >>> >>> This patch checks gadget->quirk_ep_out_aligned_size field and aligns >>> rx_bytes_expected to wMaxPacketSize only when it's needed. >>> >>> Signed-off-by: Sam Protsenko >>> --- >>> drivers/usb/gadget/f_fastboot.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c >>> index 2e87fee..54dcce0 100644 >>> --- a/drivers/usb/gadget/f_fastboot.c >>> +++ b/drivers/usb/gadget/f_fastboot.c >>> @@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id; >>> static unsigned int download_size; >>> static unsigned int download_bytes; >>> static bool is_high_speed; >>> +static bool quirk_ep_out_aligned_size; >>> >>> static struct usb_endpoint_descriptor fs_ep_in = { >>> .bLength = USB_DT_ENDPOINT_SIZE, >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f, >>> debug("%s: func: %s intf: %d alt: %d\n", >>> __func__, f->name, interface, alt); >>> >>> + quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size; >>> + >>> /* make sure we don't enable the ep twice */ >>> if (gadget->speed == USB_SPEED_HIGH) { >>> ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out); >>> @@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket) >>> return 0; >>> if (rx_remain > EP_BUFFER_SIZE) >>> return EP_BUFFER_SIZE; >>> + >>> + if (!quirk_ep_out_aligned_size) >>> + goto out; >>> + >>> if (rx_remain < maxpacket) { >>> rx_remain = maxpacket; >>> } else if (rx_remain % maxpacket != 0) { >>> rem = rx_remain % maxpacket; >>> rx_remain = rx_remain + (maxpacket - rem); >>> } >>> + >>> +out: >>> return rx_remain; >>> } >>> >>> >> >> Why do we need a special flag for this driver if other drivers e.g. mass storage >> are doing perfectly fine without it? >> > > I don't know how it works in other gadgets, but please see this patch > in kernel: [1]. That patch is doing just the same as I did (and also > in gadget code), using usb_ep_align_maybe() function for alignment. NOTE: there haven't been such quirks in the kernel drivers except for that one driver that has a user mode interface and needs more moral policing for user provided buffers. So that example is not something we should be using as reference. Our buffers are alreay aligned to maxpacket size. The only thing we need to worry about is the length of the last transaction that is not integral multiple of maxpacket size. If my understanding is right all USB controllers should work fine with bulk OUT requests that are integral multiple of maxpacket size. So we shouldn't be needing any quirk flags. > >> This patch is just covering up the real problem, by bypassing the faulty code >> with a flag. >> >> The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so >> the problem shouldn't have happened in the first place. But it is happening >> indicating something else is wrong. >> > > There is what I'm observing on platform with DWC3 controller: > - when doing "fastboot flash xloader MLO": > - the whole size of data to send is 70964 bytes > - the size of all packets (except of last packet) is 4096 bytes > - so those are being sent just fine (in req->complete, which is > rx_handler_dl_image() function) > - but last packet has size of 1332 bytes (because 70964 % 4096 = 1332) > - when its req->length is aligned to wMaxPacketSize (so it's 1536 > bytes): after we send it using usb_ep_queue(), the req->complete > callback is called one last time and we see that transmission is > finished (download_bytes >= download_size) > - but when its req->length is not aligned to wMaxPacketSize (so it's > 1332 bytes): req->complete callback is not called last time, so > transaction is not finished and we are stuck in "fastboot flash" > > So I guess the issue is real and related to DWC3 quirk. If you have > any thoughts regarding other possible causes of this problem -- please > share. I can't predict all possible causes as I'm not USB expert. I've tried to clean up the bulk out handling code in the below patch. Note you will need to apply this on top of the 2 patches I sent earlier. https://patchwork.ozlabs.org/patch/609417/ https://patchwork.ozlabs.org/patch/609896/ Steve, do let me know if it works for you. If it doesn't then we need to figure out why. Can you please share details about the USB controller on your board? Does it not like OUT requests that are aligned to maxpacket size? What does ep->maxpacket show for your board? diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 7961231..28b244a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -39,6 +39,11 @@ #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE (0x0040) #define EP_BUFFER_SIZE 4096 +/* + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size + * (64 or 512 or 1024), else we break on certain controllers like DWC3 + * that expect bulk OUT requests to be divisible by maxpacket size. + */ struct f_fastboot { struct usb_function usb_function; @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func; static unsigned int fastboot_flash_session_id; static unsigned int download_size; static unsigned int download_bytes; -static bool is_high_speed; static struct usb_endpoint_descriptor fs_ep_in = { .bLength = USB_DT_ENDPOINT_SIZE, @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f, debug("%s: func: %s intf: %d alt: %d\n", __func__, f->name, interface, alt); - if (gadget->speed == USB_SPEED_HIGH) - is_high_speed = true; - else - is_high_speed = false; - d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out); ret = usb_ep_enable(f_fb->out_ep, d); if (ret) { @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) fastboot_tx_write_str(response); } -static unsigned int rx_bytes_expected(unsigned int maxpacket) +static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = download_size - download_bytes; - int rem = 0; - if (rx_remain < 0) + unsigned int rem; + unsigned int maxpacket = ep->maxpacket; + + if (rx_remain <= 0) return 0; - if (rx_remain > EP_BUFFER_SIZE) + else if (rx_remain > EP_BUFFER_SIZE) return EP_BUFFER_SIZE; - if (rx_remain < maxpacket) { - rx_remain = maxpacket; - } else if (rx_remain % maxpacket != 0) { - rem = rx_remain % maxpacket; + + /* + * Some controllers e.g. DWC3 don't like OUT transfers to be + * not ending in maxpacket boundary. So just make them happy by + * always requesting for integral multiple of maxpackets. + * This shouldn't bother controllers that don't care about it. + */ + rem = rx_remain % maxpacket; + if (rem > 0) rx_remain = rx_remain + (maxpacket - rem); - } + return rx_remain; } @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) const unsigned char *buffer = req->buf; unsigned int buffer_size = req->actual; unsigned int pre_dot_num, now_dot_num; - unsigned int max; if (req->status != 0) { printf("Bad status: %d\n", req->status); @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) printf("\ndownloading of %d bytes finished\n", download_bytes); } else { - max = is_high_speed ? hs_ep_out.wMaxPacketSize : - fs_ep_out.wMaxPacketSize; - req->length = rx_bytes_expected(max); - if (req->length < ep->maxpacket) - req->length = ep->maxpacket; + req->length = rx_bytes_expected(ep); } req->actual = 0; @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) { char *cmd = req->buf; char response[FASTBOOT_RESPONSE_LEN]; - unsigned int max; strsep(&cmd, ":"); download_size = simple_strtoul(cmd, NULL, 16); @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) } else { sprintf(response, "DATA%08x", download_size); req->complete = rx_handler_dl_image; - max = is_high_speed ? hs_ep_out.wMaxPacketSize : - fs_ep_out.wMaxPacketSize; - req->length = rx_bytes_expected(max); - if (req->length < ep->maxpacket) - req->length = ep->maxpacket; + req->length = rx_bytes_expected(ep); } fastboot_tx_write_str(response); }