Endianness of control setup packets

Document behaviour where host-endian and bus-endian (little) should be
used respectively.

Also remove packed attribute from libusb_control_setup as all fields
are naturally aligned.

6 files changed, 53 insertions, 41 deletions

diff --git a/TODO b/TODO
index df24828..d5ab973 100644
--- a/TODO
+++ b/TODO
@@ -1,8 +1,6 @@
 for 1.0
 =======
-fixme review
 review functionality missing over 0.1
-endianness of control setup, issues when resubmitting transfers
 serialization of handle_events
 internal docs for OS porters
 
diff --git a/libusb/core.c b/libusb/core.c
index 175158d..0f71133 100644
--- a/libusb/core.c
+++ b/libusb/core.c
@@ -216,11 +216,7 @@ libusb_free_device_list(list, 1);
  * libusb_close().
  */
 
-/**
- * @defgroup misc Miscellaneous structures and constants
- * This page documents structures and constants that don't belong anywhere
- * else
- */
+/** @defgroup misc Miscellaneous */
 
 /* we traverse usbfs without knowing how many devices we are going to find.
  * so we create this discovered_devs model which is similar to a linked-list
diff --git a/libusb/io.c b/libusb/io.c
index 04ffc6e..133f849 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -315,6 +315,12 @@ if (r == 0 && actual_length == sizeof(data)) {
  *     packet.
  * -# Submit the transfer.
  *
+ * The multi-byte control setup fields (wValue, wIndex and wLength) must
+ * be given in little-endian byte order (the endianness of the USB bus).
+ * Endianness conversion is transparently handled by
+ * libusb_fill_control_setup() which is documented to accept host-endian
+ * values.
+ *
  * Further considerations are needed when handling transfer completion in
  * your callback function:
  * - As you might expect, the setup packet will still be sitting at the start
@@ -722,19 +728,6 @@ API_EXPORTED int libusb_submit_transfer(struct libusb_transfer *transfer)
 	if (r < 0)
 		return LIBUSB_ERROR_OTHER;
 
-	if (transfer->type == LIBUSB_TRANSFER_TYPE_CONTROL) {
-		struct libusb_control_setup *setup =
-			(struct libusb_control_setup *) transfer->buffer;
-	
-		usbi_dbg("RQT=%02x RQ=%02x VAL=%04x IDX=%04x length=%d",
-			setup->bmRequestType, setup->bRequest, setup->wValue, setup->wIndex,
-			setup->wLength);
-
-		setup->wValue = cpu_to_le16(setup->wValue);
-		setup->wIndex = cpu_to_le16(setup->wIndex);
-		setup->wLength = cpu_to_le16(setup->wLength);
-	}
-	
 	add_to_flying_list(itransfer);
 	r = usbi_backend->submit_transfer(itransfer);
 	if (r) {
diff --git a/libusb/libusb.h b/libusb/libusb.h
index a24fce3..4617f6a 100644
--- a/libusb/libusb.h
+++ b/libusb/libusb.h
@@ -21,11 +21,41 @@
 #ifndef __LIBUSB_H__
 #define __LIBUSB_H__
 
+#include <endian.h>
 #include <stdint.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <time.h>
 
+#define bswap16(x) (((x & 0xff) << 8) | (x >> 8))
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+#define libusb_cpu_to_le16(x) (x)
+#define libusb_le16_to_cpu(x) (x)
+#elif __BYTE_ORDER == __BIG_ENDIAN
+#define libusb_le16_to_cpu(x) bswap16(x)
+#define libusb_cpu_to_le16(x) bswap16(x)
+#else
+#error "Unrecognized endianness"
+#endif
+
+/** \def libusb_cpu_to_le16
+ * \ingroup misc
+ * Convert a 16-bit value from host-endian to little-endian format. On
+ * little endian systems, this function does nothing. On big endian systems,
+ * the bytes are swapped.
+ * \param x the host-endian value to convert
+ * \returns the value in little-endian byte order
+ */
+
+/** \def libusb_le16_to_cpu
+ * \ingroup misc
+ * Convert a 16-bit value from little-endian to host-endian format. On
+ * little endian systems, this function does nothing. On big endian systems,
+ * the bytes are swapped.
+ * \param x the little-endian value to convert
+ * \returns the value in host-endian byte order
+ */
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -497,7 +527,7 @@ struct libusb_control_setup {
 
 	/** Number of bytes to transfer */
 	uint16_t wLength;
-} __attribute__((packed));
+};
 
 #define LIBUSB_CONTROL_SETUP_SIZE (sizeof(struct libusb_control_setup))
 
@@ -737,7 +767,8 @@ static inline struct libusb_control_setup *libusb_control_transfer_get_setup(
 
 /** \ingroup asyncio
  * Helper function to populate the setup packet (first 8 bytes of the data
- * buffer) for a control transfer.
+ * buffer) for a control transfer. The wIndex, wValue and wLength values should
+ * be given in host-endian byte order.
  * 
  * \param buffer buffer to output the setup packet into
  * \param bmRequestType see the
@@ -763,9 +794,9 @@ static inline void libusb_fill_control_setup(unsigned char *buffer,
 	struct libusb_control_setup *setup = (struct libusb_control_setup *) buffer;
 	setup->bmRequestType = bmRequestType;
 	setup->bRequest = bRequest;
-	setup->wValue = wValue;
-	setup->wIndex = wIndex;
-	setup->wLength = wLength;
+	setup->wValue = libusb_cpu_to_le16(wValue);
+	setup->wIndex = libusb_cpu_to_le16(wIndex);
+	setup->wLength = libusb_cpu_to_le16(wLength);
 }
 
 struct libusb_transfer *libusb_alloc_transfer(int iso_packets);
@@ -790,7 +821,7 @@ void libusb_free_transfer(struct libusb_transfer *transfer);
  *
  * It is also legal to pass a NULL buffer to this function, in which case this
  * function will not attempt to populate the length field. Remember that you
- * must populate the buffer and length fields later.
+ * must then populate the buffer and length fields later.
  *
  * \param transfer the transfer to populate
  * \param dev_handle handle of the device that will handle the transfer
@@ -812,7 +843,8 @@ static inline void libusb_fill_control_transfer(
 	transfer->timeout = timeout;
 	transfer->buffer = buffer;
 	if (setup)
-		transfer->length = LIBUSB_CONTROL_SETUP_SIZE + setup->wLength;
+		transfer->length = LIBUSB_CONTROL_SETUP_SIZE
+			+ libusb_le16_to_cpu(setup->wLength);
 	transfer->user_data = user_data;
 	transfer->callback = callback;
 }
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
index a59a5e6..414da25 100644
--- a/libusb/libusbi.h
+++ b/libusb/libusbi.h
@@ -23,7 +23,6 @@
 
 #include <config.h>
 
-#include <endian.h>
 #include <pthread.h>
 #include <stddef.h>
 #include <sys/select.h>
@@ -101,17 +100,6 @@ static inline void list_del(struct list_head *entry)
         const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
         (type *)( (char *)__mptr - offsetof(type,member) );})
 
-#define bswap16(x) (((x & 0xff) << 8) | (x >> 8))
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-#define cpu_to_le16(x) (x)
-#define le16_to_cpu(x) (x)
-#elif __BYTE_ORDER == __BIG_ENDIAN
-#define le16_to_cpu(x) bswap16(x)
-#define cpu_to_le16(x) bswap16(x)
-#else
-#error "Unrecognized endianness"
-#endif
-
 #define MIN(a, b)	((a) < (b) ? (a) : (b))
 #define MAX(a, b)	((a) > (b) ? (a) : (b))
 
diff --git a/libusb/sync.c b/libusb/sync.c
index c2790fb..1d0f7c4 100644
--- a/libusb/sync.c
+++ b/libusb/sync.c
@@ -42,8 +42,13 @@ static void ctrl_transfer_cb(struct libusb_transfer *transfer)
 }
 
 /** \ingroup syncio
- * Perform a USB control transfer. The direction of the transfer is inferred
- * from the bmRequestType field of the setup packet.
+ * Perform a USB control transfer.
+ *
+ * The direction of the transfer is inferred from the bmRequestType field of
+ * the setup packet.
+ *
+ * The wValue, wIndex and wLength fields values should be given in host-endian
+ * byte order.
  *
  * \param dev_handle a handle for the device to communicate with
  * \param bmRequestType the request type field for the setup packet