cleanup of drivers/usb/serial/whiteheat.c

Hi,

this driver was long known to be unclean. This should fix all issues.
However, I have no hardware to test, so I'd appreciate any comment.

	Regards
		Oliver

-----

--- a/drivers/usb/serial/whiteheat.c	2007-03-20 20:34:36.000000000 +0100
+++ b/drivers/usb/serial/whiteheat.c	2007-03-22 13:05:42.000000000 +0100
@@ -74,6 +74,7 @@
 #include <linux/tty_flip.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <asm/uaccess.h>
 #include <asm/termbits.h>
 #include <linux/usb.h>
@@ -203,7 +204,8 @@
 
 
 struct whiteheat_command_private {
-	spinlock_t		lock;
+	struct mutex		mutex;
+	spinlock_t		buflock;
 	__u8			port_running;
 	__u8			command_finished;
 	wait_queue_head_t	wait_command;	/* for handling sleeping while waiting for a command to finish */
@@ -225,6 +227,7 @@
 	spinlock_t		lock;
 	__u8			flags;
 	__u8			mcr;
+	char			shutting_down;
 	struct list_head	rx_urbs_free;
 	struct list_head	rx_urbs_submitted;
 	struct list_head	rx_urb_q;
@@ -495,7 +498,8 @@
 		goto no_command_private;
 	}
 
-	spin_lock_init(&command_info->lock);
+	spin_lock_init(&command_info->buflock);
+	mutex_init(&command_info->mutex);
 	command_info->port_running = 0;
 	init_waitqueue_head(&command_info->wait_command);
 	usb_set_serial_port_data(command_port, command_info);
@@ -552,7 +556,6 @@
 	return -ENOMEM;
 }
 
-
 static void whiteheat_shutdown (struct usb_serial *serial)
 {
 	struct usb_serial_port *command_port;
@@ -573,6 +576,11 @@
 	for (i = 0; i < serial->num_ports; i++) {
 		port = serial->port[i];
 		info = usb_get_serial_port_data(port);
+
+		spin_lock_irq(&info->lock);
+		info->shutting_down = 1;
+		spin_unlock_irq(&info->lock);
+
 		list_for_each_safe(tmp, tmp2, &info->rx_urbs_free) {
 			list_del(tmp);
 			wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
@@ -589,6 +597,7 @@
 			kfree(urb->transfer_buffer);
 			usb_free_urb(urb);
 		}
+		flush_scheduled_work();
 		kfree(info);
 	}
 
@@ -685,21 +694,35 @@
 
 	/* shutdown our bulk reads and writes */
 	spin_lock_irqsave(&info->lock, flags);
+	info->shutting_down = 1;
+	smp_wmb(); 
+	/* this prevents new urbs in the queue
+	 * thus we cannot miss an urb due to dropping
+	 * the lock. We might doubly kill, but that's
+	 * harmless */
 	list_for_each_safe(tmp, tmp2, &info->rx_urbs_submitted) {
 		wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
 		urb = wrap->urb;
+		spin_unlock_irqrestore(&info->lock, flags);
 		usb_kill_urb(urb);
-		list_move(tmp, &info->rx_urbs_free);
+		spin_lock_irqsave(&info->lock, flags);
 	}
+	/* must walk again - cannot assume they stayed on the list */
+	list_for_each_safe(tmp, tmp2, &info->rx_urbs_submitted)
+		list_move(tmp, &info->rx_urbs_free);
+
 	list_for_each_safe(tmp, tmp2, &info->rx_urb_q)
 		list_move(tmp, &info->rx_urbs_free);
 
 	list_for_each_safe(tmp, tmp2, &info->tx_urbs_submitted) {
 		wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
 		urb = wrap->urb;
+		spin_unlock_irqrestore(&info->lock, flags);
 		usb_kill_urb(urb);
-		list_move(tmp, &info->tx_urbs_free);
+		spin_lock_irqsave(&info->lock, flags);
 	}
+	list_for_each_safe(tmp, tmp2, &info->tx_urbs_submitted)
+		list_move(tmp, &info->tx_urbs_free);
 	spin_unlock_irqrestore(&info->lock, flags);
 
 	stop_command_port(port->serial);
@@ -724,7 +747,7 @@
 
 	if (count == 0) {
 		dbg("%s - write request of 0 bytes", __FUNCTION__);
-		return (0);
+		return 0;
 	}
 
 	while (count) {
@@ -746,24 +769,28 @@
 
 		urb->dev = serial->dev;
 		urb->transfer_buffer_length = bytes;
-		result = usb_submit_urb(urb, GFP_ATOMIC);
-		if (result) {
-			err("%s - failed submitting write urb, error %d", __FUNCTION__, result);
-			sent = result;
-			spin_lock_irqsave(&info->lock, flags);
+		spin_lock_irqsave(&info->lock, flags);
+		if (!info->shutting_down) {
+			result = usb_submit_urb(urb, GFP_ATOMIC);
+			if (result) {
+				err("%s - failed submitting write urb, error %d", __FUNCTION__, result);
+				list_add(tmp, &info->tx_urbs_free);
+				spin_unlock_irqrestore(&info->lock, flags);
+				break;
+			} else {
+				sent += bytes;
+				count -= bytes;
+				list_add(tmp, &info->tx_urbs_submitted);
+				spin_unlock_irqrestore(&info->lock, flags);
+			}
+		} else {
 			list_add(tmp, &info->tx_urbs_free);
 			spin_unlock_irqrestore(&info->lock, flags);
 			break;
-		} else {
-			sent += bytes;
-			count -= bytes;
-			spin_lock_irqsave(&info->lock, flags);
-			list_add(tmp, &info->tx_urbs_submitted);
-			spin_unlock_irqrestore(&info->lock, flags);
 		}
 	}
 
-	return sent;
+	return sent ? sent : result;
 }
 
 
@@ -865,10 +892,10 @@
 			break;
 
 		default:
-			break;
+			return -ENOIOCTLCMD;
 	}
 
-	return -ENOIOCTLCMD;
+	return 0;
 }
 
 
@@ -920,7 +947,7 @@
 	spin_unlock_irqrestore(&info->lock, flags);
 
 	dbg ("%s - returns %d", __FUNCTION__, chars);
-	return (chars);
+	return chars;
 }
 
 
@@ -979,7 +1006,6 @@
 	struct whiteheat_command_private *command_info;
 	unsigned char *data = urb->transfer_buffer;
 	int result;
-	unsigned long flags;
 
 	dbg("%s", __FUNCTION__);
 
@@ -995,7 +1021,7 @@
 		dbg ("%s - command_info is NULL, exiting.", __FUNCTION__);
 		return;
 	}
-	spin_lock_irqsave(&command_info->lock, flags);
+	spin_lock(&command_info->buflock);
 
 	if (data[0] == WHITEHEAT_CMD_COMPLETE) {
 		command_info->command_finished = WHITEHEAT_CMD_COMPLETE;
@@ -1017,7 +1043,7 @@
 	/* Continue trying to always read */
 	command_port->read_urb->dev = command_port->serial->dev;
 	result = usb_submit_urb(command_port->read_urb, GFP_ATOMIC);
-	spin_unlock_irqrestore(&command_info->lock, flags);
+	spin_unlock(&command_info->buflock);
 	if (result)
 		dbg("%s - failed resubmitting read urb, error %d", __FUNCTION__, result);
 }
@@ -1040,19 +1066,14 @@
 		return;
 	}
 	list_del(&wrap->list);
-	spin_unlock(&info->lock);
 
 	if (urb->status) {
 		dbg("%s - nonzero read bulk status received: %d", __FUNCTION__, urb->status);
-		spin_lock(&info->lock);
 		list_add(&wrap->list, &info->rx_urbs_free);
 		spin_unlock(&info->lock);
 		return;
 	}
 
-	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, urb->actual_length, data);
-
-	spin_lock(&info->lock);
 	list_add_tail(&wrap->list, &info->rx_urb_q);
 	if (info->flags & THROTTLED) {
 		info->flags |= ACTUALLY_THROTTLED;
@@ -1060,6 +1081,7 @@
 		return;
 	}
 	spin_unlock(&info->lock);
+	usb_serial_debug_data(debug, &port->dev, __FUNCTION__, urb->actual_length, data);
 
 	schedule_work(&info->rx_work);
 }
@@ -1070,10 +1092,12 @@
 	struct usb_serial_port *port = (struct usb_serial_port *)urb->context;
 	struct whiteheat_private *info = usb_get_serial_port_data(port);
 	struct whiteheat_urb_wrap *wrap;
+	int status;
 
 	dbg("%s - port %d", __FUNCTION__, port->number);
 
 	spin_lock(&info->lock);
+	status = urb->status;
 	wrap = urb_to_wrap(urb, &info->tx_urbs_submitted);
 	if (!wrap) {
 		spin_unlock(&info->lock);
@@ -1083,8 +1107,8 @@
 	list_move(&wrap->list, &info->tx_urbs_free);
 	spin_unlock(&info->lock);
 
-	if (urb->status) {
-		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, urb->status);
+	if (status) {
+		dbg("%s - nonzero write bulk status received: %d", __FUNCTION__, status);
 		return;
 	}
 
@@ -1102,13 +1126,12 @@
 	struct whiteheat_private *info;
 	__u8 *transfer_buffer;
 	int retval = 0;
-	unsigned long flags;
 
 	dbg("%s - command %d", __FUNCTION__, command);
 
 	command_port = port->serial->port[COMMAND_PORT];
 	command_info = usb_get_serial_port_data(command_port);
-	spin_lock_irqsave(&command_info->lock, flags);
+	mutex_lock(&command_info->mutex);
 	command_info->command_finished = FALSE;
 	
 	transfer_buffer = (__u8 *)command_port->write_urb->transfer_buffer;
@@ -1116,29 +1139,28 @@
 	memcpy (&transfer_buffer[1], data, datasize);
 	command_port->write_urb->transfer_buffer_length = datasize + 1;
 	command_port->write_urb->dev = port->serial->dev;
-	retval = usb_submit_urb (command_port->write_urb, GFP_KERNEL);
+	retval = usb_submit_urb(command_port->write_urb, GFP_NOIO);
 	if (retval) {
 		dbg("%s - submit urb failed", __FUNCTION__);
 		goto exit;
 	}
-	spin_unlock_irqrestore(&command_info->lock, flags);
 
 	/* wait for the command to complete */
 	wait_event_interruptible_timeout(command_info->wait_command, 
 		(command_info->command_finished != FALSE), COMMAND_TIMEOUT);
 
-	spin_lock_irqsave(&command_info->lock, flags);
 
+	spin_lock_irq(&command_info->buflock);
 	if (command_info->command_finished == FALSE) {
 		dbg("%s - command timed out.", __FUNCTION__);
 		retval = -ETIMEDOUT;
-		goto exit;
+		goto exit2;
 	}
 
 	if (command_info->command_finished == WHITEHEAT_CMD_FAILURE) {
 		dbg("%s - command failed.", __FUNCTION__);
 		retval = -EIO;
-		goto exit;
+		goto exit2;
 	}
 
 	if (command_info->command_finished == WHITEHEAT_CMD_COMPLETE) {
@@ -1150,9 +1172,10 @@
 				break;
 		}
 	}
-
+exit2:
+	spin_unlock_irq(&command_info->buflock);
 exit:
-	spin_unlock_irqrestore(&command_info->lock, flags);
+	mutex_unlock(&command_info->mutex);
 	return retval;
 }
 
@@ -1305,12 +1328,11 @@
 {
 	struct usb_serial_port *command_port;
 	struct whiteheat_command_private *command_info;
-	unsigned long flags;
 	int retval = 0;
 	
 	command_port = serial->port[COMMAND_PORT];
 	command_info = usb_get_serial_port_data(command_port);
-	spin_lock_irqsave(&command_info->lock, flags);
+	mutex_lock(&command_info->mutex);
 	if (!command_info->port_running) {
 		/* Work around HCD bugs */
 		usb_clear_halt(serial->dev, command_port->read_urb->pipe);
@@ -1325,7 +1347,7 @@
 	command_info->port_running++;
 
 exit:
-	spin_unlock_irqrestore(&command_info->lock, flags);
+	mutex_unlock(&command_info->mutex);
 	return retval;
 }
 
@@ -1334,15 +1356,14 @@
 {
 	struct usb_serial_port *command_port;
 	struct whiteheat_command_private *command_info;
-	unsigned long flags;
 
 	command_port = serial->port[COMMAND_PORT];
 	command_info = usb_get_serial_port_data(command_port);
-	spin_lock_irqsave(&command_info->lock, flags);
+	mutex_lock(&command_info->mutex);
 	command_info->port_running--;
 	if (!command_info->port_running)
 		usb_kill_urb(command_port->read_urb);
-	spin_unlock_irqrestore(&command_info->lock, flags);
+	mutex_unlock(&command_info->mutex);
 }
 
 
@@ -1363,15 +1384,22 @@
 		wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
 		urb = wrap->urb;
 		urb->dev = port->serial->dev;
-		retval = usb_submit_urb(urb, GFP_KERNEL);
+		retval = usb_submit_urb(urb, GFP_ATOMIC);
 		if (retval) {
+			info->shutting_down = 1;
+			smp_wmb();
 			list_add(tmp, &info->rx_urbs_free);
 			list_for_each_safe(tmp, tmp2, &info->rx_urbs_submitted) {
 				wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
 				urb = wrap->urb;
+				spin_unlock_irqrestore(&info->lock, flags);
 				usb_kill_urb(urb);
-				list_move(tmp, &info->rx_urbs_free);
+				spin_lock_irqsave(&info->lock, flags);
 			}
+			list_for_each_safe(tmp, tmp2, &info->rx_urbs_submitted)
+				list_move(tmp, &info->rx_urbs_free);
+			smp_wmb();
+			info->shutting_down = 0;
 			break;
 		}
 		list_add(tmp, &info->rx_urbs_submitted);
@@ -1426,7 +1454,6 @@
 
 	list_for_each_safe(tmp, tmp2, &info->rx_urb_q) {
 		list_del(tmp);
-		spin_unlock_irqrestore(&info->lock, flags);
 
 		wrap = list_entry(tmp, struct whiteheat_urb_wrap, list);
 		urb = wrap->urb;
@@ -1435,9 +1462,7 @@
 			int len = tty_buffer_request_room(tty, urb->actual_length);
 			/* This stuff can go away now I suspect */
 			if (unlikely(len < urb->actual_length)) {
-				spin_lock_irqsave(&info->lock, flags);
 				list_add(tmp, &info->rx_urb_q);
-				spin_unlock_irqrestore(&info->lock, flags);
 				tty_flip_buffer_push(tty);
 				schedule_work(&info->rx_work);
 				return;
@@ -1447,15 +1472,16 @@
 		}
 
 		urb->dev = port->serial->dev;
-		result = usb_submit_urb(urb, GFP_ATOMIC);
+		if (!info->shutting_down)
+			result = usb_submit_urb(urb, GFP_ATOMIC);
+		else
+			result = -EBUSY;
 		if (result) {
 			err("%s - failed resubmitting read urb, error %d", __FUNCTION__, result);
-			spin_lock_irqsave(&info->lock, flags);
 			list_add(tmp, &info->rx_urbs_free);
 			continue;
 		}
 
-		spin_lock_irqsave(&info->lock, flags);
 		list_add(tmp, &info->rx_urbs_submitted);
 	}
 	spin_unlock_irqrestore(&info->lock, flags);

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel