IrDA updates 1/4:

	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

drivers/net/irda/irda-usb.c

@@ -30,23 +30,24 @@
  *			    IMPORTANT NOTE
  *			    --------------
  *
- * As of kernel 2.4.10, this is the state of compliance and testing of
+ * As of kernel 2.5.20, 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 usb-ohci	(For other USB controllers)
+ *	o usb-uhci-hcd	(For Intel/Via USB controllers)
+ *	o uhci-hcd	(Alternate/JE driver for Intel/Via 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.
+ * This driver DOESN'T SEEM TO WORK with the following drivers :
+ *	o ohci-hcd	(For other USB controllers)
+ * The first outgoing URB never calls its completion/failure callback.
+ *
+ * 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
  */
@@ -167,7 +168,8 @@ static void irda_usb_build_header(struct irda_usb_cb *self,
 
 		IRDA_DEBUG(2, __FUNCTION__ "(), changing speed to %d\n", 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 +210,8 @@ static void irda_usb_build_header(struct irda_usb_cb *self,
 	if (self->new_xbofs != -1) {
 		IRDA_DEBUG(2, __FUNCTION__ "(), changing xbofs to %d\n", 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:
@@ -286,7 +289,8 @@ static void irda_usb_change_speed_xbofs(struct irda_usb_cb *self)
 
 /*------------------------------------------------------------------*/
 /*
- * 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 *urb)
 {
@@ -295,10 +299,9 @@ static void speed_bulk_callback(struct urb *urb)
 	IRDA_DEBUG(2, __FUNCTION__ "()\n");
 
 	/* We should always have a context */
-	if (self == NULL) {
-		WARNING(__FUNCTION__ "(), Bug : self == NULL\n");
-		return;
-	}
+	ASSERT(self != NULL, return;);
+	/* We should always be called for the speed URB */
+	ASSERT(urb == self->speed_urb, return;);
 
 	/* Check for timeout and other USB nasties */
 	if (urb->status != 0) {
@@ -314,12 +317,14 @@ static void speed_bulk_callback(struct urb *urb)
 	}
 
 	/* urb is now available */
-	urb->status = 0;
+	//urb->status = 0; -> tested above
 
-	/* If it was the speed URB, allow the stack to send more packets */
-	if(urb == 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);
 }
 
 /*------------------------------------------------------------------*/
@@ -334,6 +339,9 @@ static int irda_usb_hard_xmit(struct sk_buff *skb, struct net_device *netdev)
 	s32 speed;
 	s16 xbofs;
 	int res, mtt;
+	int	err = 1;	/* Failed */
+
+	IRDA_DEBUG(4, __FUNCTION__ "() on %s\n", netdev->name);
 
 	netif_stop_queue(netdev);
 
@@ -343,10 +351,9 @@ static int irda_usb_hard_xmit(struct sk_buff *skb, struct net_device *netdev)
 	/* 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, __FUNCTION__ "(), Device is gone...\n");
-		spin_unlock_irqrestore(&self->lock, flags);
-		return 1;	/* Failed */
+		goto drop;
 	}
 
 	/* Check if we need to change the number of xbofs */
@@ -373,6 +380,7 @@ static int irda_usb_hard_xmit(struct sk_buff *skb, struct net_device *netdev)
 			irda_usb_change_speed_xbofs(self);
 			netdev->trans_start = jiffies;
 			/* Will netif_wake_queue() in callback */
+			err = 0;	/* No error */
 			goto drop;
 		}
 	}
@@ -479,7 +487,7 @@ static int irda_usb_hard_xmit(struct sk_buff *skb, struct net_device *netdev)
 	/* Drop silently the skb and exit */
 	dev_kfree_skb(skb);
 	spin_unlock_irqrestore(&self->lock, flags);
-	return 0;
+	return err;		/* Usually 1 */
 }
 
 /*------------------------------------------------------------------*/
@@ -495,10 +503,9 @@ static void write_bulk_callback(struct urb *urb)
 	IRDA_DEBUG(2, __FUNCTION__ "()\n");
 
 	/* We should always have a context */
-	if (self == NULL) {
-		WARNING(__FUNCTION__ "(), Bug : self == NULL\n");
-		return;
-	}
+	ASSERT(self != NULL, return;);
+	/* We should always be called for the speed URB */
+	ASSERT(urb == self->tx_urb, return;);
 
 	/* Free up the skb */
 	dev_kfree_skb_any(skb);
@@ -531,10 +538,21 @@ static void write_bulk_callback(struct urb *urb)
 		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, __FUNCTION__ "(), Changing speed now...\n");
-		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);
@@ -559,11 +577,13 @@ static void irda_usb_net_timeout(struct net_device *netdev)
 	int	done = 0;	/* If we have made any progress */
 
 	IRDA_DEBUG(0, __FUNCTION__ "(), Network layer thinks we timed out!\n");
+	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(__FUNCTION__ "(), device not present!\n");
 		netif_stop_queue(netdev);
 		spin_unlock_irqrestore(&self->lock, flags);
@@ -678,35 +698,7 @@ static void irda_usb_net_timeout(struct net_device *netdev)
 /*------------------------------------------------------------------*/
 /*
  * 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.
- *
- * 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
  */