From stern@rowland.harvard.edu Tue Aug 21 12:39:33 2007 From: Alan Stern Date: Tue, 21 Aug 2007 15:39:21 -0400 (EDT) Subject: USB: centralize -EREMOTEIO handling To: Greg KH Cc: David Brownell , Olav Kongas , Yoshihiro Shimoda , Tony Olech Message-ID: This patch (as969) continues the ongoing changes to the way HCDs report URB statuses. The programming interface has been simplified by making usbcore responsible for clearing urb->hcpriv and for setting -EREMOTEIO status when an URB with the URB_SHORT_NOT_OK flag ends up as a short transfer. By moving the work out of the HCDs, this removes a fair amount of repeated code. Signed-off-by: Alan Stern CC: David Brownell CC: Olav Kongas CC: Yoshihiro Shimoda CC: Tony Olech Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/hcd.c | 9 +++++++-- drivers/usb/gadget/dummy_hcd.c | 9 ++------- drivers/usb/host/ehci-q.c | 7 ++++--- drivers/usb/host/isp116x-hcd.c | 8 +------- drivers/usb/host/ohci-q.c | 11 ----------- drivers/usb/host/r8a66597-hcd.c | 7 +------ drivers/usb/host/sl811-hcd.c | 14 +++----------- drivers/usb/host/u132-hcd.c | 5 ----- drivers/usb/host/uhci-q.c | 8 -------- 9 files changed, 18 insertions(+), 60 deletions(-) --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -366,6 +366,7 @@ static int rh_call_control (struct usb_h spin_unlock_irq(&hcd_root_hub_lock); if (status) return status; + urb->hcpriv = hcd; /* Indicate it's queued */ cmd = (struct usb_ctrlrequest *) urb->setup_packet; typeReq = (cmd->bRequestType << 8) | cmd->bRequest; @@ -579,7 +580,6 @@ void usb_hcd_poll_rh_status(struct usb_h hcd->poll_pending = 0; hcd->status_urb = NULL; urb->status = 0; - urb->hcpriv = NULL; urb->actual_length = length; memcpy(urb->transfer_buffer, buffer, length); @@ -675,7 +675,6 @@ static int usb_rh_urb_dequeue(struct usb del_timer (&hcd->rh_timer); if (urb == hcd->status_urb) { hcd->status_urb = NULL; - urb->hcpriv = NULL; usb_hcd_unlink_urb_from_ep(hcd, urb); spin_unlock(&hcd_root_hub_lock); @@ -1192,6 +1191,7 @@ int usb_hcd_submit_urb (struct urb *urb, if (unlikely(status)) { usbmon_urb_submit_error(&hcd->self, urb, status); unmap_urb_for_dma(hcd, urb); + urb->hcpriv = NULL; INIT_LIST_HEAD(&urb->urb_list); atomic_dec(&urb->use_count); if (urb->reject) @@ -1265,6 +1265,11 @@ void usb_hcd_giveback_urb (struct usb_hc unmap_urb_for_dma(hcd, urb); usbmon_urb_complete (&hcd->self, urb); usb_unanchor_urb(urb); + urb->hcpriv = NULL; + if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && + urb->actual_length < urb->transfer_buffer_length && + !urb->status)) + urb->status = -EREMOTEIO; /* pass ownership to the completion handler */ urb->complete (urb); --- a/drivers/usb/gadget/dummy_hcd.c +++ b/drivers/usb/gadget/dummy_hcd.c @@ -1099,8 +1099,7 @@ top: * * partially filling a buffer optionally blocks queue advances * (so completion handlers can clean up the queue) but we don't - * need to emulate such data-in-flight. so we only show part - * of the URB_SHORT_NOT_OK effect: completion status. + * need to emulate such data-in-flight. */ if (is_short) { if (host_len == dev_len) { @@ -1111,10 +1110,7 @@ top: if (dev_len > host_len) maybe_set_status (urb, -EOVERFLOW); else - maybe_set_status (urb, - (urb->transfer_flags - & URB_SHORT_NOT_OK) - ? -EREMOTEIO : 0); + maybe_set_status (urb, 0); } else if (!to_host) { maybe_set_status (urb, 0); if (host_len > dev_len) @@ -1516,7 +1512,6 @@ restart: continue; return_urb: - urb->hcpriv = NULL; list_del (&urbp->urbp_list); kfree (urbp); if (ep) --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -232,7 +232,6 @@ __acquires(ehci->lock) } spin_lock (&urb->lock); - urb->hcpriv = NULL; switch (urb->status) { case -EINPROGRESS: /* success */ urb->status = 0; @@ -395,8 +394,10 @@ halt: /* remove it from the queue */ spin_lock (&urb->lock); qtd_copy_status (ehci, urb, qtd->length, token); - do_status = (urb->status == -EREMOTEIO) - && usb_pipecontrol (urb->pipe); + if (unlikely(urb->status == -EREMOTEIO)) { + do_status = usb_pipecontrol(urb->pipe); + urb->status = 0; + } spin_unlock (&urb->lock); if (stopped && qtd->qtd_list.prev != &qh->qtd_list) { --- a/drivers/usb/host/isp116x-hcd.c +++ b/drivers/usb/host/isp116x-hcd.c @@ -282,7 +282,6 @@ __releases(isp116x->lock) __acquires(isp { unsigned i; - urb->hcpriv = NULL; ep->error_count = 0; if (usb_pipecontrol(urb->pipe)) @@ -446,12 +445,7 @@ static void postproc_atl_queue(struct is if (PTD_GET_ACTIVE(ptd) || (cc != TD_CC_NOERROR && cc < 0x0E)) break; - if ((urb->transfer_flags & URB_SHORT_NOT_OK) && - urb->actual_length < - urb->transfer_buffer_length) - status = -EREMOTEIO; - else - status = 0; + status = 0; ep->nextpid = 0; break; default: --- a/drivers/usb/host/ohci-q.c +++ b/drivers/usb/host/ohci-q.c @@ -43,21 +43,10 @@ __acquires(ohci->lock) // ASSERT (urb->hcpriv != 0); urb_free_priv (ohci, urb->hcpriv); - urb->hcpriv = NULL; spin_lock (&urb->lock); if (likely (urb->status == -EINPROGRESS)) urb->status = 0; - /* report short control reads right even though the data TD always - * has TD_R set. (much simpler, but creates the 1-td limit.) - */ - if (unlikely (urb->transfer_flags & URB_SHORT_NOT_OK) - && unlikely (usb_pipecontrol (urb->pipe)) - && urb->actual_length < urb->transfer_buffer_length - && usb_pipein (urb->pipe) - && urb->status == 0) { - urb->status = -EREMOTEIO; - } spin_unlock (&urb->lock); switch (usb_pipetype (urb->pipe)) { --- a/drivers/usb/host/r8a66597-hcd.c +++ b/drivers/usb/host/r8a66597-hcd.c @@ -783,7 +783,6 @@ static void force_dequeue(struct r8a6659 if (urb) { urb->status = -ENODEV; - urb->hcpriv = NULL; usb_hcd_unlink_urb_from_ep(r8a66597_to_hcd(r8a66597), urb); @@ -1134,7 +1133,6 @@ __releases(r8a66597->lock) __acquires(r8 if (usb_pipeisoc(urb->pipe)) urb->start_frame = r8a66597_get_frame(hcd); - urb->hcpriv = NULL; usb_hcd_unlink_urb_from_ep(r8a66597_to_hcd(r8a66597), urb); spin_unlock(&r8a66597->lock); @@ -1202,9 +1200,6 @@ static void packet_read(struct r8a66597 td->zero_packet = 1; if (rcv_len < bufsize) { td->short_packet = 1; - if (urb->transfer_buffer_length != urb->actual_length && - urb->transfer_flags & URB_SHORT_NOT_OK) - status = -EREMOTEIO; } if (usb_pipeisoc(urb->pipe)) { urb->iso_frame_desc[td->iso_cnt].actual_length = size; @@ -1214,7 +1209,7 @@ static void packet_read(struct r8a66597 } /* check transfer finish */ - if (check_transfer_finish(td, urb)) { + if (finish || check_transfer_finish(td, urb)) { pipe_stop(r8a66597, td->pipe); pipe_irq_disable(r8a66597, pipenum); finish = 1; --- a/drivers/usb/host/sl811-hcd.c +++ b/drivers/usb/host/sl811-hcd.c @@ -438,7 +438,6 @@ static void finish_request( spin_lock(&urb->lock); if (urb->status == -EINPROGRESS) urb->status = status; - urb->hcpriv = NULL; spin_unlock(&urb->lock); usb_hcd_unlink_urb_from_ep(sl811_to_hcd(sl811), urb); @@ -545,17 +544,10 @@ done(struct sl811 *sl811, struct sl811h_ sl811_read_buf(sl811, SL811HS_PACKET_BUF(bank == 0), buf, len); usb_dotoggle(udev, ep->epnum, 0); - if (urb->actual_length == urb->transfer_buffer_length) + if (urb->actual_length == urb->transfer_buffer_length + || len < ep->maxpacket) urbstat = 0; - else if (len < ep->maxpacket) { - if (urb->transfer_flags & URB_SHORT_NOT_OK) - urbstat = -EREMOTEIO; - else - urbstat = 0; - } - if (usb_pipecontrol(urb->pipe) - && (urbstat == -EREMOTEIO - || urbstat == 0)) { + if (usb_pipecontrol(urb->pipe) && urbstat == 0) { /* NOTE if the status stage STALLs (why?), * this reports the wrong urb status. --- a/drivers/usb/host/u132-hcd.c +++ b/drivers/usb/host/u132-hcd.c @@ -519,7 +519,6 @@ static void u132_hcd_giveback_urb(struct struct usb_hcd *hcd = u132_to_hcd(u132); urb->error_count = 0; urb->status = status; - urb->hcpriv = NULL; spin_lock_irqsave(&endp->queue_lock.slock, irqs); usb_hcd_unlink_urb_from_ep(hcd, urb); endp->queue_next += 1; @@ -560,7 +559,6 @@ static void u132_hcd_abandon_urb(struct struct usb_hcd *hcd = u132_to_hcd(u132); urb->error_count = 0; urb->status = status; - urb->hcpriv = NULL; spin_lock_irqsave(&endp->queue_lock.slock, irqs); usb_hcd_unlink_urb_from_ep(hcd, urb); endp->queue_next += 1; @@ -2430,7 +2428,6 @@ static int dequeue_from_overflow_chain(s list_del(scan); endp->queue_size -= 1; urb->error_count = 0; - urb->hcpriv = NULL; usb_hcd_giveback_urb(hcd, urb); return 0; } else @@ -2472,7 +2469,6 @@ static int u132_endp_urb_dequeue(struct endp->edset_flush = 1; u132_endp_queue_work(u132, endp, 0); spin_unlock_irqrestore(&endp->queue_lock.slock, irqs); - urb->hcpriv = NULL; return 0; } else { spin_unlock_irqrestore(&endp->queue_lock.slock, irqs); @@ -2517,7 +2513,6 @@ static int u132_endp_urb_dequeue(struct irqs); kfree(urbq); } urb->error_count = 0; - urb->hcpriv = NULL; usb_hcd_giveback_urb(hcd, urb); return 0; } else if (list_empty(&endp->urb_more)) { --- a/drivers/usb/host/uhci-q.c +++ b/drivers/usb/host/uhci-q.c @@ -757,7 +757,6 @@ static void uhci_free_urb_priv(struct uh uhci_free_td(uhci, td); } - urbp->urb->hcpriv = NULL; kmem_cache_free(uhci_up_cachep, urbp); } @@ -1494,13 +1493,6 @@ __acquires(uhci->lock) * unlinked first. Regardless, don't confuse people with a * negative length. */ urb->actual_length = max(urb->actual_length, 0); - - /* Report erroneous short transfers */ - if (unlikely((urb->transfer_flags & URB_SHORT_NOT_OK) && - urb->actual_length < - urb->transfer_buffer_length && - urb->status == 0)) - urb->status = -EREMOTEIO; } /* When giving back the first URB in an Isochronous queue,