[PATCH] : ir257_usb_disconnect_atomic-2.diff

[PATCH] : ir257_usb_disconnect_atomic-2.diff

[Jean Tourrilhes]

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");

Re: [PATCH] : ir257_usb_disconnect_atomic-2.diff

[Greg KH]

On Tue, Apr 02, 2002 at 06:24:13PM -0800, Jean Tourrilhes wrote:
> @@ -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

That's one of the next things I'm going to be working on fixing :)

The patch looks good.  Thanks for setting the proper GFP_* flag.

greg k-h

Re: [PATCH] : ir257_usb_disconnect_atomic-2.diff

[Jean Tourrilhes]

On Tue, Apr 02, 2002 at 07:00:38PM -0800, Greg KH wrote:
> On Tue, Apr 02, 2002 at 06:24:13PM -0800, Jean Tourrilhes wrote:
> > @@ -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
> 
> That's one of the next things I'm going to be working on fixing :)

	By the time you will "fix" that, all the USB driver will be
fixed to workaround this issue. Actually, it force to be a bit more
careful about the disconnect, and avoid zombies instances all over the
place, so is not such a bad thing after all.

> The patch looks good.  Thanks for setting the proper GFP_* flag.

	That was Martin... Don't need to thank, because the impact is
limited to the IrDA driver...

> greg k-h

	Have fun...

	Jean