<div dir="ltr"><div>Hi All<br><br>I have been debugging some usb errors that I have been encountering and here are my conclusions:<br><br></div>-- The error is a BULK IN transfer error, after adding further code to display the internal message:<br>
<br>BULK IN transfer error; rc=-5 : libusb0-dll:err [_usb_reap_async] reaping request failed, win error: A device attached to the system is not functioning.<br><div><br></div><div>-- BUT when I used USBLyzer to analyze the underlying problem, it's actually a [buffer overflow] problem.<br>
<br></div><div>1) According to usb specification, USB full speed device maximum data packet payload size is 1023<br></div><div><br><a href="http://www.usb.org/developers/docs/usb_20_070113.zip" target="_blank">http://www.usb.org/developers/docs/usb_20_070113.zip</a><br>
<br></div><div>2) According to the AT91SAM7S device specification, its USB endpoint size is 64 bytes.<br><br></div><div>3) According to libusb api description, bulk transfer overflow is caused by too small buffer ( <a href="http://libusb.sourceforge.net/api-1.0/packetoverflow.html">http://libusb.sourceforge.net/api-1.0/packetoverflow.html</a> ). Should use multiple of the device payload size.<br>
<br></div><div>4) Since we don't have a size indicator in our USB protocol (our header is 4 bytes, indicating ATR, wait timeout flag etc), the client software must not see more than 1 req_ctx per bulk_usb_transfer call<br>
<br></div><div>Therefore, we SHOULD:<br><br></div><div>>> A << Use a multiple of 64 bytes as req_ctx size, and must be <= 1023 [per USB specification]. I chose 960. Consequently, there is only ONE size for req_ctx, no more large / small)<br>
<br></div><div>>> B << Desktop/Host software MUST use the same size in static void run_mainloop(struct usb_dev_handle *devh). Otherwise, multiple req_ctx WILL be combined by the usb driver / libusb and the additional req_ctx header in the middle will confuse the apdu_splitter<br>
<br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Sep 4, 2013 at 10:37 PM, Min Xu <span dir="ltr"><<a href="mailto:mxu@sanjole.com" target="_blank">mxu@sanjole.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi Harald<br><br></div>Thank you for getting back to me so quickly. I am using Emacs on an Ubuntu netbook to do the edits, which likes to reformat sections as I move (copy/paste section around) and I haven't figured out a command to undo those changes. It'd be nice if there is a .emacsrc or something that automatically formats the code to the project standard? (presumably someone has one? ..)<br>
<br><div class="gmail_extra">I started only wanted to make one change and submit (was initially the req_ctx), but couldn't reliably verify it until the debugp was working better without affecting the system, so the changes ballooned up to this stage. So I wanted to submit the changes before I change the code further. I will try limit the scope of changes in my future updates.<br>
<br></div><div class="gmail_extra">To your questions:<br><br></div><div class="gmail_extra">1) The numerical values of the RCTX_STATE are changed to facilitate the choosing of the FIFO/double linked list of req_ctx to use. An array of FIFO, using the state to index into the array to get the FIFO is simplest. When I looked how the reqctx_find / ... functions are used, I found all callers use the constant name rather than any specific numeric value.<br>
<br></div><div class="gmail_extra">2) I will look at this tomorrow. But I was under the impression on how the lib_Atmel?? (sorry, can't access the source at the moment) was invoked to install the irq handler, that the entry point to the interrupt would be directly the function passed in. I looked at the sysirq_handler.c?? more closely than the other so I wasn't aware of the Cstartup.c file<br>
<br></div><div class="gmail_extra">Since I increased the req_ctx buffer size to 1kb each, most req_ctx that are transferred to pc contain many many apdus but very often, the req_ctx do not begin at the start of an apdu. So I would like to have an offset (2 byte, since it could be > 256) to indicate the position of the first start of an apdu. After thinking about it more, I would also like to add a sequence number into the header (making the header 8 bytes then?) to better allow for detecting dropped req_ctx (if there are any..). The offset would be 8 for a req_ctx that starts with a new APDU, and would be 0 for an req_ctx that only contains a middle or last fragment of an APDU. <br>
<br></div><div class="gmail_extra">I have already modified the host software to run on Windows (currently using Windows 7 x64), so I will probably make those changes locally only to help me detect any problem I might still be facing. <br>
</div><div class="gmail_extra"><br></div><div class="gmail_extra">Again, thank you very much for getting back so quickly, and I appreciate your product very much.<br><br></div><div class="gmail_extra">Best Regards<br></div>
<div><div class="h5">
<div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_quote">On Wed, Sep 4, 2013 at 9:31 PM, Harald Welte <span dir="ltr"><<a href="mailto:laforge@gnumonks.org" target="_blank">laforge@gnumonks.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Dear Min Xu,<br>
<div><br>
On Wed, Sep 04, 2013 at 07:47:46PM -1000, Min Xu wrote:<br>
> I made a bunch of changes that significantly improved my test scenario:<br>
<br>
</div>I _really_ appreciate your excellent technical work. However, I would<br>
like you to go the little extra effort it takes to 'properly' interact<br>
with community based Free Software projects, where there are rules like<br>
<br>
* only one feature / logical change per patch, resulting in a series<br>
of incremental patches, each taking us one step further, each<br>
rendering a compile-able and functional build after being applied.<br>
* no re-formatting of code (whitespace changes)<br>
* no patch for local changes like different compiler name in Makefile<br>
* stick to coding style of the project (tab-wide indent, ...) for<br>
consistency<br>
* remove old/dead code rather than comment it out (RCTX_STATE_...)<br>
* avoid asm() statements whenever possible. If you need them, please<br>
wrap them in an inline C function with descriptive name.<br>
<br>
Some quesetions:<br>
* why did the numberic values of RCTX_STATE_* have t be changed rather<br>
than amended by your new values, keeping the old ones as-is?<br>
<br>
* interrupt nesting _should_ have been active all the time, see<br>
IRQ_Handler_Entry in Cstartup.S, where we first save SPSR and then<br>
un-set the IRQ and FIQ bits _before_ branching to the interrupt<br>
handler function:<br>
<br>
/*- Enable Interrupt and Switch in Supervisor Mode */<br>
msr CPSR_c, #ARM_MODE_SVC<br>
<br>
Where ARM_MODE_SVC is set to 0x13, i.e. without 0x40 or 0x80.<br>
<br>
So if there's something wrong with the existing code, it should be<br>
fixed there rather than two copies of inline-asm in the USART and USB<br>
IRQ handler routines.<br>
<br>
So I do want to merge this very much, but I think it needs clean-up<br>
before it can be merged. I don't expect to have much time for this in<br>
the next couple of weeks, so I would be happy if either you or somebody<br>
else on the list could work on this.<br>
<br>
Regarding your comment about changing the USB protocol: This would lead<br>
to host / firmware version incompatibilities, and I'd like to avoid that<br>
if possible in any way. We already have the simtrace_hdr.flags<br>
structure member, which contains things like SIMTRACE_FLAG_ATR. I'm not<br>
sure if I understand your request fully, but why not simply add flags<br>
like:<br>
<br>
* First fragment of a fragmented APDU<br>
* More fragments to follow for this APDU<br>
<br>
This way a new APDU still has to start at the beginning of a USB<br>
transfer, but no changes to the simtrace_hdr are required.<br>
<br>
<br>
Thanks again,<br>
<div><div> Harald<br>
<br>
<br>
--<br>
- Harald Welte <<a href="mailto:laforge@gnumonks.org" target="_blank">laforge@gnumonks.org</a>> <a href="http://laforge.gnumonks.org/" target="_blank">http://laforge.gnumonks.org/</a><br>
============================================================================<br>
"Privacy in residential applications is a desirable marketing option."<br>
(ETSI EN 300 175-7 Ch. A6)<br>
</div></div></blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>