Hi,
I've been working late on the irda-usb driver. Lot's of errors
and timeout from the USB layer were not handled properly. As I have a
platform with is weird, those errors were showing all over the place.
On my PC were it was working, it is still working properly, so
I guess I didn't broke anything. On my other platform, it now crash
somewhere else (argh !!!).
Enjoy...
Jean
diff -u -p linux/drivers/net/irda/irda-usb.j1.h linux/drivers/net/irda/irda-usb.h
--- linux/drivers/net/irda/irda-usb.j1.h Mon Feb 26 17:36:33 2001
+++ linux/drivers/net/irda/irda-usb.h Thu Mar 1 17:07:44 2001
@@ -32,9 +32,9 @@
#define RX_COPY_THRESHOLD 200
#define IRDA_USB_MAX_MTU 2050
-#define IRDA_USB_SPEED_MTU 64 /* Weird, but work like this */
+#define IRDA_USB_SPEED_MTU 128 /* Weird, but work like this */
-/* Maximum number of URB on the Rx and Tx path
+/* Maximum number of active URB on the Rx path
* This is the amount of buffers the we keep between the USB harware and the
* IrDA stack.
* Note : the network layer does also queue the packets between us and the
@@ -47,7 +47,21 @@
* On the other hand, increasing the number of URB will have penalities
* in term of latency and will interact with the link management in IrLAP...
* Jean II */
-#define IU_MAX_URB 1 /* Don't touch !!! */
+#define IU_MAX_ACTIVE_URB 1 /* Don't touch !!! */
+/* When a Rx URB is passed back to us, we can't reuse it immediately,
+ * because it may still be referenced by the USB layer. Therefore we
+ * need to keep one extra URB in the Rx path.
+ * Jean II */
+#define IU_MAX_URB (IU_MAX_ACTIVE_URB + 1)
+
+/* Various ugly stuff to try to workaround problems */
+/* The USB layer should send empty frames at the end of packet. As it
+ * doesn't do it, we need to do it ourselves... */
+#define IU_BUG_KICK_TX
+/* Send empty frame before setting the speed, same reason */
+//#define IU_BUG_KICK_SPEED -> Make things worse
+/* Send speed command in case of timeout, same reason */
+#define IU_BUG_KICK_TIMEOUT
/* Inbound header */
#define MEDIA_BUSY 0x80
@@ -103,8 +117,10 @@ struct irda_usb_cb {
wait_queue_head_t wait_q; /* for timeouts */
struct urb rx_urb[IU_MAX_URB]; /* URBs used to receive data frames */
+ struct urb *idle_rx_urb; /* Pointer to idle URB in Rx path */
struct urb tx_urb; /* URB used to send data frames */
struct urb speed_urb; /* URB used to send speed commands */
+ struct urb empty_urb; /* URB used to send empty commands */
struct net_device *netdev; /* Yes! we are some kind of netdev. */
struct net_device_stats stats;
@@ -113,6 +129,7 @@ struct irda_usb_cb {
iobuff_t tx_buff; /* Transmit buffer */
iobuff_t rx_buff; /* Receive buffer */
iobuff_t speed_buff; /* Buffer for speed changes */
+ iobuff_t empty_buff; /* Buffer for empty commands */
struct timeval stamp;
struct timeval now;
diff -u -p linux/drivers/net/irda/irda-usb.j1.c linux/drivers/net/irda/irda-usb.c
--- linux/drivers/net/irda/irda-usb.j1.c Mon Feb 26 17:33:59 2001
+++ linux/drivers/net/irda/irda-usb.c Thu Mar 1 17:09:40 2001
@@ -424,6 +424,14 @@ static int irda_usb_open(struct irda_usb
return -1;
memset(self->speed_buff.head, 0, self->speed_buff.truesize);
self->speed_buff.data = self->speed_buff.head;
+ /* Same for empty buff */
+ self->empty_buff.truesize = IRDA_USB_SPEED_MTU;
+ self->empty_buff.head = (__u8 *) kmalloc(self->empty_buff.truesize,
+ GFP_KERNEL);
+ if (self->empty_buff.head == NULL)
+ return -1;
+ memset(self->empty_buff.head, 0, self->empty_buff.truesize);
+ self->empty_buff.data = self->empty_buff.head;
/* Create a network device for us */
if (!(netdev = dev_alloc("irda%d", &err))) {
@@ -476,6 +484,10 @@ static int irda_usb_close(struct irda_us
kfree(self->speed_buff.head);
self->speed_buff.head = NULL;
}
+ if (self->empty_buff.head) {
+ kfree(self->empty_buff.head);
+ self->empty_buff.head = NULL;
+ }
return 0;
}
@@ -490,6 +502,41 @@ static int irda_usb_send_cmd(void)
#endif
/*
+ * Send an empty URB to the dongle
+ * The goal there is to try to resynchronise with the dongle. An empty
+ * frame signify the end of a Tx frame. Jean II
+ */
+static inline void irda_usb_send_empty(struct irda_usb_cb *self)
+{
+ purb_t purb;
+ int ret;
+
+ IRDA_DEBUG(0, __FUNCTION__ "()\n");
+
+ /* Grab the empty URB */
+ purb = &self->empty_urb;
+ if (purb->status != USB_ST_NOERROR) {
+ WARNING(__FUNCTION__ "(), Empty URB still in use!\n");
+ return;
+ }
+
+ /* Submit the Empty URB */
+ FILL_BULK_URB(purb, self->usbdev,
+ usb_sndbulkpipe(self->usbdev, self->bulk_out_ep),
+ self->empty_buff.head, IRDA_USB_SPEED_MTU,
+ write_bulk_callback, self);
+ purb->transfer_buffer_length = 0;
+ //purb->transfer_flags |= USB_ASYNC_UNLINK;
+ //purb->transfer_flags |= USB_QUEUE_BULK;
+ purb->transfer_flags = USB_QUEUE_BULK;
+ purb->timeout = MSECS_TO_JIFFIES(100);
+
+ if ((ret = usb_submit_urb(purb))) {
+ IRDA_DEBUG(0, __FUNCTION__ "(), failed Empty URB\n");
+ }
+}
+
+/*
* Function irda_usb_build_header(self, skb, header)
*
* Builds USB-IrDA outbound header
@@ -607,6 +654,16 @@ static void irda_usb_change_speed_xbofs(
IRDA_DEBUG(0, __FUNCTION__ "(), speed=%d, xbofs=%d\n",
self->new_speed, self->new_xbofs);
+#ifdef IU_BUG_KICK_SPEED
+ /* Before sending the speed command, make sure that the dongle
+ * is ready by sending an empty frame - Jean II */
+ IRDA_DEBUG(3, __FUNCTION__ "(), Kick Speed...\n");
+ spin_lock_irqsave(&self->lock, flags);
+ irda_usb_send_empty(self);
+ spin_unlock_irqrestore(&self->lock, flags);
+#endif /* IU_BUG_KICK_SPEED */
+
+ /* Grab the speed URB */
purb = &self->speed_urb;
if (purb->status != USB_ST_NOERROR) {
WARNING(__FUNCTION__ "(), URB still in use!\n");
@@ -624,11 +681,12 @@ static void irda_usb_change_speed_xbofs(
/* Submit the 0 length IrDA frame to trigger new speed settings */
FILL_BULK_URB(purb, self->usbdev,
usb_sndbulkpipe(self->usbdev, self->bulk_out_ep),
- frame, IRDA_USB_MAX_MTU,
+ frame, IRDA_USB_SPEED_MTU,
write_bulk_callback, self);
purb->transfer_buffer_length = USB_IRDA_HEADER;
//purb->transfer_flags |= USB_ASYNC_UNLINK;
- purb->transfer_flags |= USB_QUEUE_BULK;
+ //purb->transfer_flags |= USB_QUEUE_BULK;
+ purb->transfer_flags = USB_QUEUE_BULK;
purb->timeout = MSECS_TO_JIFFIES(100);
if ((ret = usb_submit_urb(purb))) {
@@ -723,7 +781,8 @@ static int irda_usb_hard_xmit(struct sk_
self->tx_buff.head, IRDA_USB_MAX_MTU,
write_bulk_callback, self);
purb->transfer_buffer_length = self->tx_buff.len;
- purb->transfer_flags |= USB_QUEUE_BULK;
+ //purb->transfer_flags |= USB_QUEUE_BULK;
+ purb->transfer_flags = USB_QUEUE_BULK;
purb->timeout = MSECS_TO_JIFFIES(100);
if ((res = usb_submit_urb(purb))) {
@@ -731,25 +790,20 @@ static int irda_usb_hard_xmit(struct sk_
self->stats.tx_errors++;
netif_start_queue(netdev);
} else {
+#ifdef IU_BUG_KICK_TX
/* Kick Tx?
* If the packet is a multiple of 64, the USB layer
* should send an empty frame (a short packet) to signal
* the end of frame (that's part of the USB spec).
* In theory, we would need to do that only if USB_DISABLE_SPD
* is not enabled, and with the current setting the USB layer
- * should do it for us.
- * So why do we need this workaround ? Mystery... */
+ * should do it for us. However, the USB layer doesn't handle
+ * USB_DISABLE_SPD properly, so we need this workaround... */
if ((self->tx_buff.len % 64) == 0) {
- /* Borrow speed urb to send empty frame */
- purb = &self->speed_urb;
- FILL_BULK_URB(purb, self->usbdev,
- usb_sndbulkpipe(self->usbdev, self->bulk_out_ep),
- self->tx_buff.head, IRDA_USB_MAX_MTU,
- write_bulk_callback, self);
- purb->transfer_buffer_length = 0;
- purb->transfer_flags |= USB_QUEUE_BULK;
- res = usb_submit_urb(purb);
+ IRDA_DEBUG(3, __FUNCTION__ "(), Kick Tx...\n");
+ irda_usb_send_empty(self);
}
+#endif /* IU_BUG_KICK_TX */
self->stats.tx_packets++;
self->stats.tx_bytes += skb->len;
@@ -776,6 +830,19 @@ static void write_bulk_callback(purb_t p
return;
}
+ /* Check for timeout and other USB nasties */
+ if(purb->status != USB_ST_NOERROR) {
+ /* I get a lot of -ECONNABORTED = -103 here - Jean II */
+ WARNING(__FUNCTION__ "(), URB complete status %d, transfer_flags
+0x%04X\n", purb->status, purb->transfer_flags);
+
+ /* Don't do anything here, that might confuse the USB layer.
+ * Instead, we will wait for irda_usb_net_timeout(), the
+ * network layer watchdog, to fix the situation.
+ * Jean II */
+ /* A reset of the dongle might be welcomed here - Jean II */
+ return;
+ }
+
/* urb is now available */
purb->status = USB_ST_NOERROR;
@@ -785,6 +852,12 @@ static void write_bulk_callback(purb_t p
return;
}
+ /* If it was the empty URB, don't do anything... */
+ if(purb == &self->empty_urb) {
+ IRDA_DEBUG(3, __FUNCTION__ "(), Empty URB completed...\n");
+ return;
+ }
+
/* If we need to change the speed or xbofs, do it now */
if ((self->new_speed != -1) || (self->new_xbofs != -1)) {
IRDA_DEBUG(0, __FUNCTION__ "(), Changing speed now...\n");
@@ -795,6 +868,9 @@ static void write_bulk_callback(purb_t p
}
}
+/*
+ * Submit a Rx URB to the USB layer to handle reception of a frame
+ */
static void irda_usb_submit(struct irda_usb_cb *self, struct sk_buff *skb, purb_t
purb)
{
struct irda_skb_cb *cb;
@@ -834,7 +910,8 @@ static void irda_usb_submit(struct irda_
usb_rcvbulkpipe(self->usbdev, self->bulk_in_ep),
skb->data, skb->truesize,
irda_usb_receive, skb);
- purb->transfer_flags |= USB_QUEUE_BULK;
+ //purb->transfer_flags |= USB_QUEUE_BULK;
+ purb->transfer_flags = USB_QUEUE_BULK;
purb->status = USB_ST_NOERROR;
ret = usb_submit_urb(purb);
@@ -843,6 +920,42 @@ static void irda_usb_submit(struct irda_
* Basically, the Rx path will stop... */
IRDA_DEBUG(0, __FUNCTION__ "(), Failed to submit Rx URB %d\n", ret);
}
+
+ /* Important note :
+ * The function process_urb() 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 are automatically re-submitted after the
+ * completion handler is called.
+ * What is weird is that most driver also re-submit the Rx URB in
+ * the completion handler. No comments...
+ *
+ * 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 (see above).
+ *
+ * Moreover, this scheme will interfer with the idle_rx_urb stuff
+ * down there if we submit more than one URB (so, don't do that).
+ * I wish it was possible to disable this feature...
+ *
+ * Jean II
+ */
}
/*
@@ -881,8 +994,14 @@ static void irda_usb_receive(purb_t purb
self->stats.rx_errors++;
self->stats.rx_crc_errors++;
break;
+ case -ECONNRESET: /* -104 */
+ WARNING(__FUNCTION__ "(), Connection Reset !!!\n");
+ /* uhci_cleanup_unlink() is going to kill the Rx
+ * URB just after we return. No problem, at this
+ * point the URB will be idle ;-) - Jean II */
+ break;
default:
- WARNING(__FUNCTION__ "(), RX status %d\n", purb->status);
+ WARNING(__FUNCTION__ "(), RX status %d,transfer_flags 0x%04X
+\n", purb->status, purb->transfer_flags);
break;
}
goto done;
@@ -917,6 +1036,7 @@ static void irda_usb_receive(purb_t purb
/* Copy packet, so we can recycle the original */
memcpy(skb_put(new, skb->len), skb->data, skb->len);
+ /* We will cleanup the skb in irda_usb_submit() */
} else {
/* Deliver the original skb */
new = skb;
@@ -931,9 +1051,25 @@ static void irda_usb_receive(purb_t purb
new->mac.raw = new->data;
new->protocol = htons(ETH_P_IRDA);
netif_rx(new);
+
done:
- /* Recycle Rx URB (and possible the skb as well) */
- irda_usb_submit(self, skb, purb);
+ /* Note : at this point, the URB we've just received (purb)
+ * is still referenced by the USB layer. For example, if we
+ * have received a -ECONNRESET, uhci_cleanup_unlink() will
+ * continue to process it (in fact, cleaning it up).
+ * If we were to submit this URB, disaster would ensue.
+ * Therefore, we submit our idle URB, and put this URB in our
+ * idle slot....
+ * Jean II */
+ /* Note : with this scheme, we could submit the idle URB before
+ * processing the Rx URB. Another time... Jean II */
+
+ /* Submit the idle URB to replace the URB we've just received */
+ irda_usb_submit(self, (struct sk_buff *) self->idle_rx_urb->context,
+ self->idle_rx_urb);
+ /* Recycle Rx URB : Now, the idle URB is the present one */
+ self->idle_rx_urb = purb;
+ self->idle_rx_urb->context = skb;
}
static int irda_usb_net_init(struct net_device *dev)
@@ -998,8 +1134,11 @@ static int irda_usb_net_open(struct net_
/* Now that we can pass data to IrLAP, allow the USB layer
* to send us some data... */
- for (i = 0; i < IU_MAX_URB; i++)
+ for (i = 0; i < IU_MAX_ACTIVE_URB; i++)
irda_usb_submit(self, NULL, &(self->rx_urb[i]));
+ /* Note : we submit all the Rx URB except for one - Jean II */
+ self->idle_rx_urb = &(self->rx_urb[IU_MAX_ACTIVE_URB]);
+ self->idle_rx_urb->context = NULL;
/* Ready to play !!! */
MOD_INC_USE_COUNT;
@@ -1068,39 +1207,92 @@ static void irda_usb_net_timeout(struct
return;
}
- WARNING("%s: Tx timed out, urb->status=%d\n", netdev->name, purb->status);
- self->stats.tx_errors++;
+ /* Check empty URB */
+ purb = &(self->empty_urb);
+ if (purb->status != USB_ST_NOERROR) {
+ WARNING("%s: Empty change timed out, urb->status=%d,
+urb->transfer_flags=0x%04X\n", netdev->name, purb->status, purb->transfer_flags);
- /* Check Tx URB */
- purb = &(self->tx_urb);
- switch (purb->status) {
- case -ECONNABORTED: /* Can't find proper USB_ST_* code */
+ switch (purb->status) {
+ case -ECONNABORTED: /* -103 */
+ case -ECONNRESET: /* -104 */
+ case -ENOENT: /* -2 */
purb->status = USB_ST_NOERROR;
- netif_wake_queue(self->netdev);
done = 1;
break;
case USB_ST_URB_PENDING: /* -EINPROGRESS == -115 */
usb_unlink_urb(purb);
+ /* Note : above will *NOT* call netif_wake_queue()
+ * in completion handler - Jean II */
done = 1;
break;
default:
+ /* ??? */
break;
+ }
}
+
/* Check speed URB */
purb = &(self->speed_urb);
- switch (purb->status) {
- case -ECONNABORTED: /* Can't find proper USB_ST_* code */
+ if (purb->status != USB_ST_NOERROR) {
+ WARNING("%s: Speed change timed out, urb->status=%d,
+urb->transfer_flags=0x%04X\n", netdev->name, purb->status, purb->transfer_flags);
+
+ switch (purb->status) {
+ case -ECONNABORTED: /* -103 */
+ case -ECONNRESET: /* -104 */
+ case -ENOENT: /* -2 */
purb->status = USB_ST_NOERROR;
netif_wake_queue(self->netdev);
done = 1;
break;
- case USB_ST_URB_PENDING: /* -EINPROGRESS */
+ case USB_ST_URB_PENDING: /* -EINPROGRESS == -115 */
usb_unlink_urb(purb);
+ /* Note : above will call netif_wake_queue()
+ * in completion handler - Jean II */
done = 1;
break;
default:
+ /* ??? */
break;
+ }
}
+
+ /* Check Tx URB */
+ purb = &(self->tx_urb);
+ if (purb->status != USB_ST_NOERROR) {
+ WARNING("%s: Tx timed out, urb->status=%d,
+urb->transfer_flags=0x%04X\n", netdev->name, purb->status, purb->transfer_flags);
+
+ /* Increase error count */
+ self->stats.tx_errors++;
+
+#ifdef IU_BUG_KICK_TIMEOUT
+ /* Can't be a bad idea to reset the speed ;-) - Jean II */
+ if(self->new_speed == -1)
+ self->new_speed = self->speed;
+ if(self->new_xbofs == -1)
+ self->new_xbofs = self->xbofs;
+ irda_usb_change_speed_xbofs(self);
+#endif /* IU_BUG_KICK_TIMEOUT */
+
+ switch (purb->status) {
+ case -ECONNABORTED: /* -103 */
+ case -ECONNRESET: /* -104 */
+ case -ENOENT: /* -2 */
+ purb->status = USB_ST_NOERROR;
+ netif_wake_queue(self->netdev);
+ done = 1;
+ break;
+ case USB_ST_URB_PENDING: /* -EINPROGRESS == -115 */
+ usb_unlink_urb(purb);
+ /* Note : above will call netif_wake_queue()
+ * in completion handler - Jean II */
+ done = 1;
+ break;
+ default:
+ /* ??? */
+ break;
+ }
+ }
+
/* Maybe we need a reset */
/* Note : Some drivers seem to use a usb_set_interface() when they
* need to reset the hardware. Hum...
