Re: [linux-usb-devel] Possible bug in isp116x-hcd
On Sat, Jun 23, 2007 at 08:25:55PM -0400, Alan Stern wrote: Below is a patch based on your work. There's a number of differences, plus some things I left as-is but which look suspicious. Thanks for the review. You touched the part of the code that took the lognest time to stabilize. It took me time to understand it again. I took out the references to urb-lock in preproc_atl_queue and isp116x_urb_enqueue. They didn't seem to serve any purpose; is that correct? Seems to be correct. urb-hcpriv is already protected by isp116x-lock. Is urb-lock entirely to guard only urb-status? If so, is the urb-lock unnecessary also in preproc_atl_queue()? All the other updates are in postproc_atl_queue: In the revised code, short_not_ok doesn't serve a clear purpose. I changed it to short_packet_received; now it works somewhat differently and it should be easier to understand. There's a new local variable called status which indicates whether the URB is completed and holds the final status value. The code at the end of the big loop uses the variable to set urb-status. I anticipated that :) There's a comment with associated code: /* According to usb spec, zero-length Int transfer signals finishing of the urb. Hey, does this apply only for IN endpoints? */ This doesn't make any sense to me. Firstly, zero-length packets signal the end of any Control-, Bulk-, or Int-IN transfer; there's nothing special about Int. Secondly, with OUT transfers the only way you can get a zero-length packet is if it is the last packet of the transfer anyway; all the packets before the last must be of maximum length. I left that stuff in but it probably should be removed. I remember the purpose of explicitly handling Int transfers at that point was to keep the following code simpler. Maybe it did some time in the past, but now it seems to be unnecessary indeed. The question in the comment was about Int-OUT transfers. In this section: /* Relax after previously failed, but later succeeded or correctly NAK'ed retransmission attempt */ if (ep-error_count (cc == TD_CC_NOERROR || cc == TD_NOTACCESSED)) ep-error_count = 0; there's no need to test ep-error_count; it's easier just to set it to 0. OK In the big switch (ep-nextpid) block, I think it would be better to factor this test: if (PTD_GET_ACTIVE(ptd) || (cc != TD_CC_NOERROR cc 0x0E)) break; out of the individual cases. On the other hand, I'm not entirely sure what the test is intended for. Does it have something to do with retrying a failed transaction? PTD_GET_ACTIVE(ptd) == 1 means that the transfer got only NAKed during the frame. The rest of the check is about retrying on soft errors. The USB_PID_IN/USB_PID OUT case got changed considerably. I think the logic is pretty much the same as it was before, but it makes more sense to me like this. This test if (urb-transfer_flags URB_ZERO_PACKET ep-nextpid == USB_PID_OUT !(PTD_GET_COUNT(ptd) % ep-maxpacket)) { DBG(Zero packet requested\n); seems wrong. Wouldn't it get stuck in an infinite loop? I think the last part of the test ought to be more like: PTD_GET_COUNT(ptd) == ep-maxpacket except that wouldn't be quite right when the zero-length packet has to be retried. Good question. It did not hang in my tests. But I don't understand, why. Tests will show. PTD_GET_COUNT(ptd) can be multiple of ep-maxpacket. In the USB_PID_SETUP case, this test if (urb-transfer_buffer_length == urb-actual_length) ep-nextpid = USB_PID_ACK; might be a little clearer if it was changed to if (urb-transfer_buffer_length == 0) ep-nextpid = USB_PID_ACK; Overall, I'm not sure that the toggle setting is correct in all the possible pathways. There were so many changes, it's hard to track them. In particular, I don't see where the toggle gets set to 1 for the status stage of a control transfer. Toggle for status stage is set in preproc_atl_queue(). Anyway, I can't check this as carefully as you can. Let me know what you think. Sure this preproc_atl_queue() has suffered from my attitude better not touch it if it works. Good that it can be simplified. Thanks for your review! Right now I don't have the hardware available. But I am working on it. These changes are big and have to be tested, no doubt. I will let you know. Olav - This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version
Re: [linux-usb-devel] Possible bug in isp116x-hcd
On Thu, Jun 21, 2007 at 05:17:42PM -0400, Alan Stern wrote: On Thu, 21 Jun 2007, ok wrote: Do you mean something like that below? I haven't tested it. Yes, something very much like this. It could be simplified a little; in a couple of places where you have finish_request(); spin_unlock(); continue; you ought to be able to use a goto or break instead. Also there are a few minor problems, like calling finish_request before it is declared and calling it before releasing urb-lock, but those can easily be fixed. Yes, I made a mistake with urb-lock. OK, moved finish_request unmodified up in the file, got fatter patch. The one below. If you like, I'll try revising your patch. You'll have to test it Yes, thanks. though; I don't have the hardware. Sure. Olav --- linux-2.6.22-rc5-or/drivers/usb/host/isp116x-hcd.c 2007-06-17 05:09:12.0 +0300 +++ linux-2.6.22-rc5-isp/drivers/usb/host/isp116x-hcd.c 2007-06-23 00:22:01.0 +0300 @@ -275,6 +275,61 @@ } /* + Take done or failed requests out of schedule. Give back + processed urbs. +*/ +static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep, + struct urb *urb) +__releases(isp116x-lock) __acquires(isp116x-lock) +{ + unsigned i; + + urb-hcpriv = NULL; + ep-error_count = 0; + + if (usb_pipecontrol(urb-pipe)) + ep-nextpid = USB_PID_SETUP; + + urb_dbg(urb, Finish); + + spin_unlock(isp116x-lock); + usb_hcd_giveback_urb(isp116x_to_hcd(isp116x), urb); + spin_lock(isp116x-lock); + + /* take idle endpoints out of the schedule */ + if (!list_empty(ep-hep-urb_list)) + return; + + /* async deschedule */ + if (!list_empty(ep-schedule)) { + list_del_init(ep-schedule); + return; + } + + /* periodic deschedule */ + DBG(deschedule qh%d/%p branch %d\n, ep-period, ep, ep-branch); + for (i = ep-branch; i PERIODIC_SIZE; i += ep-period) { + struct isp116x_ep *temp; + struct isp116x_ep **prev = isp116x-periodic[i]; + + while (*prev ((temp = *prev) != ep)) + prev = temp-next; + if (*prev) + *prev = ep-next; + isp116x-load[i] -= ep-load; + } + ep-branch = PERIODIC_SIZE; + isp116x_to_hcd(isp116x)-self.bandwidth_allocated -= + ep-load / ep-period; + + /* switch irq type? */ + if (!--isp116x-periodic_count) { + isp116x-irqenb = ~HCuPINT_SOF; + isp116x-irqenb |= HCuPINT_ATL; + } +} + +/* Analyze transfer results, handle partial transfers and errors */ static void postproc_atl_queue(struct isp116x *isp116x) @@ -296,11 +351,6 @@ short_not_ok = 1; spin_lock(urb-lock); - /* Data underrun is special. For allowed underrun - we clear the error and continue as normal. For - forbidden underrun we finish the DATA stage - immediately while for control transfer, - we do a STATUS stage. */ if (cc == TD_DATAUNDERRUN) { if (!(urb-transfer_flags URB_SHORT_NOT_OK)) { DBG(Allowed data underrun\n); @@ -308,22 +358,14 @@ short_not_ok = 0; } else { ep-error_count = 1; - if (usb_pipecontrol(urb-pipe)) - ep-nextpid = USB_PID_ACK; - else - usb_settoggle(udev, ep-epnum, - ep-nextpid == - USB_PID_OUT, - PTD_GET_TOGGLE(ptd)); + usb_settoggle(udev, ep-epnum, + ep-nextpid == USB_PID_OUT, + PTD_GET_TOGGLE(ptd)); urb-actual_length += PTD_GET_COUNT(ptd); urb-status = cc_to_error[TD_DATAUNDERRUN]; - spin_unlock(urb-lock); - continue; + goto done; } } - /* Keep underrun error through the STATUS stage */ - if (urb-status == cc_to_error[TD_DATAUNDERRUN]) - cc = TD_DATAUNDERRUN; if (cc != TD_CC_NOERROR cc != TD_NOTACCESSED (++ep-error_count = 3 || cc == TD_CC_STALL @@ -332,8 +374,7 @@ urb-status = cc_to_error[cc]; if (ep-nextpid == USB_PID_ACK
Re: [linux-usb-devel] Possible bug in isp116x-hcd
On Mon, Jun 18, 2007 at 11:00:10AM -0400, Alan Stern wrote: Olav, What would be involved in refactoring isp116x-hcd so that the driver called usb_hcd_giveback_urb() as soon as the URB was completed? If I understand the current code correctly, postproc_atl_queue() goes through all the active endpoints and stores the status for the completed URBs, and then later finish_atl_transfers() gives them back. It looks like this shouldn't present any difficulty except for one thing: When you get a short Control-IN transfer with URB_SHORT_NOT_OK set, the error status gets stored while the URB's status stage executes. That would have to be changed so that the status stage was skipped entirely. Doing this shouldn't cause any problems in practice; the other HCDs skip the status stage when this sort of error occurs. The point of making this change is that it would allow the driver to avoid saving values in urb-status, leading eventually to the possibility of removing the status field from struct urb. Hi Alan, Do you mean something like that below? I haven't tested it. Olav --- linux-2.6.22-rc5-or/drivers/usb/host/isp116x-hcd.c 2007-06-17 05:09:12.0 +0300 +++ linux-2.6.22-rc5-isp/drivers/usb/host/isp116x-hcd.c 2007-06-20 22:49:27.0 +0300 @@ -210,6 +210,9 @@ /*---*/ +static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep, + struct urb *urb); + /* Set up PTD's. */ @@ -296,11 +299,6 @@ short_not_ok = 1; spin_lock(urb-lock); - /* Data underrun is special. For allowed underrun - we clear the error and continue as normal. For - forbidden underrun we finish the DATA stage - immediately while for control transfer, - we do a STATUS stage. */ if (cc == TD_DATAUNDERRUN) { if (!(urb-transfer_flags URB_SHORT_NOT_OK)) { DBG(Allowed data underrun\n); @@ -308,22 +306,16 @@ short_not_ok = 0; } else { ep-error_count = 1; - if (usb_pipecontrol(urb-pipe)) - ep-nextpid = USB_PID_ACK; - else - usb_settoggle(udev, ep-epnum, - ep-nextpid == - USB_PID_OUT, - PTD_GET_TOGGLE(ptd)); + usb_settoggle(udev, ep-epnum, + ep-nextpid == USB_PID_OUT, + PTD_GET_TOGGLE(ptd)); urb-actual_length += PTD_GET_COUNT(ptd); urb-status = cc_to_error[TD_DATAUNDERRUN]; + finish_request(isp116x, ep, urb); spin_unlock(urb-lock); continue; } } - /* Keep underrun error through the STATUS stage */ - if (urb-status == cc_to_error[TD_DATAUNDERRUN]) - cc = TD_DATAUNDERRUN; if (cc != TD_CC_NOERROR cc != TD_NOTACCESSED (++ep-error_count = 3 || cc == TD_CC_STALL @@ -332,6 +324,7 @@ urb-status = cc_to_error[cc]; if (ep-nextpid == USB_PID_ACK) ep-nextpid = 0; + finish_request(isp116x, ep, urb); spin_unlock(urb-lock); continue; } @@ -341,6 +334,7 @@ if (usb_pipeint(urb-pipe) !PTD_GET_LEN(ptd)) { if (urb-status == -EINPROGRESS) urb-status = 0; + finish_request(isp116x, ep, urb); spin_unlock(urb-lock); continue; } @@ -409,6 +403,8 @@ default: BUG(); } + if (urb-status != -EINPROGRESS) + finish_request(isp116x, ep, urb); spin_unlock(urb-lock); } } @@ -570,9 +566,6 @@ */ static void finish_atl_transfers(struct isp116x *isp116x) { - struct isp116x_ep *ep; - struct urb *urb; - if (!isp116x-atl_active) return; /* Fifo not ready? */ @@ -582,16 +575,6 @@ atomic_inc(isp116x-atl_finishing); unpack_fifo(isp116x); postproc_atl_queue(isp116x); - for (ep = isp116x-atl_active; ep; ep = ep-active) { - urb = -