Menu
▾
▴
Re: [Libusb-devel] [ISSUES] OpenUSB on Linux
|
From: Lei C. <Raymond.Chen@Sun.COM> - 2008-07-09 02:32:00
|
鲍奋业(Felix Bao) wrote:
> Hi,
> 3 issues of OpenUSB on Linux:
> (1) Is there any requirement for 'callback' on synchronous transfer?
> Sync xfer API openusb_xfer_wait(...) does not check the field req->cb.
> On Linux, when synchronous transfer completed, usbi_io_complete(...)
> will invoke rep->cb if req->cb != NULL. So if callback on synchronous
> transfer is not permitted, this could be a bug. Two suggestions to fix
> it are given in this mail.
We have two choices here, to allow a callback and to not allow it. Your
solution solves the problem that openusb_xfer_wait doesn't permit a
callback. IMO, we don't care if application sets the callback and we
will not process that callback after transaction completion. That's your
second solution.
>
> (2) Isochronous does not work on Linux. There are mainly 3 problems:
> (a) variables used before allocating (src/linux.c: 932,
> src/linux.c: 993)
> (b) num_urbs counting error (src/linux.c: 949 ~ 959)
> (c) memcpy(...) has wrong target (src/linux.c: 1030)
> Suggested Fix is given in this mail.
What about your test result on your suggested fix?
>
> (3) Bulk transfer reads wrong data on multi-threads. The first
> thread always read the correct data throught transfer. However,
> threads or processes following the first thread read wrong data (first
> 8 bytes) unless the device was reconnected. That is to say, every time
> connecting the device, only the first thread for bulk transfer reads
> the correct data. Root cause to this issue is still unknown.
At what layer you see the data error? If you get packet error at the
OpenUSB API, then the OpenUSB and Linux probably both have relation with
this failure. I think you can go a little further and debug at the
Linux's API. If Linux backend already gets data error from Linux kernel,
that will need further investigation on Linux kernel. The Linux folks
may help you.
Thanks,
Lei Chen
>
>
> Suggested Fix for issue (1)
> Suggestion 1: print a warning to user if req->cb != NULL when calling
> openusb_xfer_wait(...)
> Index: src/api.c
> ===================================================================
> --- src/api.c (revision 144)
> +++ src/api.c (working copy)
> @@ -525,6 +525,11 @@
> return OPENUSB_INVALID_HANDLE;
> }
>
> + if (req->cb) {
> + usbi_debug(NULL, 1, "Invalid request: req->cb != NULL in sync
> xfer");
> + return OPENUSB_BADARG;
> + }
> +
> /* Make sure the request is not too large (if the max size is
> zero then
> * there is no maximum size */
> if (dev->idev->bus->max_xfer_size[req->type] != 0) {
> ===================================================================
>
> Suggestion 2: avoid calling req->cb in usbi_io_complete(...) if the
> request is a synchronous transfer.
> Index: src/io.c
> ===================================================================
> --- src/io.c (revision 144)
> +++ src/io.c (working copy)
> @@ -176,7 +176,7 @@
> pthread_mutex_unlock(&io->lock);
>
> /* run the user supplied callback */
> - if(io->req->cb) { io->req->cb(io->req); }
> + if(io->flag == USBI_ASYNC && io->req->cb) {
> io->req->cb(io->req); }
>
> /* run the internal callback, if it exists */
> if(io->callback) { io->callback(io,status); }
>
>
> Suggested fix for issue (2):
> Index: src/linux.c
> ===================================================================
> --- src/linux.c (revision 144)
> +++ src/linux.c (working copy)
> @@ -928,7 +928,6 @@
> /* intialize */
> this_urb_len = 0;
> packet_offset = 0;
> - io->priv->num_urbs = 0;
>
> /* allocate memory for the private part */
> io->priv = malloc(sizeof(struct usbi_io_private));
> @@ -939,6 +938,7 @@
> return (OPENUSB_NO_RESOURCES);
> }
> memset(io->priv, 0, sizeof(*io->priv));
> + io->priv->num_urbs = 1;
>
> /* get a pointer to our request (for easier access) */
> isoc = io->req->req.isoc;
> @@ -961,14 +961,14 @@
>
> /* allocate memory for our array of urbs */
> io->priv->iso_urbs = (struct usbk_urb**)malloc( io->priv->num_urbs
> -
> * sizeof(struct usbk_urb));
> +
> * sizeof(struct usbk_urb*));
> if(!io->priv->iso_urbs) {
> usbi_debug(hdev->lib_hdl, 1, "unable to allocate memory for
> %d urbs",
> io->priv->num_urbs);
> pthread_mutex_unlock(&io->lock);
> return (OPENUSB_NO_RESOURCES);
> }
> - memset(io->priv->iso_urbs, 0, io->priv->num_urbs * sizeof(struct
> usbk_urb));
> + memset(io->priv->iso_urbs, 0, io->priv->num_urbs * sizeof(struct
> usbk_urb*));
>
> io->priv->urbs_to_cancel = 0;
> io->priv->urbs_to_reap = 0;
> @@ -980,6 +980,7 @@
>
> space_remaining_in_urb = LINUX_MAX_ISOC_XFER;
> urb_packet_offset = 0;
> + this_urb_len = 0;
>
> /* get all of the packets that will fit in this urb */
> while (packet_offset < isoc->pkts.num_packets) {
> @@ -989,7 +990,7 @@
> urb_packet_offset++;
> packet_offset++;
> space_remaining_in_urb -= packet_len;
> - urb->buffer_length += packet_len;
> + this_urb_len += packet_len;
> } else {
> /* it won't fit, put it in the next urb */
> break;
> @@ -1010,6 +1011,7 @@
> io->priv->iso_urbs[i] = urb;
>
> /* allocate memory for the urb buffer */
> + urb->buffer_length = this_urb_len;
> urb->buffer = (void*)malloc(urb->buffer_length);
> if (!urb->buffer) {
> usbi_debug(hdev->lib_hdl, 1, "unable to allocate memory
> for urb buffer "
> @@ -1020,13 +1022,13 @@
> }
> memset(urb->buffer, 0, urb->buffer_length);
> urb_buffer = urb->buffer;
> -
> +
> /* setup the packet lengths and copy in the data */
> for (j=0, k=packet_offset-urb_packet_offset; k<packet_offset;
> k++, j++) {
> packet_len = isoc->pkts.packets[k].length;
> urb->iso_frame_desc[j].length = packet_len;
> if ((io->req->endpoint & USB_REQ_DIR_MASK) ==
> USB_REQ_HOST_TO_DEV) {
> - memcpy(urb->buffer, isoc->pkts.packets[k].payload,
> packet_len);
> + memcpy(urb_buffer, isoc->pkts.packets[k].payload,
> packet_len);
> }
> urb_buffer += packet_len;
> }
> @@ -1448,8 +1450,11 @@
> isoc = io->req->req.isoc;
> isoc_results = isoc->isoc_results;
> for (i = 0; i < urb->number_of_packets; i++) {
> - isoc_results[io->priv->isoc_packet_offset].status =
> + if (urb->iso_frame_desc[i].status) {
> + isoc_results[io->priv->isoc_packet_offset].status =
> translate_errno(-urb->iso_frame_desc[i].status);
> + }
> +
>
> isoc_results[io->priv->isoc_packet_offset].transferred_bytes =
> urb->iso_frame_desc[i].actual_length;
> if ((io->req->endpoint & USB_REQ_DIR_MASK) ==
> USB_REQ_DEV_TO_HOST) {
>
> --
> Best regards!
>
> Felix Bao
> bao...@gm... <mailto:bao...@gm...>
> ------------------------------------------------------------------------
>
> -------------------------------------------------------------------------
> Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
> Studies have shown that voting for your favorite open source project,
> along with a healthy diet, reduces your potential for chronic lameness
> and boredom. Vote Now at http://www.sourceforge.net/community/cca08
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Libusb-devel mailing list
> Lib...@li...
> https://lists.sourceforge.net/lists/listinfo/libusb-devel
>
|