From libusb-devel-@lists.sourceforge.net Thu May 25 14:28:31 2006 From: Chris Frey To: libusb-devel@lists.sourceforge.net Message-ID: <20060525182704.GA23592@foursquare.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.1i Subject: efficient implementation of libusb_wait() Date: Thu, 25 May 2006 14:27:04 -0400 Reply-To: libusb-devel@lists.sourceforge.net Status: RO X-Status: A Hi, I'm looking at the API call to libusb_wait() and wondering how it is possible to implement this efficiently. It's task is fairly large. It is faced with the task of finding all outstanding IO submissions for all open devices, and then waiting on a list of tags that must be unique across all devices. Given that waiting for IO completion is usually done internally with a thread condition wait, and given that pthreads has no API call with which to wait on multiple conditions at once, this will have to be implemented in a while/sleep loop, as far as I can see. Would there be any objections to changing the libusb_wait() prototype into something like this (which more closely matches the previous devel behaviour): int libusb_wait(libusb_dev_handle_t dev, unsigned int tag); This would make the wait straightforward, easy to implement, match the old devel library's behaviour, and allow for tag IDs to be unique only per device. Comments? - Chris From libusb-devel-@lists.sourceforge.net Thu May 25 16:45:07 2006 From: Chris Frey To: libusb-devel@lists.sourceforge.net Message-ID: <20060525204354.GA19069@foursquare.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Subject: [patch] small documentation patch Date: Thu, 25 May 2006 16:43:54 -0400 Reply-To: libusb-devel@lists.sourceforge.net Status: RO Hi, Small documentation patch to clarify the lifetime of the request objects. - Chris From libusb-devel-@lists.sourceforge.net Fri May 26 18:28:12 2006 From: Chris Frey To: libusb-devel@lists.sourceforge.net Message-ID: <20060526222734.GA20646@foursquare.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Subject: [patch] broadcast should be after callback Date: Fri, 26 May 2006 18:27:34 -0400 Reply-To: libusb-devel@lists.sourceforge.net Status: RO Hi, This patch changes the threading broadcast in usbi_io_complete() to occur after the callbacks finish. From the programmer's point of view, if I do a libusb_wait(), I'd like to know that any updating of my internal state resulting from my callback code has completed. i.e. storing the resulting size, etc. The below patch should accomplish that. I've moved the io->inprogress below the callbacks as well. This causes things to appear to be in progress from inside the callback, but I considered this less important than the possibility of libusb_wait() skipping the wait before the callback was finished. libusb_wait() checks to see if io is in progress and skips the pthread_cond_wait() if not. - Chris From libusb-devel-@lists.sourceforge.net Thu Jun 1 18:27:20 2006 Date: Thu, 1 Jun 2006 18:26:55 -0400 From: Chris Frey To: libusb-devel@lists.sourceforge.net Message-ID: <20060601222655.GA12047@foursquare.net> Mime-Version: 1.0 Content-Disposition: inline Subject: [patch] wait, poll, and abort support Reply-To: libusb-devel@lists.sourceforge.net Status: RO Hi, Below is one patch that contains all my previous patches, plus support for libusb_poll() and libusb_abort(). Johannes: If you want me to break these into multiple patches, let me know. This compiles, but I don't have my USB equipment here to test with. That's hopefully tomorrow. In the meantime, I'm hoping someone with more knowledge of the code that me will give me feedback on what I may have done wrong. Locking is likely a weak point. Also, there was no support for tag lookups, so I added that as a device operation, since io is OS-specific. I don't know what Johannes had in mind for that. Consider this pre-alpha code. :-) - Chris Index: src/linux.c =================================================================== --- src/linux.c (revision 640) +++ src/linux.c (working copy) @@ -346,6 +346,54 @@ return 0; } +static struct usbi_io* linux_find_usbi_io(struct usbi_dev_handle *hdev, unsigned int tag) +{ + struct usbi_io *io; + list_for_each_entry(io, &hdev->ios, list) { + unsigned int iotag; + switch( io->type ) + { + case USBI_IO_CONTROL: + iotag = io->ctrl.request->tag; + break; + + case USBI_IO_INTERRUPT: + iotag = io->intr.request->tag; + break; + + case USBI_IO_BULK: + iotag = io->bulk.request->tag; + break; + + case USBI_IO_ISOCHRONOUS: + iotag = io->isoc.request->tag; + break; + + default: + /* make sure it doesn't match */ + iotag = ~tag; + break; + } + + if( iotag == tag ) + return io; + } + return 0; +} + +static int linux_io_wait(struct usbi_dev_handle *hdev, unsigned int tag) +{ + struct usbi_io *io = hdev->idev->ops->find_usbi_io(hdev, tag); + if( !io ) { + pthread_mutex_lock(&io->lock); + if( io->inprogress ) + pthread_cond_wait(&io->cond, &io->lock); + pthread_mutex_unlock(&io->lock); + return LIBUSB_SUCCESS; + } + return LIBUSB_UNKNOWN_TAG; +} + static void *poll_events(void *unused) { char filename[PATH_MAX + 1]; @@ -1019,7 +1067,9 @@ .submit_intr = linux_submit_intr, .submit_bulk = linux_submit_bulk, .submit_isoc = linux_submit_isoc, + .find_usbi_io = linux_find_usbi_io, .io_cancel = linux_io_cancel, + .io_wait = linux_io_wait, }, }; Index: src/usbi.h =================================================================== --- src/usbi.h (revision 640) +++ src/usbi.h (working copy) @@ -256,7 +256,9 @@ int (*submit_intr)(struct usbi_dev_handle *hdev, struct usbi_io *io); int (*submit_bulk)(struct usbi_dev_handle *hdev, struct usbi_io *io); int (*submit_isoc)(struct usbi_dev_handle *hdev, struct usbi_io *io); + struct usbi_io* (*find_usbi_io)(struct usbi_dev_handle *hdev, unsigned int tag); int (*io_cancel)(struct usbi_io *io); + int (*io_wait)(struct usbi_dev_handle *hdev, unsigned int tag); }; struct usbi_backend_ops { Index: src/api.c =================================================================== --- src/api.c (revision 640) +++ src/api.c (working copy) @@ -107,3 +107,50 @@ return hdev->idev->ops->detach_kernel_driver_np(hdev, interface); } +int libusb_abort(libusb_dev_handle_t dev, unsigned int tag) +{ + struct usbi_dev_handle *hdev; + + hdev = usbi_find_dev_handle(dev); + if (!hdev) + return LIBUSB_UNKNOWN_DEVICE; + + struct usbi_io *io = hdev->idev->ops->find_usbi_io(hdev, tag); + if (!hdev) + return LIBUSB_UNKNOWN_TAG; + + return hdev->idev->ops->io_cancel(io); +} + +int libusb_wait(libusb_dev_handle_t dev, unsigned int tag) +{ + struct usbi_dev_handle *hdev; + + hdev = usbi_find_dev_handle(dev); + if (!hdev) + return LIBUSB_UNKNOWN_DEVICE; + + return hdev->idev->ops->io_wait(hdev, tag); +} + +int libusb_poll(libusb_dev_handle_t dev, unsigned int num_tags, + unsigned int *tags, unsigned int *tag) +{ + struct usbi_dev_handle *hdev; + unsigned int i; + + hdev = usbi_find_dev_handle(dev); + if (!hdev) + return LIBUSB_UNKNOWN_DEVICE; + + for( i = 0; i < num_tags; i++ ) { + struct usbi_io *io = hdev->idev->ops->find_usbi_io(hdev, tags[i]); + if( io && !io->inprogress ) { + *tag = tags[i]; + return LIBUSB_SUCCESS; + } + } + + return LIBUSB_FAILURE; +} + Index: src/libusb.h.in =================================================================== --- src/libusb.h.in (revision 640) +++ src/libusb.h.in (working copy) @@ -183,6 +183,7 @@ #define LIBUSB_NOACCESS -9 /* Access to device denied */ #define LIBUSB_PARSE_ERROR -10 /* Data could not be parsed */ #define LIBUSB_UNKNOWN_DEVICE -11 /* Device id is stale or invalid */ +#define LIBUSB_UNKNOWN_TAG -12 /* Tag ID not found */ #define LIBUSB_IO_STALL -50 /* Endpoint stalled */ #define LIBUSB_IO_CRC_ERROR -51 /* CRC error */ @@ -797,6 +798,7 @@ * * Arguments: * usb_ctrl_req - Pointer to USB control request + * request object must exist for the lifetime of IO * residue - transfer residue * callback - callback handler * arg - caller defined @@ -839,6 +841,7 @@ * Arguments: * libusb_intr_req - Pointer to USB intr request * (EP direction determines R/W) + * request object must exist for the lifetime of IO * residue - transfer residue * callback - callback handler * arg - caller defined @@ -880,6 +883,7 @@ * Arguments: * libusb_bulk_req - Pointer to USB bulk request * (EP direction determines R/W) + * request object must exist for the lifetime of IO * residue - transfer residue * callback - callback handler * arg - caller defined @@ -930,6 +934,7 @@ * Arguments: * libusb_isoc_req - Pointer to USB isoc request * (EP direction determines R/W) + * request object must exist for the lifetime of IO * residue - transfer residue * callback - callback handler * arg - caller defined @@ -984,7 +989,7 @@ * * NOT Implemented */ -int libusb_abort(unsigned int tag); +int libusb_abort(libusb_dev_handle_t dev, unsigned int tag); /* * I/O Support: @@ -1006,10 +1011,9 @@ * * NOT Implemented */ -int libusb_wait(unsigned int num_tags, unsigned int *tags, - unsigned int *tag); -int libusb_poll(unsigned int num_tags, unsigned int *tags, - unsigned int *tag); +int libusb_wait(libusb_dev_handle_t dev, unsigned int tag); +int libusb_poll(libusb_dev_handle_t dev, unsigned int num_tags, + unsigned int *tags, unsigned int *tag); #ifdef __cplusplus } Index: src/async.c =================================================================== --- src/async.c (revision 640) +++ src/async.c (working copy) @@ -93,19 +93,6 @@ /* Helper routine. To be called from the various ports */ void usbi_io_complete(struct usbi_io *io, int status, size_t transferred_bytes) { - pthread_mutex_lock(&io->lock); - io->inprogress = 0; - pthread_mutex_unlock(&io->lock); - - /* Add completion for later retrieval */ - pthread_mutex_lock(&completion_lock); - list_add(&io->list, &completions); - pthread_mutex_unlock(&completion_lock); - - pthread_mutex_lock(&io->lock); - pthread_cond_broadcast(&io->cond); - pthread_mutex_unlock(&io->lock); - switch (io->type) { case USBI_IO_CONTROL: io->ctrl.callback(io->ctrl.request, io->ctrl.arg, status, transferred_bytes); @@ -120,6 +107,19 @@ /* FIXME: Implement */ break; } + + pthread_mutex_lock(&io->lock); + io->inprogress = 0; + pthread_mutex_unlock(&io->lock); + + /* Add completion for later retrieval */ + pthread_mutex_lock(&completion_lock); + list_add(&io->list, &completions); + pthread_mutex_unlock(&completion_lock); + + pthread_mutex_lock(&io->lock); + pthread_cond_broadcast(&io->cond); + pthread_mutex_unlock(&io->lock); } /*