irda update 5/6:

o [CORRECT] Remove all "save_flags(flags);cli();" in IrDA driver
o [FEATURE] Rework broken locking in irport
o [FEATURE] Finish locking cleanup in nsc-ircc
o [FEATURE] Improve locking in smc-ircc & w83977af_ir

drivers/net/irda/irda-usb.c

@@ -1171,7 +1171,7 @@ static inline int irda_usb_open(struct irda_usb_cb *self)
 	irda_usb_init_qos(self);
 	
 	/* Initialise list of skb beeing curently transmitted */
-	self->tx_list = hashbin_new(HB_GLOBAL);
+	self->tx_list = hashbin_new(HB_NOLOCK);	/* unused */
 
 	/* Allocate the buffer for speed changes */
 	/* Don't change this buffer size and allocation without doing

drivers/net/irda/irport.c

@@ -140,7 +140,7 @@ irport_open(int i, unsigned int iobase, unsigned int irq)
 	void *ret;
 	int err;
 
-	IRDA_DEBUG(0, __FUNCTION__ "()\n");
+	IRDA_DEBUG(1, __FUNCTION__ "()\n");
 
 	/*
 	 *  Allocate new instance of the driver
@@ -284,14 +284,13 @@ int irport_close(struct irport_cb *self)
 
 void irport_start(struct irport_cb *self)
 {
-	unsigned long flags;
 	int iobase;
 
 	iobase = self->io.sir_base;
 
 	irport_stop(self);
 	
-	spin_lock_irqsave(&self->lock, flags);
+	/* We can't lock, we may be called from a FIR driver - Jean II */
 
 	/* Initialize UART */
 	outb(UART_LCR_WLEN8, iobase+UART_LCR);  /* Reset DLAB */
@@ -299,26 +298,21 @@ void irport_start(struct irport_cb *self)
 	
 	/* Turn on interrups */
 	outb(UART_IER_RLSI | UART_IER_RDI |UART_IER_THRI, iobase+UART_IER);
-
-	spin_unlock_irqrestore(&self->lock, flags);
 }
 
 void irport_stop(struct irport_cb *self)
 {
-	unsigned long flags;
 	int iobase;
 
 	iobase = self->io.sir_base;
 
-	spin_lock_irqsave(&self->lock, flags);
+	/* We can't lock, we may be called from a FIR driver - Jean II */
 
 	/* Reset UART */
 	outb(0, iobase+UART_MCR);
 	
 	/* Turn off interrupts */
 	outb(0, iobase+UART_IER);
-
-	spin_unlock_irqrestore(&self->lock, flags);
 }
 
 /*
@@ -339,27 +333,28 @@ int irport_probe(int iobase)
  *
  *    Set speed of IrDA port to specified baudrate
  *
+ * This function should be called with irq off and spin-lock.
  */
 void irport_change_speed(void *priv, __u32 speed)
 {
 	struct irport_cb *self = (struct irport_cb *) priv;
-	unsigned long flags;
 	int iobase; 
 	int fcr;    /* FIFO control reg */
 	int lcr;    /* Line control reg */
 	int divisor;
 
-	IRDA_DEBUG(0, __FUNCTION__ "(), Setting speed to: %d\n", speed);
-
 	ASSERT(self != NULL, return;);
 
+	IRDA_DEBUG(1, "%s(), Setting speed to: %d - iobase=%#x\n",
+		    __FUNCTION__, speed, self->io.sir_base);
+
+	/* We can't lock, we may be called from a FIR driver - Jean II */
+
 	iobase = self->io.sir_base;
 	
 	/* Update accounting for new speed */
 	self->io.speed = speed;
 
-	spin_lock_irqsave(&self->lock, flags);
-
 	/* Turn off interrupts */
 	outb(0, iobase+UART_IER); 
 
@@ -387,9 +382,9 @@ void irport_change_speed(void *priv, __u32 speed)
 	outb(fcr,		  iobase+UART_FCR); /* Enable FIFO's */
 
 	/* Turn on interrups */
-	outb(/*UART_IER_RLSI|*/UART_IER_RDI/*|UART_IER_THRI*/, iobase+UART_IER);
-
-	spin_unlock_irqrestore(&self->lock, flags);
+	/* This will generate a fata interrupt storm.
+	 * People calling us will do that properly - Jean II */
+	//outb(/*UART_IER_RLSI|*/UART_IER_RDI/*|UART_IER_THRI*/, iobase+UART_IER);
 }
 
 /*
@@ -397,11 +392,14 @@ void irport_change_speed(void *priv, __u32 speed)
  *
  *    State machine for changing speed of the device. We do it this way since
  *    we cannot use schedule_timeout() when we are in interrupt context
+ *
  */
 int __irport_change_speed(struct irda_task *task)
 {
 	struct irport_cb *self;
 	__u32 speed = (__u32) task->param;
+	unsigned long flags = 0;
+	int wasunlocked = 0;
 	int ret = 0;
 
 	IRDA_DEBUG(2, __FUNCTION__ "(), <%ld>\n", jiffies); 
@@ -410,6 +408,17 @@ int __irport_change_speed(struct irda_task *task)
 
 	ASSERT(self != NULL, return -1;);
 
+	/* Locking notes : this function may be called from irq context with
+	 * spinlock, via irport_write_wakeup(), or from non-interrupt without
+	 * spinlock (from the task timer). Yuck !
+	 * This is ugly, and unsafe is the spinlock is not already aquired.
+	 * This will be fixed when irda-task get rewritten.
+	 * Jean II */
+	if (!spin_is_locked(&self->lock)) {
+		spin_lock_irqsave(&self->lock, flags);
+		wasunlocked = 1;
+	}
+
 	switch (task->state) {
 	case IRDA_TASK_INIT:
 	case IRDA_TASK_WAIT:
@@ -462,6 +471,11 @@ int __irport_change_speed(struct irda_task *task)
 		ret = -1;
 		break;
 	}	
+	/* Put stuff in the sate we found them - Jean II */
+	if(wasunlocked) {
+		spin_unlock_irqrestore(&self->lock, flags);
+	}
+
 	return ret;
 }
 
@@ -491,6 +505,9 @@ static void irport_write_wakeup(struct irport_cb *self)
 				      self->tx_buff.data, self->tx_buff.len);
 		self->tx_buff.data += actual;
 		self->tx_buff.len  -= actual;
+
+		/* Turn on transmit finished interrupt. */
+		outb(UART_IER_THRI, iobase+UART_IER); 
 	} else {
 		/* 
 		 *  Now serial buffer is almost free & we can start 
@@ -498,11 +515,12 @@ static void irport_write_wakeup(struct irport_cb *self)
 		 *  if we need to change the speed of the hardware
 		 */
 		if (self->new_speed) {
-			IRDA_DEBUG(5, __FUNCTION__ "(), Changing speed!\n");
+			IRDA_DEBUG(5, "%s(), Changing speed!\n", __FUNCTION__);
 			irda_task_execute(self, __irport_change_speed, 
 					  irport_change_speed_complete, 
 					  NULL, (void *) self->new_speed);
 			self->new_speed = 0;
+			IRDA_DEBUG(5, "%s(), Speed changed!\n", __FUNCTION__ );
 		} else {
 			/* Tell network layer that we want more frames */
 			netif_wake_queue(self->netdev);
@@ -563,7 +581,7 @@ static int irport_change_speed_complete(struct irda_task *task)
 {
 	struct irport_cb *self;
 
-	IRDA_DEBUG(0, __FUNCTION__ "()\n");
+	IRDA_DEBUG(1, __FUNCTION__ "()\n");
 
 	self = (struct irport_cb *) task->instance;
 
@@ -589,13 +607,19 @@ static void irport_timeout(struct net_device *dev)
 {
 	struct irport_cb *self;
 	int iobase;
+	unsigned long flags;
 
 	self = (struct irport_cb *) dev->priv;
 	iobase = self->io.sir_base;
 	
 	WARNING("%s: transmit timed out\n", dev->name);
+	spin_lock_irqsave(&self->lock, flags);
 	irport_start(self);
 	self->change_speed(self->priv, self->io.speed);
+	/* This will re-enable irqs */
+	outb(/*UART_IER_RLSI|*/UART_IER_RDI/*|UART_IER_THRI*/, iobase+UART_IER);
+	spin_unlock_irqrestore(&self->lock, flags);
+
 	dev->trans_start = jiffies;
 	netif_wake_queue(dev);
 }
@@ -614,7 +638,7 @@ int irport_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 	int iobase;
 	s32 speed;
 
-	IRDA_DEBUG(0, __FUNCTION__ "()\n");
+	IRDA_DEBUG(1, __FUNCTION__ "()\n");
 
 	ASSERT(dev != NULL, return 0;);
 	
@@ -625,22 +649,25 @@ int irport_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	netif_stop_queue(dev);
 	
+	/* Make sure tests *& speed change are atomic */
+	spin_lock_irqsave(&self->lock, flags);
+
 	/* Check if we need to change the speed */
 	speed = irda_get_next_speed(skb);
 	if ((speed != self->io.speed) && (speed != -1)) {
 		/* Check for empty frame */
 		if (!skb->len) {
+			/* Better go there already locked - Jean II */
 			irda_task_execute(self, __irport_change_speed, 
 					  irport_change_speed_complete, 
 					  NULL, (void *) speed);
+			spin_unlock_irqrestore(&self->lock, flags);
 			dev_kfree_skb(skb);
 			return 0;
 		} else
 			self->new_speed = speed;
 	}
 
-	spin_lock_irqsave(&self->lock, flags);
-
 	/* Init tx buffer */
 	self->tx_buff.data = self->tx_buff.head;
 
@@ -771,8 +798,9 @@ int irport_net_open(struct net_device *dev)
 	struct irport_cb *self;
 	int iobase;
 	char hwname[16];
+	unsigned long flags;
 
-	IRDA_DEBUG(0, __FUNCTION__ "()\n");
+	IRDA_DEBUG(1, __FUNCTION__ "()\n");
 	
 	ASSERT(dev != NULL, return -1;);
 	self = (struct irport_cb *) dev->priv;
@@ -786,7 +814,9 @@ int irport_net_open(struct net_device *dev)
 		return -EAGAIN;
 	}
 
+	spin_lock_irqsave(&self->lock, flags);
 	irport_start(self);
+	spin_unlock_irqrestore(&self->lock, flags);
 
 
 	/* Give self a hardware name */
@@ -818,6 +848,7 @@ int irport_net_close(struct net_device *dev)
 {
 	struct irport_cb *self;
 	int iobase;
+	unsigned long flags;
 
 	IRDA_DEBUG(4, __FUNCTION__ "()\n");
 
@@ -836,7 +867,9 @@ int irport_net_close(struct net_device *dev)
 		irlap_close(self->irlap);
 	self->irlap = NULL;
 
+	spin_lock_irqsave(&self->lock, flags);
 	irport_stop(self);
+	spin_unlock_irqrestore(&self->lock, flags);
 
 	free_irq(self->io.irq, dev);
 
@@ -951,10 +984,6 @@ static int irport_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 
 	IRDA_DEBUG(2, __FUNCTION__ "(), %s, (cmd=0x%X)\n", dev->name, cmd);
 	
-	/* Disable interrupts & save flags */
-	save_flags(flags);
-	cli();
-	
 	switch (cmd) {
 	case SIOCSBANDWIDTH: /* Set bandwidth */
 		if (!capable(CAP_NET_ADMIN))
@@ -979,14 +1008,16 @@ static int irport_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		dongle->write       = irport_raw_write;
 		dongle->set_dtr_rts = irport_set_dtr_rts;
 		
-		self->dongle = dongle;
-
 		/* Now initialize the dongle!  */
 		dongle->issue->open(dongle, &self->qos);
 		
 		/* Reset dongle */
 		irda_task_execute(dongle, dongle->issue->reset, NULL, NULL, 
 				  NULL);	
+
+		/* Make dongle available to driver only now to avoid
+		 * race conditions - Jean II */
+		self->dongle = dongle;
 		break;
 	case SIOCSMEDIABUSY: /* Set media busy */
 		if (!capable(CAP_NET_ADMIN)) {
@@ -1005,14 +1036,15 @@ static int irport_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 			break;
 		}
 
+		/* No real need to lock... */
+		spin_lock_irqsave(&self->lock, flags);
 		irport_set_dtr_rts(dev, irq->ifr_dtr, irq->ifr_rts);
+		spin_unlock_irqrestore(&self->lock, flags);
 		break;
 	default:
 		ret = -EOPNOTSUPP;
 	}
 	
-	restore_flags(flags);
-	
 	return ret;
 }
 

drivers/net/irda/irtty.c

@@ -74,8 +74,10 @@ char *driver_name = "irtty";
 int __init irtty_init(void)
 {
 	int status;
-	
-	irtty = hashbin_new( HB_LOCAL);
+
+	/* Probably no need to lock here because all operations done in
+	 * open()/close() which are already safe - Jean II */
+	irtty = hashbin_new( HB_NOLOCK);
 	if ( irtty == NULL) {
 		printk( KERN_WARNING "IrDA: Can't allocate irtty hashbin!\n");
 		return -ENOMEM;
@@ -163,6 +165,7 @@ static int irtty_open(struct tty_struct *tty)
 		return -ENOMEM;
 	}
 	memset(self, 0, sizeof(struct irtty_cb));
+	spin_lock_init(&self->lock);
 	
 	self->tty = tty;
 	tty->disc_data = self;
@@ -266,11 +269,12 @@ static int irtty_open(struct tty_struct *tty)
 static void irtty_close(struct tty_struct *tty) 
 {
 	struct irtty_cb *self = (struct irtty_cb *) tty->disc_data;
+	unsigned long flags;
 	
 	/* First make sure we're connected. */
 	ASSERT(self != NULL, return;);
 	ASSERT(self->magic == IRTTY_MAGIC, return;);
-	
+
 	/* Stop tty */
 	tty->flags &= ~(1 << TTY_DO_WRITE_WAKEUP);
 	tty->disc_data = 0;
@@ -287,6 +291,11 @@ static void irtty_close(struct tty_struct *tty)
 		rtnl_unlock();
 	}
 	
+	self = hashbin_remove(irtty, (int) self, NULL);
+
+	/* Protect access to self->task and self->?x_buff - Jean II */
+	spin_lock_irqsave(&self->lock, flags);
+
 	/* Remove speed changing task if any */
 	if (self->task)
 		irda_task_delete(self->task);
@@ -294,13 +303,12 @@ static void irtty_close(struct tty_struct *tty)
 	self->tty = NULL;
 	self->magic = 0;
 	
-	self = hashbin_remove(irtty, (int) self, NULL);
-
 	if (self->tx_buff.head)
 		kfree(self->tx_buff.head);
 	
 	if (self->rx_buff.head)
 		kfree(self->rx_buff.head);
+	spin_unlock_irqrestore(&self->lock, flags);
 	
 	kfree(self);
 	
@@ -326,6 +334,7 @@ static void irtty_stop_receiver(struct irtty_cb *self, int stop)
 	else
 		cflag |= CREAD;
 
+	/* This is unsafe, but currently under discussion - Jean II */
 	self->tty->termios->c_cflag = cflag;
 	self->tty->driver.set_termios(self->tty, &old_termios);
 }
@@ -378,6 +387,7 @@ static void __irtty_change_speed(struct irtty_cb *self, __u32 speed)
 		break;
 	}	
 
+	/* This is unsafe, but currently under discussion - Jean II */
 	self->tty->termios->c_cflag = cflag;
 	self->tty->driver.set_termios(self->tty, &old_termios);
 
@@ -393,6 +403,7 @@ static void __irtty_change_speed(struct irtty_cb *self, __u32 speed)
 static int irtty_change_speed(struct irda_task *task)
 {
 	struct irtty_cb *self;
+	unsigned long flags;
 	__u32 speed = (__u32) task->param;
 	int ret = 0;
 
@@ -401,12 +412,17 @@ static int irtty_change_speed(struct irda_task *task)
 	self = (struct irtty_cb *) task->instance;
 	ASSERT(self != NULL, return -1;);
 
+	/* Protect access to self->task - Jean II */
+	spin_lock_irqsave(&self->lock, flags);
+
 	/* Check if busy */
 	if (self->task && self->task != task) {
 		IRDA_DEBUG(0, __FUNCTION__ "(), busy!\n");
+		spin_unlock_irqrestore(&self->lock, flags);
 		return MSECS_TO_JIFFIES(10);
 	} else
 		self->task = task;
+	spin_unlock_irqrestore(&self->lock, flags);
 
 	switch (task->state) {
 	case IRDA_TASK_INIT:
@@ -501,6 +517,7 @@ static int irtty_ioctl(struct tty_struct *tty, void *file, int cmd, void *arg)
 	switch (cmd) {
 	case TCGETS:
 	case TCGETA:
+		/* Unsure about locking here, to check - Jean II */
 		return n_tty_ioctl(tty, (struct file *) file, cmd, 
 				   (unsigned long) arg);
 		break;
@@ -516,15 +533,16 @@ static int irtty_ioctl(struct tty_struct *tty, void *file, int cmd, void *arg)
 		dongle->write       = irtty_raw_write;
 		dongle->set_dtr_rts = irtty_set_dtr_rts;
 		
-		/* Bind dongle */
-		self->dongle = dongle;
-		
 		/* Now initialize the dongle!  */
 		dongle->issue->open(dongle, &self->qos);
 		
 		/* Reset dongle */
 		irda_task_execute(dongle, dongle->issue->reset, NULL, NULL, 
 				  NULL);		
+
+		/* Make dongle available to driver only now to avoid
+		 * race conditions - Jean II */
+		self->dongle = dongle;
 		break;
 	case IRTTY_IOCGET:
 		ASSERT(self->netdev != NULL, return -1;);
@@ -559,6 +577,9 @@ static void irtty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 		return;
 	}
 
+	// Are we in interrupt context ? What locking is done ? - Jean II
+	//spin_lock_irqsave(&self->lock, flags);
+
 	/* Read the characters out of the buffer */
  	while (count--) {
 		/* 
@@ -589,6 +610,7 @@ static void irtty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
 			break;
 		}
 	}
+	//spin_unlock_irqrestore(&self->lock, flags);
 }
 
 /*
@@ -626,11 +648,13 @@ static int irtty_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct irtty_cb *self;
 	int actual = 0;
 	__s32 speed;
+	unsigned long flags;
 
 	self = (struct irtty_cb *) dev->priv;
 	ASSERT(self != NULL, return 0;);
 
-	/* Lock transmit buffer */
+	/* Lock transmit buffer
+	 * this serialise operations, no need to spinlock - Jean II */
 	netif_stop_queue(dev);
 	
 	/* Check if we need to change the speed */
@@ -647,6 +671,9 @@ static int irtty_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 			self->new_speed = speed;
 	}
 
+	/* Protect access to self->tx_buff - Jean II */
+	spin_lock_irqsave(&self->lock, flags);
+
 	/* Init tx buffer*/
 	self->tx_buff.data = self->tx_buff.head;
 	
@@ -667,6 +694,8 @@ static int irtty_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 	self->tx_buff.data += actual;
 	self->tx_buff.len -= actual;
 
+	spin_unlock_irqrestore(&self->lock, flags);
+
 	dev_kfree_skb(skb);
 
 	return 0;
@@ -695,6 +724,7 @@ static void irtty_write_wakeup(struct tty_struct *tty)
 {
 	struct irtty_cb *self = (struct irtty_cb *) tty->disc_data;
 	int actual = 0;
+	unsigned long flags;
 	
 	/* 
 	 *  First make sure we're connected. 
@@ -702,6 +732,11 @@ static void irtty_write_wakeup(struct tty_struct *tty)
 	ASSERT(self != NULL, return;);
 	ASSERT(self->magic == IRTTY_MAGIC, return;);
 
+	/* Protected via netif_stop_queue(dev); - Jean II */
+
+	/* Protect access to self->tx_buff - Jean II */
+	spin_lock_irqsave(&self->lock, flags);
+
 	/* Finished with frame?  */
 	if (self->tx_buff.len > 0)  {
 		/* Write data left in transmit buffer */
@@ -710,6 +745,7 @@ static void irtty_write_wakeup(struct tty_struct *tty)
 
 		self->tx_buff.data += actual;
 		self->tx_buff.len  -= actual;
+		spin_unlock_irqrestore(&self->lock, flags);
 	} else {		
 		/* 
 		 *  Now serial buffer is almost free & we can start 
@@ -721,6 +757,9 @@ static void irtty_write_wakeup(struct tty_struct *tty)
 
 		tty->flags &= ~(1 << TTY_DO_WRITE_WAKEUP);
 
+		/* Don't change speed with irq off */
+		spin_unlock_irqrestore(&self->lock, flags);
+
 		if (self->new_speed) {
 			IRDA_DEBUG(5, __FUNCTION__ "(), Changing speed!\n");
 			irda_task_execute(self, irtty_change_speed, 
@@ -755,12 +794,17 @@ static int irtty_set_dtr_rts(struct net_device *dev, int dtr, int rts)
 {
 	struct irtty_cb *self;
 	struct tty_struct *tty;
+	//unsigned long flags;
 	mm_segment_t fs;
 	int arg = 0;
 
 	self = (struct irtty_cb *) dev->priv;
 	tty = self->tty;
 
+	/* Was protected in ioctl handler, but the serial driver doesn't
+	 * like it. This may need to change. - Jean II */
+	//spin_lock_irqsave(&self->lock, flags);
+
 #ifdef TIOCM_OUT2 /* Not defined for ARM */
 	arg = TIOCM_OUT2;
 #endif
@@ -780,11 +824,14 @@ static int irtty_set_dtr_rts(struct net_device *dev, int dtr, int rts)
 	fs = get_fs();
 	set_fs(get_ds());
 	
+	/* This is probably unsafe, but currently under discussion - Jean II */
 	if (tty->driver.ioctl(tty, NULL, TIOCMSET, (unsigned long) &arg)) { 
 		IRDA_DEBUG(2, __FUNCTION__ "(), error doing ioctl!\n");
 	}
 	set_fs(fs);
 
+	//spin_unlock_irqrestore(&self->lock, flags);
+
 	return 0;
 }
 
@@ -799,13 +846,17 @@ static int irtty_set_dtr_rts(struct net_device *dev, int dtr, int rts)
 int irtty_set_mode(struct net_device *dev, int mode)
 {
 	struct irtty_cb *self;
+	unsigned long flags;
 
 	self = (struct irtty_cb *) dev->priv;
 
 	ASSERT(self != NULL, return -1;);
 
 	IRDA_DEBUG(2, __FUNCTION__ "(), mode=%s\n", infrared_mode[mode]);
-	
+
+	/* Protect access to self->rx_buff - Jean II */
+	spin_lock_irqsave(&self->lock, flags);
+
 	/* save status for driver */
 	self->mode = mode;
 	
@@ -814,6 +865,8 @@ int irtty_set_mode(struct net_device *dev, int mode)
 	self->rx_buff.len = 0;
 	self->rx_buff.state = OUTSIDE_FRAME;
 
+	spin_unlock_irqrestore(&self->lock, flags);
+
 	return 0;
 }
 
@@ -955,7 +1008,6 @@ static int irtty_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	struct if_irda_req *irq = (struct if_irda_req *) rq;
 	struct irtty_cb *self;
 	dongle_t *dongle;
-	unsigned long flags;
 	int ret = 0;
 
 	ASSERT(dev != NULL, return -1;);
@@ -971,8 +1023,7 @@ static int irtty_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 	 * irda_device_dongle_init() can't be locked.
 	 * irda_task_execute() doesn't need to be locked (but
 	 * irtty_change_speed() should protect itself).
-	 * As this driver doesn't have spinlock protection, keep
-	 * old fashion locking :-(
+	 * Other calls protect themselves.
 	 * Jean II
 	 */
 	
@@ -1025,20 +1076,14 @@ static int irtty_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		if (!capable(CAP_NET_ADMIN))
 			ret = -EPERM;
 		else {
-			save_flags(flags);
-			cli();
 			irtty_set_dtr_rts(dev, irq->ifr_dtr, irq->ifr_rts);
-			restore_flags(flags);
 		}
 		break;
 	case SIOCSMODE:
 		if (!capable(CAP_NET_ADMIN))
 			ret = -EPERM;
 		else {
-			save_flags(flags);
-			cli();
 			irtty_set_mode(dev, irq->ifr_mode);
-			restore_flags(flags);
 		}
 		break;
 	default:

drivers/net/irda/nsc-ircc.c

@@ -870,7 +870,6 @@ static void nsc_ircc_init_dongle_interface (int iobase, int dongle_id)
  */
 static void nsc_ircc_change_dongle_speed(int iobase, int speed, int dongle_id)
 {
-	unsigned long flags;
 	__u8 bank;
 
 	/* Save current bank */
@@ -916,11 +915,10 @@ static void nsc_ircc_change_dongle_speed(int iobase, int speed, int dongle_id)
 		outb(0x01, iobase+4);
 
 		if (speed == 4000000) {
-			save_flags(flags);
-			cli();
+			/* There was a cli() there, but we now are already
+			 * under spin_lock_irqsave() - JeanII */
 			outb(0x81, iobase+4);
 			outb(0x80, iobase+4);
-			restore_flags(flags);
 		} else
 			outb(0x00, iobase+4);
 		break;
@@ -1960,33 +1958,30 @@ static int nsc_ircc_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 
 	IRDA_DEBUG(2, __FUNCTION__ "(), %s, (cmd=0x%X)\n", dev->name, cmd);
 	
-	/* Disable interrupts & save flags */
-	save_flags(flags);
-	cli();
-
 	switch (cmd) {
 	case SIOCSBANDWIDTH: /* Set bandwidth */
 		if (!capable(CAP_NET_ADMIN)) {
 			ret = -EPERM;
-			goto out;
+			break;
 		}
+		spin_lock_irqsave(&self->lock, flags);
 		nsc_ircc_change_speed(self, irq->ifr_baudrate);
+		spin_unlock_irqrestore(&self->lock, flags);
 		break;
 	case SIOCSMEDIABUSY: /* Set media busy */
 		if (!capable(CAP_NET_ADMIN)) {
 			ret = -EPERM;
-			goto out;
+			break;
 		}
 		irda_device_set_media_busy(self->netdev, TRUE);
 		break;
 	case SIOCGRECEIVING: /* Check if we are receiving right now */
+		/* This is already protected */
 		irq->ifr_receiving = nsc_ircc_is_receiving(self);
 		break;
 	default:
 		ret = -EOPNOTSUPP;
 	}
-out:
-	restore_flags(flags);
 	return ret;
 }
 

drivers/net/irda/smc-ircc.c

@@ -431,6 +431,7 @@ static int __init ircc_open(unsigned int fir_base, unsigned int sir_base)
 	struct ircc_cb *self;
         struct irport_cb *irport;
 	unsigned char low, high, chip, config, dma, irq, version;
+	unsigned long flags;
 
 
 	IRDA_DEBUG(0, __FUNCTION__ "\n");
@@ -484,7 +485,6 @@ static int __init ircc_open(unsigned int fir_base, unsigned int sir_base)
 		return -ENOMEM;
 	}
 	memset(self, 0, sizeof(struct ircc_cb));
-	spin_lock_init(&self->lock);
 
 	/* Max DMA buffer size needed = (data_size + 6) * (window_size) + 6; */
 	self->rx_buff.truesize = 4000; 
@@ -555,6 +555,9 @@ static int __init ircc_open(unsigned int fir_base, unsigned int sir_base)
 
 	request_region(self->io->fir_base, CHIP_IO_EXTENT, driver_name);
 
+	/* Don't allow irport to change under us - Jean II */
+	spin_lock_irqsave(&self->irport->lock, flags);
+
 	/* Initialize QoS for this device */
 	irda_init_max_qos_capabilies(&irport->qos);
 	
@@ -581,6 +584,7 @@ static int __init ircc_open(unsigned int fir_base, unsigned int sir_base)
 	self->netdev->stop   = &ircc_net_close;
 
 	irport_start(self->irport);
+	spin_unlock_irqrestore(&self->irport->lock, flags);
 
         self->pmdev = pm_register(PM_SYS_DEV, PM_SYS_IRDA, ircc_pmproc);
         if (self->pmdev)
@@ -598,6 +602,7 @@ static int __init ircc_open(unsigned int fir_base, unsigned int sir_base)
  *
  *    Change the speed of the device
  *
+ * This function should be called with irq off and spin-lock.
  */
 static void ircc_change_speed(void *priv, u32 speed)
 {
@@ -658,6 +663,7 @@ static void ircc_change_speed(void *priv, u32 speed)
 
 	/* Make special FIR init if necessary */
 	if (speed > 115200) {
+		/* No need to lock, already locked - Jean II */
 		irport_stop(self->irport);
 
 		/* Install FIR transmit handler */
@@ -674,6 +680,7 @@ static void ircc_change_speed(void *priv, u32 speed)
 	} else {
 		/* Install SIR transmit handler */
 		dev->hard_start_xmit = &irport_hard_xmit;
+		/* No need to lock, already locked - Jean II */
 		irport_start(self->irport);
 		
 	        IRDA_DEBUG(0, __FUNCTION__ 
@@ -727,20 +734,26 @@ static int ircc_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	netif_stop_queue(dev);
 
+	/* Make sure tests *& speed change are atomic */
+	spin_lock_irqsave(&self->irport->lock, flags);
+
+	/* Note : you should make sure that speed changes are not going
+	 * to corrupt any outgoing frame. Look at nsc-ircc for the gory
+	 * details - Jean II */
+
 	/* Check if we need to change the speed after this frame */
 	speed = irda_get_next_speed(skb);
 	if ((speed != self->io->speed) && (speed != -1)) {
 		/* Check for empty frame */
 		if (!skb->len) {
 			ircc_change_speed(self, speed); 
+			spin_unlock_irqrestore(&self->irport->lock, flags);
 			dev_kfree_skb(skb);
 			return 0;
 		} else
 			self->new_speed = speed;
 	}
 	
-	spin_lock_irqsave(&self->lock, flags);
-
 	memcpy(self->tx_buff.head, skb->data, skb->len);
 
 	self->tx_buff.len = skb->len;
@@ -763,7 +776,7 @@ static int ircc_hard_xmit(struct sk_buff *skb, struct net_device *dev)
 		/* Transmit frame */
 		ircc_dma_xmit(self, iobase, 0);
 	}
-	spin_unlock_irqrestore(&self->lock, flags);
+	spin_unlock_irqrestore(&self->irport->lock, flags);
 	dev_kfree_skb(skb);
 
 	return 0;
@@ -985,12 +998,13 @@ static void ircc_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 
 	/* Check if we should use the SIR interrupt handler */
 	if (self->io->speed < 576000) {
+		/* Will spinlock itself - Jean II */
 		irport_interrupt(irq, dev_id, regs);
 		return;
 	}
 	iobase = self->io->fir_base;
 
-	spin_lock(&self->lock);	
+	spin_lock(&self->irport->lock);	
 
 	register_bank(iobase, 0);
 	iir = inb(iobase+IRCC_IIR);
@@ -1013,7 +1027,7 @@ static void ircc_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 	register_bank(iobase, 0);
 	outb(IRCC_IER_ACTIVE_FRAME|IRCC_IER_EOM, iobase+IRCC_IER);
 
-	spin_unlock(&self->lock);
+	spin_unlock(&self->irport->lock);
 }
 
 #if 0 /* unused */
@@ -1128,17 +1142,15 @@ static void ircc_suspend(struct ircc_cb *self)
 
 static void ircc_wakeup(struct ircc_cb *self)
 {
-	unsigned long flags;
-
 	if (!self->io->suspended)
 		return;
 
-	save_flags(flags);
-	cli();
+	/* The code was doing a "cli()" here, but this can't be right.
+	 * If you need protection, do it in net_open with a spinlock
+	 * or give a good reason. - Jean II */
 
 	ircc_net_open(self->netdev);
 	
-	restore_flags(flags);
 	MESSAGE("%s, Waking up\n", driver_name);
 }
 
@@ -1174,6 +1186,7 @@ static int __exit ircc_close(struct ircc_cb *self)
 
         iobase = self->irport->io.fir_base;
 
+	/* This will destroy irport */
 	irport_close(self->irport);
 
 	/* Stop interrupts */
@@ -1187,6 +1200,7 @@ static int __exit ircc_close(struct ircc_cb *self)
         outb(IRCC_CFGA_IRDA_SIR_A|IRCC_CFGA_TX_POLARITY, iobase+IRCC_SCE_CFGA);
         outb(IRCC_CFGB_IR, iobase+IRCC_SCE_CFGB);
 #endif
+
 	/* Release the PORT that this driver is using */
 	IRDA_DEBUG(0, __FUNCTION__ "(), releasing 0x%03x\n", iobase);
 

drivers/net/irda/w83977af_ir.c

@@ -175,6 +175,7 @@ int w83977af_open(int i, unsigned int iobase, unsigned int irq,
 		return -ENOMEM;
 	}
 	memset(self, 0, sizeof(struct w83977af_ir));
+	spin_lock_init(&self->lock);
    
 	/* Need to store self somewhere */
 	dev_self[i] = self;
@@ -603,8 +604,7 @@ static void w83977af_dma_write(struct w83977af_ir *self, int iobase)
 	switch_bank(iobase, SET2);
 	outb(ADCR1_D_CHSW|/*ADCR1_DMA_F|*/ADCR1_ADV_SL, iobase+ADCR1);
 #ifdef CONFIG_NETWINDER_TX_DMA_PROBLEMS
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&self->lock, flags);
 
 	disable_dma(self->io.dma);
 	clear_dma_ff(self->io.dma);
@@ -623,7 +623,7 @@ static void w83977af_dma_write(struct w83977af_ir *self, int iobase)
 	hcr = inb(iobase+HCR);
 	outb(hcr | HCR_EN_DMA, iobase+HCR);
 	enable_dma(self->io.dma);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&self->lock, flags);
 #else	
 	outb(inb(iobase+HCR) | HCR_EN_DMA | HCR_TX_WT, iobase+HCR);
 #endif
@@ -761,8 +761,7 @@ int w83977af_dma_receive(struct w83977af_ir *self)
 	self->rx_buff.data = self->rx_buff.head;
 
 #ifdef CONFIG_NETWINDER_RX_DMA_PROBLEMS
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&self->lock, flags);
 
 	disable_dma(self->io.dma);
 	clear_dma_ff(self->io.dma);
@@ -788,7 +787,7 @@ int w83977af_dma_receive(struct w83977af_ir *self)
 	hcr = inb(iobase+HCR);
 	outb(hcr | HCR_EN_DMA, iobase+HCR);
 	enable_dma(self->io.dma);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&self->lock, flags);
 #else	
 	outb(inb(iobase+HCR) | HCR_EN_DMA, iobase+HCR);
 #endif
@@ -1334,10 +1333,8 @@ static int w83977af_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 
 	IRDA_DEBUG(2, __FUNCTION__ "(), %s, (cmd=0x%X)\n", dev->name, cmd);
 	
-	/* Disable interrupts & save flags */
-	save_flags(flags);
-	cli();
-	
+	spin_lock_irqsave(&self->lock, flags);
+
 	switch (cmd) {
 	case SIOCSBANDWIDTH: /* Set bandwidth */
 		if (!capable(CAP_NET_ADMIN)) {
@@ -1360,7 +1357,7 @@ static int w83977af_net_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		ret = -EOPNOTSUPP;
 	}
 out:
-	restore_flags(flags);
+	spin_unlock_irqrestore(&self->lock, flags);
 	return ret;
 }
 

include/net/irda/irtty.h

@@ -62,6 +62,9 @@ struct irtty_cb {
 	struct qos_info qos;       /* QoS capabilities for this device */
 	dongle_t *dongle;          /* Dongle driver */
 
+
+	spinlock_t lock;           /* For serializing operations */
+
 	__u32 new_speed;
  	__u32 flags;               /* Interface flags */
 

include/net/irda/smc-ircc.h

@@ -165,7 +165,9 @@ struct ircc_cb {
 
 	struct irport_cb *irport;
 
-	spinlock_t lock;           /* For serializing operations */
+	/* Locking : half of our operations are done with irport, so we
+	 * use the irport spinlock to make sure *everything* is properly
+	 * synchronised - Jean II */
 	
 	__u32 new_speed;
 	__u32 flags;               /* Interface flags */

include/net/irda/w83977af_ir.h

@@ -179,6 +179,11 @@ struct w83977af_ir {
 	chipio_t io;               /* IrDA controller information */
 	iobuff_t tx_buff;          /* Transmit buffer */
 	iobuff_t rx_buff;          /* Receive buffer */
+
+	/* Note : currently locking is *very* incomplete, but this
+	 * will get you started. Check in nsc-ircc.c for a proper
+	 * locking strategy. - Jean II */
+	spinlock_t lock;           /* For serializing operations */
 	
 	__u32 flags;               /* Interface flags */
 	__u32 new_speed;