David Brownell wrote:
>
...
>
> Your stack trace showed usblp_check_status() was what started this
> sequence of troubles, so I looked at it. There were some issues,
> it's got a lot of pretty early usb code:
>
> - it assumes it's OK to DMA onto the stack. kmalloc would likely
> be the best fix.
Patch for this is attached. Please apply.
> - it uses a one second timeout, while the longer USB_CTRL_GET_TIMEOUT
> (or even "5" seconds as the usb spec allows ) should likely be used.
One of us is misreading the source code or just looking at different
versions of it. In 2.5.46 (and back thru 2.5.41), usblp_ctrl_msg()
uses USBLP_WRITE_TIMEOUT, which is 5 seconds.
> - it's called after usblp_write() did an illegal (racey) thing, looking
> at urb->status outside the completion function. (does anyone know
> why usblp_write() isn't just issuing a single write for that buffer?)
How should usblp_write() check urb->status?
Can someone remind me of the rules for checking urb->status?
(and part 2:)
usblp allocates a write buffer of 8192 bytes and tries to send
that entire buffer each time...right?
> Except for maybe that last one they're unlikely to be very triggering
> your symptoms (but of course hangs shouldn't happen).
>
> I'd more suspect a bug in usb_start_wait_urb (in part since every time
> I see it I suspect it's wrong; it doesn't _look_ as right as I'd like
> and its "control/bulk msg timeout" diagnostic comes up a lot), or that
> there's a completion path that doesn't handshake right with unlinking
> (long a popular spot for races, ohci did change there in 2.5).
> -------------------------------------------------------
The holy grail message...it needs to go away (be fixed).
~Randy
--- ./drivers/usb/class/usblp.c%hang Mon Nov 4 14:30:11 2002
+++ ./drivers/usb/class/usblp.c Sat Nov 9 21:14:06 2002
@@ -1,9 +1,9 @@
/*
- * usblp.c Version 0.12
+ * usblp.c Version 0.13
*
* Copyright (c) 1999 Michael Gee <[EMAIL PROTECTED]>
* Copyright (c) 1999 Pavel Machek <[EMAIL PROTECTED]>
- * Copyright (c) 2000 Randy Dunlap <[EMAIL PROTECTED]>
+ * Copyright (c) 2000 Randy Dunlap <[EMAIL PROTECTED]>
* Copyright (c) 2000 Vojtech Pavlik <[EMAIL PROTECTED]>
# Copyright (c) 2001 Pete Zaitcev <[EMAIL PROTECTED]>
# Copyright (c) 2001 David Paschal <[EMAIL PROTECTED]>
@@ -25,6 +25,7 @@
* v0.10- remove sleep_on, fix error on oom ([EMAIL PROTECTED])
* v0.11 - add proto_bias option (Pete Zaitcev)
* v0.12 - add hpoj.sourceforge.net ioctls (David Paschal)
+ * v0.13 - kmalloc() space for statusbuf with read/write bufs;
*/
/*
@@ -59,7 +60,7 @@
/*
* Version Information
*/
-#define DRIVER_VERSION "v0.12"
+#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"
@@ -120,11 +121,18 @@
#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 */
+ char *statusbuf; /* NOT on stack */
struct urb *readurb, *writeurb; /* The urbs */
wait_queue_head_t wait; /* Zzzzz ... */
int readcount; /* Counter for reads */
@@ -289,13 +297,14 @@
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;
}
+ status = *usblp->statusbuf;
if (~status & LP_PERRORP) {
newerr = 3;
if (status & LP_POUTPA) newerr = 1;
@@ -844,10 +853,11 @@
/* Malloc write/read buffers in one chunk. 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))) {
+ if (!(usblp->buf = kmalloc(2 * USBLP_BUF_SIZE + STATUS_BUF_SIZE, GFP_KERNEL)))
+{
err("out of memory for buf");
goto abort_minor;
}
+ usblp->statusbuf = usblp->buf + 2 * USBLP_BUF_SIZE;
/* Lookup quirks for this printer. */
usblp->quirks = usblp_quirks(
