[PATCH] : ir257_usb_disconnect_atomic-2.diff

Jean Tourrilhes (jt@bougret.hpl.hp.com)
Tue, 2 Apr 2002 18:24:13 -0800


ir257_usb_disconnect_atomic-2.diff :
----------------------------------
o [CRITICA] Fix race condition between disconnect and the rest
o [CRITICA] Force synchronous unlink of URBs in disconnect
o [CRITICA] Cleanup instance if disconnect before close
<Following patch from Martin Diehl>
o [CRITICA] Call usb_submit_urb() with GPF_ATOMIC

diff -u -p linux/drivers/net/irda/irda-usb.d5.c linux/drivers/net/irda/irda-usb.c
--- linux/drivers/net/irda/irda-usb.d5.c Fri Mar 22 16:24:44 2002
+++ linux/drivers/net/irda/irda-usb.c Tue Apr 2 17:01:21 2002
@@ -45,6 +45,8 @@
* 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.
*
* Jean II
*/
@@ -243,10 +245,10 @@ static void irda_usb_build_header(struct
/*------------------------------------------------------------------*/
/*
* Send a command to change the speed of the dongle
+ * Need to be called with spinlock on.
*/
static void irda_usb_change_speed_xbofs(struct irda_usb_cb *self)
{
- unsigned long flags;
__u8 *frame;
struct urb *urb;
int ret;
@@ -261,8 +263,6 @@ static void irda_usb_change_speed_xbofs(
return;
}

- spin_lock_irqsave(&self->lock, flags);
-
/* Allocate the fake frame */
frame = self->speed_buff;

@@ -278,10 +278,10 @@ static void irda_usb_change_speed_xbofs(
urb->transfer_flags = USB_QUEUE_BULK | USB_ASYNC_UNLINK;
urb->timeout = MSECS_TO_JIFFIES(100);

- if ((ret = usb_submit_urb(urb, GFP_KERNEL))) {
+ /* Irq disabled -> GFP_ATOMIC */
+ if ((ret = usb_submit_urb(urb, GFP_ATOMIC))) {
WARNING(__FUNCTION__ "(), failed Speed URB\n");
}
- spin_unlock_irqrestore(&self->lock, flags);
}

/*------------------------------------------------------------------*/
@@ -335,14 +335,20 @@ static int irda_usb_hard_xmit(struct sk_
s16 xbofs;
int res, mtt;

- /* Check if the device is still there */
+ netif_stop_queue(netdev);
+
+ /* Protect us from USB callbacks, net watchdog and else. */
+ spin_lock_irqsave(&self->lock, flags);
+
+ /* 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)) {
IRDA_DEBUG(0, __FUNCTION__ "(), Device is gone...\n");
+ spin_unlock_irqrestore(&self->lock, flags);
return 1; /* Failed */
}

- netif_stop_queue(netdev);
-
/* Check if we need to change the number of xbofs */
xbofs = irda_get_next_xbofs(skb);
if ((xbofs != self->xbofs) && (xbofs != -1)) {
@@ -366,16 +372,14 @@ static int irda_usb_hard_xmit(struct sk_
* Jean II */
irda_usb_change_speed_xbofs(self);
netdev->trans_start = jiffies;
- dev_kfree_skb(skb);
/* Will netif_wake_queue() in callback */
- return 0;
+ goto drop;
}
}

if (urb->status != 0) {
WARNING(__FUNCTION__ "(), URB still in use!\n");
- dev_kfree_skb(skb);
- return 0;
+ goto drop;
}

/* Make sure there is room for IrDA-USB header. The actual
@@ -386,13 +390,10 @@ static int irda_usb_hard_xmit(struct sk_
IRDA_DEBUG(0, __FUNCTION__ "(), Insuficient skb headroom.\n");
if (skb_cow(skb, USB_IRDA_HEADER)) {
WARNING(__FUNCTION__ "(), failed skb_cow() !!!\n");
- dev_kfree_skb(skb);
- return 0;
+ goto drop;
}
}

- spin_lock_irqsave(&self->lock, flags);
-
/* Change setting for next frame */
irda_usb_build_header(self, skb_push(skb, USB_IRDA_HEADER), 0);

@@ -457,8 +458,8 @@ static int irda_usb_hard_xmit(struct sk_
}
}

- /* Ask USB to send the packet */
- if ((res = usb_submit_urb(urb, GFP_KERNEL))) {
+ /* Ask USB to send the packet - Irq disabled -> GFP_ATOMIC */
+ if ((res = usb_submit_urb(urb, GFP_ATOMIC))) {
WARNING(__FUNCTION__ "(), failed Tx URB\n");
self->stats.tx_errors++;
/* Let USB recover : We will catch that in the watchdog */
@@ -473,6 +474,12 @@ static int irda_usb_hard_xmit(struct sk_
spin_unlock_irqrestore(&self->lock, flags);

return 0;
+
+drop:
+ /* Drop silently the skb and exit */
+ dev_kfree_skb(skb);
+ spin_unlock_irqrestore(&self->lock, flags);
+ return 0;
}

/*------------------------------------------------------------------*/
@@ -481,6 +488,7 @@ static int irda_usb_hard_xmit(struct sk_
*/
static void write_bulk_callback(struct urb *urb)
{
+ unsigned long flags;
struct sk_buff *skb = urb->context;
struct irda_usb_cb *self = ((struct irda_skb_cb *) skb->cb)->context;

@@ -511,11 +519,15 @@ static void write_bulk_callback(struct u
}

/* urb is now available */
- urb->status = 0;
+ //urb->status = 0; -> tested above
+
+ /* Make sure we read self->present properly */
+ spin_lock_irqsave(&self->lock, flags);

/* If the network is closed, stop everything */
if ((!self->netopen) || (!self->present)) {
IRDA_DEBUG(0, __FUNCTION__ "(), Network is gone...\n");
+ spin_unlock_irqrestore(&self->lock, flags);
return;
}

@@ -527,6 +539,7 @@ static void write_bulk_callback(struct u
/* Otherwise, allow the stack to send more packets */
netif_wake_queue(self->netdev);
}
+ spin_unlock_irqrestore(&self->lock, flags);
}

/*------------------------------------------------------------------*/
@@ -540,15 +553,20 @@ static void write_bulk_callback(struct u
*/
static void irda_usb_net_timeout(struct net_device *netdev)
{
+ unsigned long flags;
struct irda_usb_cb *self = netdev->priv;
struct urb *urb;
int done = 0; /* If we have made any progress */

IRDA_DEBUG(0, __FUNCTION__ "(), Network layer thinks we timed out!\n");

+ /* Protect us from USB callbacks, net Tx and else. */
+ spin_lock_irqsave(&self->lock, flags);
+
if ((!self) || (!self->present)) {
WARNING(__FUNCTION__ "(), device not present!\n");
netif_stop_queue(netdev);
+ spin_unlock_irqrestore(&self->lock, flags);
return;
}

@@ -623,6 +641,7 @@ static void irda_usb_net_timeout(struct
break;
}
}
+ spin_unlock_irqrestore(&self->lock, flags);

/* Maybe we need a reset */
/* Note : Some drivers seem to use a usb_set_interface() when they
@@ -736,8 +755,9 @@ static void irda_usb_submit(struct irda_
* irda_usb_net_close() -> free the skb - Jean II */
urb->status = 0;
urb->next = NULL; /* Don't auto resubmit URBs */
-
- ret = usb_submit_urb(urb, GFP_KERNEL);
+
+ /* Can be called from irda_usb_receive (irq handler) -> GFP_ATOMIC */
+ ret = usb_submit_urb(urb, GFP_ATOMIC);
if (ret) {
/* If this ever happen, we are in deep s***.
* Basically, the Rx path will stop... */
@@ -1014,8 +1034,10 @@ static int irda_usb_net_close(struct net
urb->context = NULL;
}
}
- /* Cancel Tx and speed URB */
+ /* Cancel Tx and speed URB - need to be synchronous to avoid races */
+ self->tx_urb->transfer_flags &= ~USB_ASYNC_UNLINK;
usb_unlink_urb(self->tx_urb);
+ self->speed_urb->transfer_flags &= ~USB_ASYNC_UNLINK;
usb_unlink_urb(self->speed_urb);

/* Stop and remove instance of IrLAP */
@@ -1034,6 +1056,7 @@ static int irda_usb_net_close(struct net
*/
static int irda_usb_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
{
+ unsigned long flags;
struct if_irda_req *irq = (struct if_irda_req *) rq;
struct irda_usb_cb *self;
int ret = 0;
@@ -1044,23 +1067,26 @@ static int irda_usb_net_ioctl(struct net

IRDA_DEBUG(2, __FUNCTION__ "(), %s, (cmd=0x%X)\n", dev->name, cmd);

- /* Check if the device is still there */
- if(!self->present)
- return -EFAULT;
-
switch (cmd) {
case SIOCSBANDWIDTH: /* Set bandwidth */
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- /* Set the desired speed */
- self->new_speed = irq->ifr_baudrate;
- irda_usb_change_speed_xbofs(self);
- /* Note : will spinlock in above function */
+ /* Protect us from USB callbacks, net watchdog and else. */
+ spin_lock_irqsave(&self->lock, flags);
+ /* Check if the device is still there */
+ if(self->present) {
+ /* Set the desired speed */
+ self->new_speed = irq->ifr_baudrate;
+ irda_usb_change_speed_xbofs(self);
+ }
+ spin_unlock_irqrestore(&self->lock, flags);
break;
case SIOCSMEDIABUSY: /* Set media busy */
if (!capable(CAP_NET_ADMIN))
return -EPERM;
- irda_device_set_media_busy(self->netdev, TRUE);
+ /* Check if the IrDA stack is still there */
+ if(self->netopen)
+ irda_device_set_media_busy(self->netdev, TRUE);
break;
case SIOCGRECEIVING: /* Check if we are receiving right now */
irq->ifr_receiving = irda_usb_is_receiving(self);
@@ -1410,8 +1436,7 @@ static void *irda_usb_probe(struct usb_d
dev->descriptor.idProduct);

/* Try to cleanup all instance that have a pending disconnect
- * Instance will be in this state is the disconnect() occurs
- * before the net_close().
+ * In theory, it can't happen any longer.
* Jean II */
for (i = 0; i < NIRUSB; i++) {
struct irda_usb_cb *irda = &irda_instance[i];
@@ -1519,33 +1544,47 @@ static void *irda_usb_probe(struct usb_d
/*
* The current irda-usb device is removed, the USB layer tell us
* to shut it down...
+ * One of the constraints is that when we exit this function,
+ * we cannot use the usb_device no more. Gone. Destroyed. kfree().
+ * Most other subsystem allow you to destroy the instance at a time
+ * when it's convenient to you, to postpone it to a later date, but
+ * not the USB subsystem.
+ * So, we must make bloody sure that everything gets deactivated.
+ * Jean II
*/
static void irda_usb_disconnect(struct usb_device *dev, void *ptr)
{
+ unsigned long flags;
struct irda_usb_cb *self = (struct irda_usb_cb *) ptr;
int i;

IRDA_DEBUG(1, __FUNCTION__ "()\n");

- /* Oups ! We are not there any more */
+ /* Make sure that the Tx path is not executing. - Jean II */
+ spin_lock_irqsave(&self->lock, flags);
+
+ /* Oups ! We are not there any more.
+ * This will stop/desactivate the Tx path. - Jean II */
self->present = 0;

- /* Hum... Check if networking is still active */
- if (self->netopen) {
+ /* We need to have irq enabled to unlink the URBs. That's OK,
+ * at this point the Tx path is gone - Jean II */
+ spin_unlock_irqrestore(&self->lock, flags);
+
+ /* Hum... Check if networking is still active (avoid races) */
+ if((self->netopen) || (self->irlap)) {
/* Accept no more transmissions */
/*netif_device_detach(self->netdev);*/
netif_stop_queue(self->netdev);
/* Stop all the receive URBs */
for (i = 0; i < IU_MAX_RX_URBS; i++)
usb_unlink_urb(self->rx_urb[i]);
- /* Cancel Tx and speed URB */
+ /* Cancel Tx and speed URB.
+ * Toggle flags to make sure it's synchronous. */
+ self->tx_urb->transfer_flags &= ~USB_ASYNC_UNLINK;
usb_unlink_urb(self->tx_urb);
+ self->speed_urb->transfer_flags &= ~USB_ASYNC_UNLINK;
usb_unlink_urb(self->speed_urb);
-
- IRDA_DEBUG(0, __FUNCTION__ "(), postponing disconnect, network is still active...\n");
- /* better not do anything just yet, usb_irda_cleanup()
- * will do whats needed */
- return;
}

/* Cleanup the device stuff */
@@ -1556,7 +1595,7 @@ static void irda_usb_disconnect(struct u
/* Clean up our urbs */
for (i = 0; i < IU_MAX_RX_URBS; i++)
usb_free_urb(self->rx_urb[i]);
- /* Cancel Tx and speed URB */
+ /* Clean up Tx and speed URB */
usb_free_urb(self->tx_urb);
usb_free_urb(self->speed_urb);

@@ -1603,7 +1642,8 @@ void __exit usb_irda_cleanup(void)
struct irda_usb_cb *irda = NULL;
int i;

- /* Find zombie instances and kill them... */
+ /* Find zombie instances and kill them...
+ * In theory, it can't happen any longer. Jean II */
for (i = 0; i < NIRUSB; i++) {
irda = &irda_instance[i];
/* If the Device is zombie */
@@ -1626,3 +1666,4 @@ MODULE_PARM(qos_mtt_bits, "i");
MODULE_PARM_DESC(qos_mtt_bits, "Minimum Turn Time");
MODULE_AUTHOR("Roman Weissgaerber <weissg@vienna.at>, Dag Brattli <dag@brattli.net> and Jean Tourrilhes <jt@hpl.hp.com>");
MODULE_DESCRIPTION("IrDA-USB Dongle Driver");
+MODULE_LICENSE("GPL");
-
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/