FC1 USB printer (110307)

Pete Zaitcev zaitcev at redhat.com
Tue Dec 16 21:40:59 UTC 2003


This is taken by Greg and is "obviously correct", and it was on
my 2.4 TODO list forever, but the bug requestor is not helping
too much... Your call. Please let me know if you accept or reject
so that I can advance the bug state into MODIFIED or WONTFIX.

-- Pete

--- linux-2.4.23-rc1/drivers/usb/printer.c	2002-11-28 15:53:14.000000000 -0800
+++ linux-2.4.23-rc1-nip/drivers/usb/printer.c	2003-11-19 17:11:37.000000000 -0800
@@ -22,8 +22,11 @@
  *	v0.7 - fixed bulk-IN read and poll (David Paschal)
  *	v0.8 - add devfs support
  *	v0.9 - fix unplug-while-open paths
- *	v0.10 - add proto_bias option (Pete Zaitcev)
- *	v0.11 - add hpoj.sourceforge.net ioctls (David Paschal)
+ *	v0.10- remove sleep_on, fix error on oom (oliver at neukum.org)
+ *	v0.11 - add proto_bias option (Pete Zaitcev)
+ *	v0.12 - add hpoj.sourceforge.net ioctls (David Paschal)
+ *	v0.13 - alloc space for statusbuf (<status> not on stack);
+ *		use usb_buffer_alloc() for read buf & write buf;
  */
 
 /*
@@ -58,12 +61,12 @@
 /*
  * Version Information
  */
-#define DRIVER_VERSION "v0.11"
+#define DRIVER_VERSION "v0.13"
 #define DRIVER_AUTHOR "Michael Gee, Pavel Machek, Vojtech Pavlik, Randy Dunlap, Pete Zaitcev, David Paschal"
 #define DRIVER_DESC "USB Printer Device Class driver"
 
 #define USBLP_BUF_SIZE		8192
-#define DEVICE_ID_SIZE		1024
+#define USBLP_DEVICE_ID_SIZE	1024
 
 /* ioctls: */
 #define LPGETSTATUS		0x060b		/* same as in drivers/char/lp.c */
@@ -115,12 +118,20 @@
 #define USBLP_LAST_PROTOCOL	3
 #define USBLP_MAX_PROTOCOLS	(USBLP_LAST_PROTOCOL+1)
 
+/*
+ * some arbitrary status buffer size;
+ * need a status buffer that is allocated via kmalloc(), not on stack
+ */
+#define STATUS_BUF_SIZE		8
+
 struct usblp {
 	struct usb_device 	*dev;			/* USB device */
 	devfs_handle_t		devfs;			/* devfs device */
 	struct semaphore	sem;			/* locks this struct, especially "dev" */
-	char			*buf;		/* writeurb.transfer_buffer */
-	struct urb		readurb, writeurb;	/* The urbs */
+	char			*writebuf;		/* write transfer_buffer */
+	char			*readbuf;		/* read transfer_buffer */
+	char			*statusbuf;		/* status transfer_buffer */
+	struct urb		*readurb, *writeurb;	/* The urbs */
 	wait_queue_head_t	wait;			/* Zzzzz ... */
 	int			readcount;		/* Counter for reads */
 	int			ifnum;			/* Interface number */
@@ -133,8 +144,11 @@
 	}			protocol[USBLP_MAX_PROTOCOLS];
 	int			current_protocol;
 	int			minor;			/* minor number of device */
+	int			wcomplete;		/* writing is completed */
+	int			rcomplete;		/* reading is completed */
 	unsigned int		quirks;			/* quirks flags */
 	unsigned char		used;			/* True if open */
+	unsigned char		present;		/* True if not disconnected */
 	unsigned char		bidir;			/* interface is bidirectional */
 	unsigned char		*device_id_string;	/* IEEE 1284 DEVICE ID string (ptr) */
 							/* first 2 bytes are (big-endian) length */
@@ -146,8 +160,11 @@
 
 	dbg("usblp=0x%p", usblp);
 	dbg("dev=0x%p", usblp->dev);
-	dbg("devfs=0x%p", usblp->devfs);
-	dbg("buf=0x%p", usblp->buf);
+	dbg("present=%d", usblp->present);
+	dbg("readbuf=0x%p", usblp->readbuf);
+	dbg("writebuf=0x%p", usblp->writebuf);
+	dbg("readurb=0x%p", usblp->readurb);
+	dbg("writeurb=0x%p", usblp->writeurb);
 	dbg("readcount=%d", usblp->readcount);
 	dbg("ifnum=%d", usblp->ifnum);
     for (p = USBLP_FIRST_PROTOCOL; p <= USBLP_LAST_PROTOCOL; p++) {
@@ -157,6 +174,8 @@
     }
 	dbg("current_protocol=%d", usblp->current_protocol);
 	dbg("minor=%d", usblp->minor);
+	dbg("wcomplete=%d", usblp->wcomplete);
+	dbg("rcomplete=%d", usblp->rcomplete);
 	dbg("quirks=%d", usblp->quirks);
 	dbg("used=%d", usblp->used);
 	dbg("bidir=%d", usblp->bidir);
@@ -239,17 +258,31 @@
  * URB callback.
  */
 
-static void usblp_bulk(struct urb *urb)
+static void usblp_bulk_read(struct urb *urb)
 {
 	struct usblp *usblp = urb->context;
 
-	if (!usblp || !usblp->dev || !usblp->used)
+	if (!usblp || !usblp->dev || !usblp->used || !usblp->present)
 		return;
 
-	if (urb->status)
+	if (unlikely(urb->status))
 		warn("usblp%d: nonzero read/write bulk status received: %d",
 			usblp->minor, urb->status);
+	usblp->rcomplete = 1;
+	wake_up_interruptible(&usblp->wait);
+}
+
+static void usblp_bulk_write(struct urb *urb)
+{
+	struct usblp *usblp = urb->context;
+
+	if (!usblp || !usblp->dev || !usblp->used || !usblp->present)
+		return;
 
+	if (unlikely(urb->status))
+		warn("usblp%d: nonzero read/write bulk status received: %d",
+			usblp->minor, urb->status);
+	usblp->wcomplete = 1;
 	wake_up_interruptible(&usblp->wait);
 }
 
@@ -257,25 +290,28 @@
  * Get and print printer errors.
  */
 
-static char *usblp_messages[] = { "ok", "out of paper", "off-line", "unknown error" };
+static char *usblp_messages[] = { "ok", "out of paper", "off-line", "on fire" };
 
 static int usblp_check_status(struct usblp *usblp, int err)
 {
 	unsigned char status, newerr = 0;
 	int error;
 
-	error = usblp_read_status (usblp, &status);
+	error = usblp_read_status (usblp, usblp->statusbuf);
 	if (error < 0) {
 		err("usblp%d: error %d reading printer status",
 			usblp->minor, error);
 		return 0;
 	}
 
-	if (~status & LP_PERRORP) {
+	status = *usblp->statusbuf;
+
+	if (~status & LP_PERRORP)
 		newerr = 3;
-		if (status & LP_POUTPA) newerr = 1;
-		if (~status & LP_PSELECD) newerr = 2;
-	}
+	if (status & LP_POUTPA)
+		newerr = 1;
+	if (~status & LP_PSELECD)
+		newerr = 2;
 
 	if (newerr != err)
 		info("usblp%d: %s", usblp->minor, usblp_messages[newerr]);
@@ -300,7 +336,7 @@
 	usblp  = usblp_table[minor];
 
 	retval = -ENODEV;
-	if (!usblp || !usblp->dev)
+	if (!usblp || !usblp->dev || !usblp->present)
 		goto out;
 
 	retval = -EBUSY;
@@ -324,13 +360,14 @@
 	usblp->used = 1;
 	file->private_data = usblp;
 
-	usblp->writeurb.transfer_buffer_length = 0;
-	usblp->writeurb.status = 0;
+	usblp->writeurb->transfer_buffer_length = 0;
+	usblp->wcomplete = 1; /* we begin writeable */
+	usblp->rcomplete = 0;
 
 	if (usblp->bidir) {
 		usblp->readcount = 0;
-		usblp->readurb.dev = usblp->dev;
-		if (usb_submit_urb(&usblp->readurb) < 0) {
+		usblp->readurb->dev = usblp->dev;
+		if (usb_submit_urb(usblp->readurb) < 0) {
 			retval = -EIO;
 			usblp->used = 0;
 			file->private_data = NULL;
@@ -347,16 +384,20 @@
 	usblp_table [usblp->minor] = NULL;
 	info("usblp%d: removed", usblp->minor);
 
-	kfree (usblp->writeurb.transfer_buffer);
+	kfree (usblp->writebuf);
+	kfree (usblp->readbuf);
 	kfree (usblp->device_id_string);
+	kfree (usblp->statusbuf);
+	usb_free_urb(usblp->writeurb);
+	usb_free_urb(usblp->readurb);
 	kfree (usblp);
 }
 
 static void usblp_unlink_urbs(struct usblp *usblp)
 {
-	usb_unlink_urb(&usblp->writeurb);
+	usb_unlink_urb(usblp->writeurb);
 	if (usblp->bidir)
-		usb_unlink_urb(&usblp->readurb);
+		usb_unlink_urb(usblp->readurb);
 }
 
 static int usblp_release(struct inode *inode, struct file *file)
@@ -366,7 +407,7 @@
 	down (&usblp->sem);
 	lock_kernel();
 	usblp->used = 0;
-	if (usblp->dev) {
+	if (usblp->present) {
 		usblp_unlink_urbs(usblp);
 		up(&usblp->sem);
 	} else 		/* finish cleanup from disconnect */
@@ -380,21 +421,21 @@
 {
 	struct usblp *usblp = file->private_data;
 	poll_wait(file, &usblp->wait, wait);
- 	return ((!usblp->bidir || usblp->readurb.status  == -EINPROGRESS) ? 0 : POLLIN  | POLLRDNORM)
- 			       | (usblp->writeurb.status == -EINPROGRESS  ? 0 : POLLOUT | POLLWRNORM);
+ 	return ((!usblp->bidir || !usblp->rcomplete) ? 0 : POLLIN  | POLLRDNORM)
+ 			       | (!usblp->wcomplete ? 0 : POLLOUT | POLLWRNORM);
 }
 
 static int usblp_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct usblp *usblp = file->private_data;
 	int length, err, i;
-	unsigned char lpstatus, newChannel;
+	unsigned char newChannel;
 	int status;
 	int twoints[2];
 	int retval = 0;
 
 	down (&usblp->sem);
-	if (!usblp->dev) {
+	if (!usblp->present) {
 		retval = -ENODEV;
 		goto done;
 	}
@@ -534,18 +575,18 @@
 				break;
 
 			default:
-				retval = -EINVAL;
+				retval = -ENOTTY;
 		}
 	else	/* old-style ioctl value */
 		switch (cmd) {
 
 			case LPGETSTATUS:
-				if (usblp_read_status(usblp, &lpstatus)) {
+				if (usblp_read_status(usblp, usblp->statusbuf)) {
 					err("usblp%d: failed reading printer status", usblp->minor);
 					retval = -EIO;
 					goto done;
 				}
-				status = lpstatus;
+				status = *usblp->statusbuf;
 				if (copy_to_user ((int *)arg, &status, sizeof(int)))
 					retval = -EFAULT;
 				break;
@@ -561,41 +602,48 @@
 
 static ssize_t usblp_write(struct file *file, const char *buffer, size_t count, loff_t *ppos)
 {
+	DECLARE_WAITQUEUE(wait, current);
 	struct usblp *usblp = file->private_data;
 	int timeout, err = 0;
 	size_t writecount = 0;
 
 	while (writecount < count) {
-
-		// FIXME:  only use urb->status inside completion
-		// callbacks; this way is racey...
-		if (usblp->writeurb.status == -EINPROGRESS) {
-
+		if (!usblp->wcomplete) {
+			barrier();
 			if (file->f_flags & O_NONBLOCK)
 				return -EAGAIN;
 
 			timeout = USBLP_WRITE_TIMEOUT;
-			while (timeout && usblp->writeurb.status == -EINPROGRESS) {
+			add_wait_queue(&usblp->wait, &wait);
+			while ( 1==1 ) {
 
-				if (signal_pending(current))
+				if (signal_pending(current)) {
+					remove_wait_queue(&usblp->wait, &wait);
 					return writecount ? writecount : -EINTR;
-
-				timeout = interruptible_sleep_on_timeout(&usblp->wait, timeout);
+				}
+				set_current_state(TASK_INTERRUPTIBLE);
+				if (timeout && !usblp->wcomplete) {
+					timeout = schedule_timeout(timeout);
+				} else {
+					set_current_state(TASK_RUNNING);
+					break;
+				}
 			}
+			remove_wait_queue(&usblp->wait, &wait);
 		}
 
 		down (&usblp->sem);
-		if (!usblp->dev) {
+		if (!usblp->present) {
 			up (&usblp->sem);
 			return -ENODEV;
 		}
 
-		if (usblp->writeurb.status != 0) {
+		if (usblp->writeurb->status != 0) {
 			if (usblp->quirks & USBLP_QUIRK_BIDIR) {
-				if (usblp->writeurb.status != -EINPROGRESS)
+				if (!usblp->wcomplete)
 					err("usblp%d: error %d writing to printer",
-						usblp->minor, usblp->writeurb.status);
-				err = usblp->writeurb.status;
+						usblp->minor, usblp->writeurb->status);
+				err = usblp->writeurb->status;
 			} else
 				err = usblp_check_status(usblp, err);
 			up (&usblp->sem);
@@ -607,25 +655,30 @@
 			continue;
 		}
 
-		writecount += usblp->writeurb.transfer_buffer_length;
-		usblp->writeurb.transfer_buffer_length = 0;
+		writecount += usblp->writeurb->transfer_buffer_length;
+		usblp->writeurb->transfer_buffer_length = 0;
 
 		if (writecount == count) {
 			up (&usblp->sem);
 			break;
 		}
 
-		usblp->writeurb.transfer_buffer_length = (count - writecount) < USBLP_BUF_SIZE ?
-							 (count - writecount) : USBLP_BUF_SIZE;
+		usblp->writeurb->transfer_buffer_length = (count - writecount) < USBLP_BUF_SIZE ?
+							  (count - writecount) : USBLP_BUF_SIZE;
 
-		if (copy_from_user(usblp->writeurb.transfer_buffer, buffer + writecount,
-				usblp->writeurb.transfer_buffer_length)) {
+		if (copy_from_user(usblp->writeurb->transfer_buffer, buffer + writecount,
+				usblp->writeurb->transfer_buffer_length)) {
 			up(&usblp->sem);
 			return writecount ? writecount : -EFAULT;
 		}
 
-		usblp->writeurb.dev = usblp->dev;
-		usb_submit_urb(&usblp->writeurb);
+		usblp->writeurb->dev = usblp->dev;
+		usblp->wcomplete = 0;
+		if (usb_submit_urb(usblp->writeurb)) {
+			count = -EIO;
+			up (&usblp->sem);
+			break;
+		}
 		up (&usblp->sem);
 	}
 
@@ -635,63 +688,77 @@
 static ssize_t usblp_read(struct file *file, char *buffer, size_t count, loff_t *ppos)
 {
 	struct usblp *usblp = file->private_data;
+	DECLARE_WAITQUEUE(wait, current);
 
 	if (!usblp->bidir)
 		return -EINVAL;
 
 	down (&usblp->sem);
-	if (!usblp->dev) {
+	if (!usblp->present) {
 		count = -ENODEV;
 		goto done;
 	}
 
-	if (usblp->readurb.status == -EINPROGRESS) {
+	if (!usblp->rcomplete) {
+		barrier();
 
 		if (file->f_flags & O_NONBLOCK) {
 			count = -EAGAIN;
 			goto done;
 		}
 
-		// FIXME:  only use urb->status inside completion
-		// callbacks; this way is racey...
-		while (usblp->readurb.status == -EINPROGRESS) {
+		add_wait_queue(&usblp->wait, &wait);
+		while (1==1) {
 			if (signal_pending(current)) {
 				count = -EINTR;
+				remove_wait_queue(&usblp->wait, &wait);
 				goto done;
 			}
 			up (&usblp->sem);
-			interruptible_sleep_on(&usblp->wait);
+			set_current_state(TASK_INTERRUPTIBLE);
+			if (!usblp->rcomplete) {
+				schedule();
+			} else {
+				set_current_state(TASK_RUNNING);
+				break;
+			}
 			down (&usblp->sem);
 		}
+		remove_wait_queue(&usblp->wait, &wait);
 	}
 
-	if (!usblp->dev) {
+	if (!usblp->present) {
 		count = -ENODEV;
 		goto done;
 	}
 
-	if (usblp->readurb.status) {
+	if (usblp->readurb->status) {
 		err("usblp%d: error %d reading from printer",
-			usblp->minor, usblp->readurb.status);
-		usblp->readurb.dev = usblp->dev;
+			usblp->minor, usblp->readurb->status);
+		usblp->readurb->dev = usblp->dev;
  		usblp->readcount = 0;
-		usb_submit_urb(&usblp->readurb);
+		if (usb_submit_urb(usblp->readurb) < 0)
+			dbg("error submitting urb");
 		count = -EIO;
 		goto done;
 	}
 
-	count = count < usblp->readurb.actual_length - usblp->readcount ?
-		count :	usblp->readurb.actual_length - usblp->readcount;
+	count = count < usblp->readurb->actual_length - usblp->readcount ?
+		count :	usblp->readurb->actual_length - usblp->readcount;
 
-	if (copy_to_user(buffer, usblp->readurb.transfer_buffer + usblp->readcount, count)) {
+	if (copy_to_user(buffer, usblp->readurb->transfer_buffer + usblp->readcount, count)) {
 		count = -EFAULT;
 		goto done;
 	}
 
-	if ((usblp->readcount += count) == usblp->readurb.actual_length) {
+	if ((usblp->readcount += count) == usblp->readurb->actual_length) {
 		usblp->readcount = 0;
-		usblp->readurb.dev = usblp->dev;
-		usb_submit_urb(&usblp->readurb);
+		usblp->readurb->dev = usblp->dev;
+		usblp->rcomplete = 0;
+		if (usb_submit_urb(usblp->readurb)) {
+			count = -EIO;
+			goto done;
+		}
 	}
 
 done:
@@ -766,19 +833,42 @@
 		}
 	}
 
+	usblp->writeurb = usb_alloc_urb(0);
+	if (!usblp->writeurb) {
+		err("out of memory");
+		goto abort;
+	}
+	usblp->readurb = usb_alloc_urb(0);
+	if (!usblp->readurb) {
+		err("out of memory");
+		goto abort;
+	}
+
 	/* Malloc device ID string buffer to the largest expected length,
 	 * since we can re-query it on an ioctl and a dynamic string
 	 * could change in length. */
-	if (!(usblp->device_id_string = kmalloc(DEVICE_ID_SIZE, GFP_KERNEL))) {
+	if (!(usblp->device_id_string = kmalloc(USBLP_DEVICE_ID_SIZE, GFP_KERNEL))) {
 		err("out of memory for device_id_string");
 		goto abort;
 	}
 
-	/* Malloc write/read buffers in one chunk.  We somewhat wastefully
+	usblp->writebuf = usblp->readbuf = NULL;
+	/* Malloc write & read buffers.  We somewhat wastefully
 	 * malloc both regardless of bidirectionality, because the
 	 * alternate setting can be changed later via an ioctl. */
-	if (!(usblp->buf = kmalloc(2 * USBLP_BUF_SIZE, GFP_KERNEL))) {
-		err("out of memory for buf");
+	if (!(usblp->writebuf = kmalloc(USBLP_BUF_SIZE, GFP_KERNEL))) {
+		err("out of memory for write buf");
+		goto abort;
+	}
+	if (!(usblp->readbuf = kmalloc(USBLP_BUF_SIZE, GFP_KERNEL))) {
+		err("out of memory for read buf");
+		goto abort;
+	}
+
+	/* Allocate buffer for printer status */
+	usblp->statusbuf = kmalloc(STATUS_BUF_SIZE, GFP_KERNEL);
+	if (!usblp->statusbuf) {
+		err("out of memory for statusbuf");
 		goto abort;
 	}
 
@@ -818,17 +908,26 @@
 
 	info("usblp%d: USB %sdirectional printer dev %d "
 		"if %d alt %d proto %d vid 0x%4.4X pid 0x%4.4X",
-		usblp->minor, usblp->bidir ? "Bi" : "Uni", dev->devnum, ifnum,
+		usblp->minor, usblp->bidir ? "Bi" : "Uni", dev->devnum,
+		usblp->ifnum,
 		usblp->protocol[usblp->current_protocol].alt_setting,
 		usblp->current_protocol, usblp->dev->descriptor.idVendor,
 		usblp->dev->descriptor.idProduct);
 
+	usblp->present = 1;
+
 	return usblp;
 
 abort:
 	if (usblp) {
-		if (usblp->buf) kfree(usblp->buf);
-		if (usblp->device_id_string) kfree(usblp->device_id_string);
+		if (usblp->writebuf)
+			kfree (usblp->writebuf);
+		if (usblp->readbuf)
+			kfree (usblp->readbuf);
+		kfree(usblp->statusbuf);
+		kfree(usblp->device_id_string);
+		usb_free_urb(usblp->writeurb);
+		usb_free_urb(usblp->readurb);
 		kfree(usblp);
 	}
 	return NULL;
@@ -943,19 +1042,19 @@
 		return r;
 	}
 
-	FILL_BULK_URB(&usblp->writeurb, usblp->dev,
+	usb_fill_bulk_urb(usblp->writeurb, usblp->dev,
 		usb_sndbulkpipe(usblp->dev,
-		 usblp->protocol[protocol].epwrite->bEndpointAddress),
-		usblp->buf, 0,
-		usblp_bulk, usblp);
+		  usblp->protocol[protocol].epwrite->bEndpointAddress),
+		usblp->writebuf, 0,
+		usblp_bulk_write, usblp);
 
 	usblp->bidir = (usblp->protocol[protocol].epread != 0);
 	if (usblp->bidir)
-		FILL_BULK_URB(&usblp->readurb, usblp->dev,
+		usb_fill_bulk_urb(usblp->readurb, usblp->dev,
 			usb_rcvbulkpipe(usblp->dev,
-			 usblp->protocol[protocol].epread->bEndpointAddress),
-			usblp->buf + USBLP_BUF_SIZE, USBLP_BUF_SIZE,
-			usblp_bulk, usblp);
+			  usblp->protocol[protocol].epread->bEndpointAddress),
+			usblp->readbuf, USBLP_BUF_SIZE,
+			usblp_bulk_read, usblp);
 
 	usblp->current_protocol = protocol;
 	dbg("usblp%d set protocol %d", usblp->minor, protocol);
@@ -969,7 +1068,7 @@
 {
 	int err, length;
 
-	err = usblp_get_id(usblp, 0, usblp->device_id_string, DEVICE_ID_SIZE - 1);
+	err = usblp_get_id(usblp, 0, usblp->device_id_string, USBLP_DEVICE_ID_SIZE - 1);
 	if (err < 0) {
 		dbg("usblp%d: error = %d reading IEEE-1284 Device ID string",
 			usblp->minor, err);
@@ -983,8 +1082,8 @@
 	length = (usblp->device_id_string[0] << 8) + usblp->device_id_string[1];
 	if (length < 2)
 		length = 2;
-	else if (length >= DEVICE_ID_SIZE)
-		length = DEVICE_ID_SIZE - 1;
+	else if (length >= USBLP_DEVICE_ID_SIZE)
+		length = USBLP_DEVICE_ID_SIZE - 1;
 	usblp->device_id_string[length] = '\0';
 
 	dbg("usblp%d Device ID string [len=%d]=\"%s\"",
@@ -1004,13 +1103,13 @@
 
 	down (&usblp->sem);
 	lock_kernel();
-	usblp->dev = NULL;
+	usblp->present = 0;
 
 	usblp_unlink_urbs(usblp);
 
 	if (!usblp->used)
 		usblp_cleanup (usblp);
-	else 	/* cleanup later, on close */
+	else 	/* cleanup later, on release */
 		up (&usblp->sem);
 	unlock_kernel();
 }





More information about the fedora-devel-list mailing list