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 of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
