[PATCH 2.4] USB driver fixes

From: Jean Tourrilhes (jt@bougret.hpl.hp.com)
Date: Mon Jun 16 2003 - 21:07:45 EST


ir241_usb_cleanup-4.diff :
        o [FEATURE] Update various comments to current state
        o [CORRECT] Handle properly failure of URB with new speed
        o [CORRECT] Don't test for (self != NULL) after using it (doh !)
        o [FEATURE] Other minor cleanups
        o [CORRECT] Add ID for new USB device (thanks to Sami Kyostila)
        o [CORRECT] Fix for big endian platforms (thanks to Jacek Jakubowski)

diff -u -p linux/drivers/net/irda/irda-usb.d0.c linux/drivers/net/irda/irda-usb.c
--- linux/drivers/net/irda/irda-usb.d0.c Mon Jun 16 16:48:27 2003
+++ linux/drivers/net/irda/irda-usb.c Mon Jun 16 16:53:54 2003
@@ -30,23 +30,21 @@
  * IMPORTANT NOTE
  * --------------
  *
- * As of kernel 2.4.10, this is the state of compliance and testing of
+ * As of kernel 2.4.21, this is the state of compliance and testing of
  * this driver (irda-usb) with regards to the USB low level drivers...
  *
  * This driver has been tested SUCCESSFULLY with the following drivers :
  * o usb-uhci (For Intel/Via USB controllers)
+ * o uhci (Alternate/JE driver for Intel/Via USB controllers)
  * o usb-ohci (For other USB controllers)
  *
  * This driver has NOT been tested with the following drivers :
- * o usb-ehci (USB 2.0 controllers)
+ * o ehci-hcd (USB 2.0 controllers)
  *
- * This driver WON'T WORK with the following drivers :
- * o uhci (Alternate/JE driver for Intel/Via USB controllers)
- * Amongst the reasons :
- * o uhci doesn't implement USB_ZERO_PACKET
- * o uhci non-compliant use of urb->timeout
- * The final fix for USB_ZERO_PACKET in uhci is likely to be in 2.4.19 and
- * 2.5.8. With this fix, the driver will work properly. More on that later.
+ * Note that all HCD drivers do USB_ZERO_PACKET and timeout properly,
+ * so we don't have to worry about that anymore.
+ * One common problem is the failure to set the address on the dongle,
+ * but this happens before the driver gets loaded...
  *
  * Jean II
  */
@@ -81,8 +79,10 @@ static struct irda_usb_cb irda_instance[
 
 /* These are the currently known IrDA USB dongles. Add new dongles here */
 static struct usb_device_id dongles[] = {
- /* ACTiSYS Corp, ACT-IR2000U FIR-USB Adapter */
+ /* ACTiSYS Corp., ACT-IR2000U FIR-USB Adapter */
         { USB_DEVICE(0x9c4, 0x011), driver_info: IUC_SPEED_BUG | IUC_NO_WINDOW },
+ /* Look like ACTiSYS, Report : IBM Corp., IBM UltraPort IrDA */
+ { USB_DEVICE(0x4428, 0x012), driver_info: IUC_SPEED_BUG | IUC_NO_WINDOW },
         /* KC Technology Inc., KC-180 USB IrDA Device */
         { USB_DEVICE(0x50f, 0x180), driver_info: IUC_SPEED_BUG | IUC_NO_WINDOW },
         /* Extended Systems, Inc., XTNDAccess IrDA USB (ESI-9685) */
@@ -167,7 +167,8 @@ static void irda_usb_build_header(struct
 
                 IRDA_DEBUG(2, "%s(), changing speed to %d\n", __FUNCTION__, self->new_speed);
                 self->speed = self->new_speed;
- self->new_speed = -1;
+ /* We will do ` self->new_speed = -1; ' in the completion
+ * handler just in case the current URB fail - Jean II */
 
                 switch (self->speed) {
                 case 2400:
@@ -208,7 +209,8 @@ static void irda_usb_build_header(struct
         if (self->new_xbofs != -1) {
                 IRDA_DEBUG(2, "%s(), changing xbofs to %d\n", __FUNCTION__, self->new_xbofs);
                 self->xbofs = self->new_xbofs;
- self->new_xbofs = -1;
+ /* We will do ` self->new_xbofs = -1; ' in the completion
+ * handler just in case the current URB fail - Jean II */
 
                 switch (self->xbofs) {
                 case 48:
@@ -285,7 +287,8 @@ static void irda_usb_change_speed_xbofs(
 
 /*------------------------------------------------------------------*/
 /*
- * Note : this function will be called with both speed_urb and empty_urb...
+ * Speed URB callback
+ * Now, we can only get called for the speed URB.
  */
 static void speed_bulk_callback(struct urb *purb)
 {
@@ -294,10 +297,9 @@ static void speed_bulk_callback(struct u
         IRDA_DEBUG(2, "%s()\n", __FUNCTION__);
 
         /* We should always have a context */
- if (self == NULL) {
- WARNING("%s(), Bug : self == NULL\n", __FUNCTION__);
- return;
- }
+ ASSERT(self != NULL, return;);
+ /* We should always be called for the speed URB */
+ ASSERT(purb == &self->speed_urb, return;);
 
         /* Check for timeout and other USB nasties */
         if(purb->status != USB_ST_NOERROR) {
@@ -313,12 +315,14 @@ static void speed_bulk_callback(struct u
         }
 
         /* urb is now available */
- purb->status = USB_ST_NOERROR;
+ //purb->status = USB_ST_NOERROR; -> tested above
 
- /* If it was the speed URB, allow the stack to send more packets */
- if(purb == &self->speed_urb) {
- netif_wake_queue(self->netdev);
- }
+ /* New speed and xbof is now commited in hardware */
+ self->new_speed = -1;
+ self->new_xbofs = -1;
+
+ /* Allow the stack to send more packets */
+ netif_wake_queue(self->netdev);
 }
 
 /*------------------------------------------------------------------*/
@@ -333,6 +337,9 @@ static int irda_usb_hard_xmit(struct sk_
         s32 speed;
         s16 xbofs;
         int res, mtt;
+ int err = 1; /* Failed */
+
+ IRDA_DEBUG(4, __FUNCTION__ "() on %s\n", netdev->name);
 
         netif_stop_queue(netdev);
 
@@ -342,10 +349,9 @@ static int irda_usb_hard_xmit(struct sk_
         /* Check if the device is still there.
          * We need to check self->present under the spinlock because
          * of irda_usb_disconnect() is synchronous - Jean II */
- if ((!self) || (!self->present)) {
+ if (!self->present) {
                 IRDA_DEBUG(0, "%s(), Device is gone...\n", __FUNCTION__);
- spin_unlock_irqrestore(&self->lock, flags);
- return 1; /* Failed */
+ goto drop;
         }
 
         /* Check if we need to change the number of xbofs */
@@ -372,6 +378,7 @@ static int irda_usb_hard_xmit(struct sk_
                         irda_usb_change_speed_xbofs(self);
                         netdev->trans_start = jiffies;
                         /* Will netif_wake_queue() in callback */
+ err = 0; /* No error */
                         goto drop;
                 }
         }
@@ -386,7 +393,7 @@ static int irda_usb_hard_xmit(struct sk_
          * Also, we don't use directly skb_cow(), because it require
          * headroom >= 16, which force unnecessary copies - Jean II */
         if (skb_headroom(skb) < USB_IRDA_HEADER) {
- IRDA_DEBUG(0, "%s(), Insuficient skb headroom.\n", __FUNCTION__);
+ IRDA_DEBUG(1, "%s(), Insuficient skb headroom.\n", __FUNCTION__);
                 if (skb_cow(skb, USB_IRDA_HEADER)) {
                         WARNING("%s(), failed skb_cow() !!!\n", __FUNCTION__);
                         goto drop;
@@ -478,7 +485,7 @@ drop:
         /* Drop silently the skb and exit */
         dev_kfree_skb(skb);
         spin_unlock_irqrestore(&self->lock, flags);
- return 0;
+ return err; /* Usually 1 */
 }
 
 /*------------------------------------------------------------------*/
@@ -494,10 +501,9 @@ static void write_bulk_callback(struct u
         IRDA_DEBUG(2, "%s()\n", __FUNCTION__);
 
         /* We should always have a context */
- if (self == NULL) {
- WARNING("%s(), Bug : self == NULL\n", __FUNCTION__);
- return;
- }
+ ASSERT(self != NULL, return;);
+ /* We should always be called for the speed URB */
+ ASSERT(purb == &self->tx_urb, return;);
 
         /* Free up the skb */
         dev_kfree_skb_any(skb);
@@ -530,10 +536,21 @@ static void write_bulk_callback(struct u
                 return;
         }
 
- /* If we need to change the speed or xbofs, do it now */
+ /* If changes to speed or xbofs is pending... */
         if ((self->new_speed != -1) || (self->new_xbofs != -1)) {
- IRDA_DEBUG(1, "%s(), Changing speed now...\n", __FUNCTION__);
- irda_usb_change_speed_xbofs(self);
+ if ((self->new_speed != self->speed) ||
+ (self->new_xbofs != self->xbofs)) {
+ /* We haven't changed speed yet (because of
+ * IUC_SPEED_BUG), so do it now - Jean II */
+ IRDA_DEBUG(1, __FUNCTION__ "(), Changing speed now...\n");
+ irda_usb_change_speed_xbofs(self);
+ } else {
+ /* New speed and xbof is now commited in hardware */
+ self->new_speed = -1;
+ self->new_xbofs = -1;
+ /* Done, waiting for next packet */
+ netif_wake_queue(self->netdev);
+ }
         } else {
                 /* Otherwise, allow the stack to send more packets */
                 netif_wake_queue(self->netdev);
@@ -558,11 +575,13 @@ static void irda_usb_net_timeout(struct
         int done = 0; /* If we have made any progress */
 
         IRDA_DEBUG(0, "%s(), Network layer thinks we timed out!\n", __FUNCTION__);
+ ASSERT(self != NULL, return;);
 
         /* Protect us from USB callbacks, net Tx and else. */
         spin_lock_irqsave(&self->lock, flags);
 
- if ((!self) || (!self->present)) {
+ /* self->present *MUST* be read under spinlock */
+ if (!self->present) {
                 WARNING("%s(), device not present!\n", __FUNCTION__);
                 netif_stop_queue(netdev);
                 spin_unlock_irqrestore(&self->lock, flags);
@@ -677,36 +696,7 @@ static void irda_usb_net_timeout(struct
 /*------------------------------------------------------------------*/
 /*
  * Submit a Rx URB to the USB layer to handle reception of a frame
- *
- * Important note :
- * The function process_urb() in usb-uhci.c contains the following code :
- * > urb->complete ((struct urb *) urb);
- * > // Re-submit the URB if ring-linked
- * > if (is_ring && (urb->status != -ENOENT) && !contains_killed) {
- * > urb->dev=usb_dev;
- * > uhci_submit_urb (urb);
- * > }
- * The way I see it is that if we submit more than one Rx URB at a
- * time, the Rx URB can be automatically re-submitted after the
- * completion handler is called.
- * We make sure to disable this feature by setting urb->next to NULL
- *
- * My take is that it's a questionable feature, and quite difficult
- * to control and to make work effectively.
- * The outcome (re-submited or not) depend on various complex
- * test ('is_ring' and 'contains_killed'), and the completion handler
- * don't have this information, so basically the driver has no way
- * to know if URB are resubmitted or not. Yuck !
- * If everything is perfect, it's cool, but the problem is when
- * an URB is killed (timeout, call to unlink_urb(), ...), things get
- * messy...
- * The other problem is that this scheme deal only with the URB
- * and ignore everything about the associated buffer. So, it would
- * resubmit URB even if the buffer is still in use or non-existent.
- * On the other hand, submitting ourself in the completion callback
- * is quite trivial and work well (this function).
- * Moreover, this scheme doesn't allow to have an idle URB, which is
- * necessary to overcome some URB failures.
+ * Mostly called by the completion callback of the previous URB.
  *
  * Jean II
  */
@@ -1127,7 +1117,10 @@ static inline void irda_usb_init_qos(str
         /* Initialize QoS for this device */
         irda_init_max_qos_capabilies(&self->qos);
 
- self->qos.baud_rate.bits = desc->wBaudRate;
+ /* See spec section 7.2 for meaning.
+ * Values are little endian (as most USB stuff), the IrDA stack
+ * use it in native order (see parameters.c). - Jean II */
+ self->qos.baud_rate.bits = le16_to_cpu(desc->wBaudRate);
         self->qos.min_turn_time.bits = desc->bmMinTurnaroundTime;
         self->qos.additional_bofs.bits = desc->bmAdditionalBOFs;
         self->qos.window_size.bits = desc->bmWindowSize;
@@ -1138,7 +1131,7 @@ static inline void irda_usb_init_qos(str
 
         /* Don't always trust what the dongle tell us */
         if(self->capability & IUC_SIR_ONLY)
- self->qos.baud_rate.bits &= 0xff;
+ self->qos.baud_rate.bits &= 0x00ff;
         if(self->capability & IUC_SMALL_PKT)
                 self->qos.data_size.bits = 0x07;
         if(self->capability & IUC_NO_WINDOW)
@@ -1338,13 +1331,14 @@ static inline int irda_usb_parse_endpoin
  */
 static inline void irda_usb_dump_class_desc(struct irda_class_desc *desc)
 {
+ /* Values are little endian */
         printk("bLength=%x\n", desc->bLength);
         printk("bDescriptorType=%x\n", desc->bDescriptorType);
- printk("bcdSpecRevision=%x\n", desc->bcdSpecRevision);
+ printk("bcdSpecRevision=%x\n", le16_to_cpu(desc->bcdSpecRevision));
         printk("bmDataSize=%x\n", desc->bmDataSize);
         printk("bmWindowSize=%x\n", desc->bmWindowSize);
         printk("bmMinTurnaroundTime=%d\n", desc->bmMinTurnaroundTime);
- printk("wBaudRate=%x\n", desc->wBaudRate);
+ printk("wBaudRate=%x\n", le16_to_cpu(desc->wBaudRate));
         printk("bmAdditionalBOFs=%x\n", desc->bmAdditionalBOFs);
         printk("bIrdaRateSniff=%x\n", desc->bIrdaRateSniff);
         printk("bMaxUnicastList=%x\n", desc->bMaxUnicastList);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Mon Jun 23 2003 - 22:00:20 EST