USB driver fixes

USB driver fixes

Post by Jean Tourrilhe » Wed, 25 Jun 2003 01:30:18



        Hi Marcelo,

        Various fixes for the irda-usb driver.
        Please apply ;-)

        Jean

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/