Hi All
I have been debugging some usb errors that I have been encountering and
here are my conclusions:
-- The error is a BULK IN transfer error, after adding further code to
display the internal message:
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.
-- BUT when I used USBLyzer to analyze the underlying problem, it's
actually a [buffer overflow] problem.
1) According to usb specification, USB full speed device maximum data
packet payload size is 1023
http://www.usb.org/developers/docs/usb_20_070113.zip
2) According to the AT91SAM7S device specification, its USB endpoint size
is 64 bytes.
3) According to libusb api description, bulk transfer overflow is caused
by too small buffer (
http://libusb.sourceforge.net/api-1.0/packetoverflow.html ). Should use
multiple of the device payload size.
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
Therefore, we SHOULD:
> 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)
> 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
On Wed, Sep 4, 2013 at 10:37 PM, Min Xu <mxu(a)sanjole.com> wrote:
Hi Harald
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? ..)
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.
To your questions:
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.
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
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.
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.
Again, thank you very much for getting back so quickly, and I appreciate
your product very much.
Best Regards
On Wed, Sep 4, 2013 at 9:31 PM, Harald Welte <laforge(a)gnumonks.org> wrote:
Dear Min Xu,
On Wed, Sep 04, 2013 at 07:47:46PM -1000, Min Xu wrote:
I made a bunch of changes that significantly
improved my test scenario:
I _really_ appreciate your excellent technical work. However, I would
like you to go the little extra effort it takes to 'properly' interact
with community based Free Software projects, where there are rules like
* only one feature / logical change per patch, resulting in a series
of incremental patches, each taking us one step further, each
rendering a compile-able and functional build after being applied.
* no re-formatting of code (whitespace changes)
* no patch for local changes like different compiler name in Makefile
* stick to coding style of the project (tab-wide indent, ...) for
consistency
* remove old/dead code rather than comment it out (RCTX_STATE_...)
* avoid asm() statements whenever possible. If you need them, please
wrap them in an inline C function with descriptive name.
Some quesetions:
* why did the numberic values of RCTX_STATE_* have t be changed rather
than amended by your new values, keeping the old ones as-is?
* interrupt nesting _should_ have been active all the time, see
IRQ_Handler_Entry in Cstartup.S, where we first save SPSR and then
un-set the IRQ and FIQ bits _before_ branching to the interrupt
handler function:
/*- Enable Interrupt and Switch in Supervisor Mode */
msr CPSR_c, #ARM_MODE_SVC
Where ARM_MODE_SVC is set to 0x13, i.e. without 0x40 or 0x80.
So if there's something wrong with the existing code, it should be
fixed there rather than two copies of inline-asm in the USART and USB
IRQ handler routines.
So I do want to merge this very much, but I think it needs clean-up
before it can be merged. I don't expect to have much time for this in
the next couple of weeks, so I would be happy if either you or somebody
else on the list could work on this.
Regarding your comment about changing the USB protocol: This would lead
to host / firmware version incompatibilities, and I'd like to avoid that
if possible in any way. We already have the simtrace_hdr.flags
structure member, which contains things like SIMTRACE_FLAG_ATR. I'm not
sure if I understand your request fully, but why not simply add flags
like:
* First fragment of a fragmented APDU
* More fragments to follow for this APDU
This way a new APDU still has to start at the beginning of a USB
transfer, but no changes to the simtrace_hdr are required.
Thanks again,
Harald
--
- Harald Welte <laforge(a)gnumonks.org>
http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch.
A6)