From 7484d84cbb58e01ff1d2d458d899ea1fba012724 Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Thu, 29 May 2014 14:53:00 -0600 Subject: usb: ci_udc: detect queued requests on ep0 The flipping of ep0 between IN and OUT relies on ci_ep_queue() consuming the current IN/OUT setting immediately. If this is deferred to a later point when the req is pulled out of ci_req->queue, then the IN/OUT setting may have been changed since the req was queued, and state will get out of sync. This condition doesn't occur today, but could if bugs were introduced later, and this error-check will save a lot of debugging time. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 9cd003636a..a68a85f84e 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -397,6 +397,21 @@ static int ci_ep_queue(struct usb_ep *ep, num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; in = (ci_ep->desc->bEndpointAddress & USB_DIR_IN) != 0; + if (!num && ci_ep->req_primed) { + /* + * The flipping of ep0 between IN and OUT relies on + * ci_ep_queue consuming the current IN/OUT setting + * immediately. If this is deferred to a later point when the + * req is pulled out of ci_req->queue, then the IN/OUT setting + * may have been changed since the req was queued, and state + * will get out of sync. This condition doesn't occur today, + * but could if bugs were introduced later, and this error + * check will save a lot of debugging time. + */ + printf("%s: ep0 transaction already in progress\n", __func__); + return -EPROTO; + } + ret = ci_bounce(ci_req, in); if (ret) return ret; -- cgit From 054731b09e1944cdb130b755ac0f6c188920e98a Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Thu, 29 May 2014 14:53:01 -0600 Subject: usb: ci_udc: use a single descriptor for ep0 ci_udc currently points ep->desc at separate descriptors for IN and OUT. These descriptors only differ in the ep address IN/OUT field. Modify the code to use a single descriptor, and change that descriptor's ep address to indicate IN/OUT as required. This removes some data duplication. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index a68a85f84e..77f8c9ef7f 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -56,14 +56,7 @@ static const char *reqname(unsigned r) } #endif -static struct usb_endpoint_descriptor ep0_out_desc = { - .bLength = sizeof(struct usb_endpoint_descriptor), - .bDescriptorType = USB_DT_ENDPOINT, - .bEndpointAddress = 0, - .bmAttributes = USB_ENDPOINT_XFER_CONTROL, -}; - -static struct usb_endpoint_descriptor ep0_in_desc = { +static struct usb_endpoint_descriptor ep0_desc = { .bLength = sizeof(struct usb_endpoint_descriptor), .bDescriptorType = USB_DT_ENDPOINT, .bEndpointAddress = USB_DIR_IN, @@ -435,7 +428,7 @@ static void handle_ep_complete(struct ci_ep *ep) num = ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; in = (ep->desc->bEndpointAddress & USB_DIR_IN) != 0; if (num == 0) - ep->desc = &ep0_out_desc; + ep0_desc.bEndpointAddress = 0; item = ci_get_qtd(num, in); ci_invalidate_qtd(num); @@ -460,7 +453,7 @@ static void handle_ep_complete(struct ci_ep *ep) if (num == 0) { ci_req->req.length = 0; usb_ep_queue(&ep->ep, &ci_req->req, 0); - ep->desc = &ep0_in_desc; + ep0_desc.bEndpointAddress = USB_DIR_IN; } } @@ -771,7 +764,7 @@ static int ci_udc_probe(void) /* Init EP 0 */ memcpy(&controller.ep[0].ep, &ci_ep_init[0], sizeof(*ci_ep_init)); - controller.ep[0].desc = &ep0_in_desc; + controller.ep[0].desc = &ep0_desc; INIT_LIST_HEAD(&controller.ep[0].queue); controller.ep[0].req_primed = false; controller.gadget.ep0 = &controller.ep[0].ep; -- cgit From a2d8f929857b7bc50528114c29e48a99cbcee1f1 Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Thu, 29 May 2014 14:53:02 -0600 Subject: usb: ci_udc: pre-allocate ep0 req Allocate ep0's USB request object when the UDC driver is probed. This solves a couple of issues in the current code: a) A request object always exists for ep0. Prior to this patch, if setup transactions arrived in an unexpected order, handle_setup() would need to reply to a setup transaction before any ep0 usb_req was created. This issue was introduced in commit 2813006fecda "usb: ci_udc: allow multiple buffer allocs per ep." b) handle_ep_complete no longer /has/ to queue the ep0 request again after every single request completion. This is currently required, since handle_setup() assumes it can find some request object in ep0's request queue. This patch doesn't actually stop handle_ep_complete() from always requeueing the request, but the next patch will. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 77f8c9ef7f..03ad23164f 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -198,8 +198,14 @@ static void ci_invalidate_qtd(int ep_num) static struct usb_request * ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags) { + struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep); + int num; struct ci_req *ci_req; + num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; + if (num == 0 && controller.ep0_req) + return &controller.ep0_req->req; + ci_req = memalign(ARCH_DMA_MINALIGN, sizeof(*ci_req)); if (!ci_req) return NULL; @@ -207,6 +213,9 @@ ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags) INIT_LIST_HEAD(&ci_req->queue); ci_req->b_buf = 0; + if (num == 0) + controller.ep0_req = ci_req; + return &ci_req->req; } @@ -471,7 +480,7 @@ static void handle_setup(void) int num, in, _num, _in, i; char *buf; - ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue); + ci_req = controller.ep0_req; req = &ci_req->req; head = ci_get_qh(0, 0); /* EP0 OUT */ @@ -780,6 +789,12 @@ static int ci_udc_probe(void) &controller.gadget.ep_list); } + ci_ep_alloc_request(&controller.ep[0].ep, 0); + if (!controller.ep0_req) { + free(controller.epts); + return -ENOMEM; + } + return 0; } -- cgit From 006c7026882011ba49c9a39d27c4aff3ace07847 Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Thu, 29 May 2014 14:53:03 -0600 Subject: usb: ci_udc: complete ep0 direction handling handle_setup() currently assumes that the response to a Setup transaction will be an OUT transaction, and any subsequent packet (if any) will be an IN transaction. This appears to be valid in many cases; both USB enumeration and Mass Storage work OK with this restriction. However, DFU uses ep0 to transfer data in both directions. This renders the assumption invalid; when sending data from device to host, the Data Stage is an IN transaction, and the Status Stage is an OUT transaction. Enhance handle_setup() to deduce the correct direction for the USB transactions based on Setup transaction data. ep0's request object only needs to be automatically re-queued when the Data Stage completes, in order to implement the Status Stage. Once the Status Stage transaction is complete, there is no need to re-queue the USB request, so don't do that. Don't sent USB request completion callbacks for Status Stage transactions. These were queued by ci_udc itself, and only serve to confuse the USB function code. For example, f_dfu attempts to interpret the 0-length data buffers for Status Stage transactions as DFU packets. These buffers contain stale data from the previous transaction. This causes f_dfu to complain about a sequence number mismatch. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 48 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 7 deletions(-) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 03ad23164f..6dc20c6c95 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -428,6 +428,17 @@ static int ci_ep_queue(struct usb_ep *ep, return 0; } +static void flip_ep0_direction(void) +{ + if (ep0_desc.bEndpointAddress == USB_DIR_IN) { + DBG("%s: Flipping ep0 ot OUT\n", __func__); + ep0_desc.bEndpointAddress = 0; + } else { + DBG("%s: Flipping ep0 ot IN\n", __func__); + ep0_desc.bEndpointAddress = USB_DIR_IN; + } +} + static void handle_ep_complete(struct ci_ep *ep) { struct ept_queue_item *item; @@ -436,8 +447,6 @@ static void handle_ep_complete(struct ci_ep *ep) num = ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; in = (ep->desc->bEndpointAddress & USB_DIR_IN) != 0; - if (num == 0) - ep0_desc.bEndpointAddress = 0; item = ci_get_qtd(num, in); ci_invalidate_qtd(num); @@ -458,11 +467,18 @@ static void handle_ep_complete(struct ci_ep *ep) DBG("ept%d %s req %p, complete %x\n", num, in ? "in" : "out", ci_req, len); - ci_req->req.complete(&ep->ep, &ci_req->req); - if (num == 0) { + if (num != 0 || controller.ep0_data_phase) + ci_req->req.complete(&ep->ep, &ci_req->req); + if (num == 0 && controller.ep0_data_phase) { + /* + * Data Stage is complete, so flip ep0 dir for Status Stage, + * which always transfers a packet in the opposite direction. + */ + DBG("%s: flip ep0 dir for Status Stage\n", __func__); + flip_ep0_direction(); + controller.ep0_data_phase = false; ci_req->req.length = 0; usb_ep_queue(&ep->ep, &ci_req->req, 0); - ep0_desc.bEndpointAddress = USB_DIR_IN; } } @@ -491,8 +507,26 @@ static void handle_setup(void) #else writel(EPT_RX(0), &udc->epstat); #endif - DBG("handle setup %s, %x, %x index %x value %x\n", reqname(r.bRequest), - r.bRequestType, r.bRequest, r.wIndex, r.wValue); + DBG("handle setup %s, %x, %x index %x value %x length %x\n", + reqname(r.bRequest), r.bRequestType, r.bRequest, r.wIndex, + r.wValue, r.wLength); + + /* Set EP0 dir for Data Stage based on Setup Stage data */ + if (r.bRequestType & USB_DIR_IN) { + DBG("%s: Set ep0 to IN for Data Stage\n", __func__); + ep0_desc.bEndpointAddress = USB_DIR_IN; + } else { + DBG("%s: Set ep0 to OUT for Data Stage\n", __func__); + ep0_desc.bEndpointAddress = 0; + } + if (r.wLength) { + controller.ep0_data_phase = true; + } else { + /* 0 length -> no Data Stage. Flip dir for Status Stage */ + DBG("%s: 0 length: flip ep0 dir for Status Stage\n", __func__); + flip_ep0_direction(); + controller.ep0_data_phase = false; + } list_del_init(&ci_req->queue); ci_ep->req_primed = false; -- cgit From 43a8f25b6ca77894ddfd46c2b1196c7bd487561f Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Tue, 10 Jun 2014 11:02:35 -0600 Subject: usb: ci_udc: call udc_disconnect() from ci_pullup() ci_pullup()'s !is_on path contains a cut/paste copy of udc_disconnect(). Remove the duplication by simply calling udc_disconnect() instead. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 6dc20c6c95..5f308563e2 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -697,6 +697,17 @@ int usb_gadget_handle_interrupts(void) return value; } +void udc_disconnect(void) +{ + struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; + /* disable pullup */ + stop_activity(); + writel(USBCMD_FS2, &udc->usbcmd); + udelay(800); + if (controller.driver) + controller.driver->disconnect(&controller.gadget); +} + static int ci_pullup(struct usb_gadget *gadget, int is_on) { struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; @@ -715,27 +726,12 @@ static int ci_pullup(struct usb_gadget *gadget, int is_on) /* Turn on the USB connection by enabling the pullup resistor */ writel(USBCMD_ITC(MICRO_8FRAME) | USBCMD_RUN, &udc->usbcmd); } else { - stop_activity(); - writel(USBCMD_FS2, &udc->usbcmd); - udelay(800); - if (controller.driver) - controller.driver->disconnect(gadget); + udc_disconnect(); } return 0; } -void udc_disconnect(void) -{ - struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; - /* disable pullup */ - stop_activity(); - writel(USBCMD_FS2, &udc->usbcmd); - udelay(800); - if (controller.driver) - controller.driver->disconnect(&controller.gadget); -} - static int ci_udc_probe(void) { struct ept_queue_head *head; -- cgit From bdf81611e444e8aef21cb05eeae69f694c0c7a39 Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Tue, 10 Jun 2014 11:02:36 -0600 Subject: usb: ci_udc: fix freeing of ep0 req ci_ep_alloc_request() avoids allocating multiple request objects for ep0 by keeping a record of the first req allocated for ep0, and always returning that instead of allocating a new req. However, if this req is ever freed, the record of the previous allocation is not cleared, so ci_ep_alloc_request() will keep returning this stale pointer. Fix ci_ep_free_request() to clear the record of the previous allocation. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 5f308563e2..7a6563f83f 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -221,9 +221,14 @@ ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags) static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *req) { - struct ci_req *ci_req; + struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep); + struct ci_req *ci_req = container_of(req, struct ci_req, req); + int num; + + num = ci_ep->desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; + if (num == 0) + controller.ep0_req = 0; - ci_req = container_of(req, struct ci_req, req); if (ci_req->b_buf) free(ci_req->b_buf); free(ci_req); -- cgit From 9a7d34be13a6934e0fddfaf12236d8784343f902 Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Tue, 10 Jun 2014 11:02:37 -0600 Subject: usb: ci_udc: fix probe error cleanup If allocation of the ep0 req fails, clean up all the allocations that were made in ci_udc_probe(). Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 7a6563f83f..1428af85cf 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -826,6 +826,7 @@ static int ci_udc_probe(void) ci_ep_alloc_request(&controller.ep[0].ep, 0); if (!controller.ep0_req) { + free(controller.items_mem); free(controller.epts); return -ENOMEM; } -- cgit From b7c0051687f42173e15b151a74e524587e8b59db Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Tue, 10 Jun 2014 11:02:38 -0600 Subject: usb: ci_udc: clean up all allocations in unregister usb_gadget_unregister_driver() is called to tear down the USB device mode stack. Fix the driver to stop the USB HW (which causes any attached host to notice the disappearance of the device), and free all allocations (which obviously prevents memory leaks). Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 1428af85cf..435a2720e9 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -875,5 +875,11 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) { + udc_disconnect(); + + ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req); + free(controller.items_mem); + free(controller.epts); + return 0; } -- cgit From e0672b3c3ae4fdd48a66af9f7932d2c8e839e459 Mon Sep 17 00:00:00 2001 From: Stephen Warren Date: Tue, 10 Jun 2014 15:27:39 -0600 Subject: usb: ci_udc: terminate ep0 INs with a zlp when required Sometimes, a zero-length packet is required at the end of an IN transaction so that the host knows the device is done sending data. Enhance ci_udc to send a zlp when necessary. See the comments for more details. Signed-off-by: Stephen Warren --- drivers/usb/gadget/ci_udc.c | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) (limited to 'drivers/usb/gadget/ci_udc.c') diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c index 435a2720e9..b18bee43ad 100644 --- a/drivers/usb/gadget/ci_udc.c +++ b/drivers/usb/gadget/ci_udc.c @@ -369,18 +369,50 @@ static void ci_ep_submit_next_request(struct ci_ep *ci_ep) ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue); len = ci_req->req.length; - item->next = TERMINATE; - item->info = INFO_BYTES(len) | INFO_IOC | INFO_ACTIVE; + item->info = INFO_BYTES(len) | INFO_ACTIVE; item->page0 = (uint32_t)ci_req->hw_buf; item->page1 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x1000; item->page2 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x2000; item->page3 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x3000; item->page4 = ((uint32_t)ci_req->hw_buf & 0xfffff000) + 0x4000; - ci_flush_qtd(num); head->next = (unsigned) item; head->info = 0; + /* + * When sending the data for an IN transaction, the attached host + * knows that all data for the IN is sent when one of the following + * occurs: + * a) A zero-length packet is transmitted. + * b) A packet with length that isn't an exact multiple of the ep's + * maxpacket is transmitted. + * c) Enough data is sent to exactly fill the host's maximum expected + * IN transaction size. + * + * One of these conditions MUST apply at the end of an IN transaction, + * or the transaction will not be considered complete by the host. If + * none of (a)..(c) already applies, then we must force (a) to apply + * by explicitly sending an extra zero-length packet. + */ + /* IN !a !b !c */ + if (in && len && !(len % ci_ep->ep.maxpacket) && ci_req->req.zero) { + /* + * Each endpoint has 2 items allocated, even though typically + * only 1 is used at a time since either an IN or an OUT but + * not both is queued. For an IN transaction, item currently + * points at the second of these items, so we know that we + * can use (item - 1) to transmit the extra zero-length packet + */ + item->next = (unsigned)(item - 1); + item--; + item->info = INFO_ACTIVE; + } + + item->next = TERMINATE; + item->info |= INFO_IOC; + + ci_flush_qtd(num); + DBG("ept%d %s queue len %x, req %p, buffer %p\n", num, in ? "in" : "out", len, ci_req, ci_req->hw_buf); ci_flush_qh(num); -- cgit