On Tue, 24 Jan 2006, Pete Zaitcev wrote:
> On Thu, 12 Jan 2006 17:19:02 -0500 (EST), Alan Stern <[EMAIL PROTECTED]>
> wrote:
>
> > There was a mistake in my patch, an == that should have been !=.
>
> You are not the first and not the last to be trapped by a function
> returning a boolean instead of an error code. I approach this problem
> with two things. First, I use != and == for error codes, and use
> direct tests for error codes (so you'll never catch me writing
> !(p = kmalloc(len, gfp)) ). Second, I do not add new functions
> returning booleans in kernel code.
It would be great to have a system that always worked. The problems are:
When routines return a boolean, sometimes the boolean is an
error flag and sometimes it's a success flag. (Compare
down_trylock and down_read_trylock.)
Some routines (like kmalloc) return data values, with 0 or
NULL to indicate an error. Others return error codes, with
0 to represent success.
So the symbol you use for the test doesn't really help much; it's still
necessary to know the meaning of the return value.
> > Here's the updated patch.
>
> Looks good to me. I would not allocate the work struct, seems too much
> trouble for an equivalent of seven pointers in size. Also, it is wrong
> to submit URBs with GFP_KERNEL while holding a spinlock. I'm surprised
> it didn't keel over with might_sleep()...
Oops, I missed that. Just goes to show that the USB stack and HCDs don't
really use the mem_flags argument very much...
> The hid_open is an oddball. The hiddev does not check its result, so
> we should never return an error. Maybe it should be fixed elsewhere,
> but for the purposes of this patch it better be made to return zero.
> The whole system is weird. Observe how the disconnect causes forceful
> closes.
I don't understand what you mean here. How does a disconnect cause a
forceful close?
> I'm amazed this works... Better not strain it too much.
>
> Attached are changes I propose on top of the patch. Alan, your call.
I'll take your advice on the work struct and hid_open. I don't like
moving the usb_submit_urb outside the spinlock; there's a potential race.
Easier just to use GFP_ATOMIC always.
Here's the next version of the patch. If you give the okay I'll submit
it to Greg. There have already been reports from two users that the
current version helps (and the logs show that the reset routine was
getting called) -- although I suspect they'd be better off replacing their
hardware.
Alan Stern
Index: usb-2.6/drivers/usb/input/hid-core.c
===================================================================
--- usb-2.6.orig/drivers/usb/input/hid-core.c
+++ usb-2.6/drivers/usb/input/hid-core.c
@@ -903,6 +903,99 @@ static int hid_input_report(int type, st
}
/*
+ * Input submission and I/O error handler.
+ */
+
+static void hid_io_error(struct hid_device *hid);
+
+/* Start up the input URB */
+static int hid_start_in(struct hid_device *hid)
+{
+ unsigned long flags;
+ int rc = 0;
+
+ spin_lock_irqsave(&hid->inlock, flags);
+ if (hid->open > 0 && !test_bit(HID_SUSPENDED, &hid->iofl) &&
+ !test_and_set_bit(HID_IN_RUNNING, &hid->iofl)) {
+ rc = usb_submit_urb(hid->urbin, GFP_ATOMIC);
+ if (rc != 0)
+ clear_bit(HID_IN_RUNNING, &hid->iofl);
+ }
+ spin_unlock_irqrestore(&hid->inlock, flags);
+ return rc;
+}
+
+/* I/O retry timer routine */
+static void hid_retry_timeout(unsigned long _hid)
+{
+ struct hid_device *hid = (struct hid_device *) _hid;
+
+ dev_dbg(&hid->intf->dev, "retrying intr urb\n");
+ if (hid_start_in(hid))
+ hid_io_error(hid);
+}
+
+/* Workqueue routine to reset the device */
+static void hid_reset(void *_hid)
+{
+ struct hid_device *hid = (struct hid_device *) _hid;
+ int rc_lock, rc;
+
+ dev_dbg(&hid->intf->dev, "resetting device\n");
+ rc = rc_lock = usb_lock_device_for_reset(hid->dev, hid->intf);
+ if (rc_lock >= 0) {
+ rc = usb_reset_device(hid->dev);
+ if (rc_lock)
+ usb_unlock_device(hid->dev);
+ }
+ clear_bit(HID_RESET_PENDING, &hid->iofl);
+
+ if (rc == 0) {
+ hid->retry_delay = 0;
+ if (hid_start_in(hid))
+ hid_io_error(hid);
+ } else if (!(rc == -ENODEV || rc == -EHOSTUNREACH || rc == -EINTR))
+ err("can't reset device, %s-%s/input%d, status %d",
+ hid->dev->bus->bus_name,
+ hid->dev->devpath,
+ hid->ifnum, rc);
+}
+
+/* Main I/O error handler */
+static void hid_io_error(struct hid_device *hid)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&hid->inlock, flags);
+
+ /* Stop when disconnected */
+ if (usb_get_intfdata(hid->intf) == NULL)
+ goto done;
+
+ /* When an error occurs, retry at increasing intervals */
+ if (hid->retry_delay == 0) {
+ hid->retry_delay = 13; /* Then 26, 52, 104, 104, ... */
+ hid->stop_retry = jiffies + msecs_to_jiffies(1000);
+ } else if (hid->retry_delay < 100)
+ hid->retry_delay *= 2;
+
+ if (time_after(jiffies, hid->stop_retry)) {
+
+ /* Retries failed, so do a port reset */
+ if (!test_and_set_bit(HID_RESET_PENDING, &hid->iofl)) {
+ if (schedule_work(&hid->reset_work))
+ goto done;
+ clear_bit(HID_RESET_PENDING, &hid->iofl);
+ }
+ }
+
+ mod_timer(&hid->io_retry,
+ jiffies + msecs_to_jiffies(hid->retry_delay));
+done:
+ spin_unlock_irqrestore(&hid->inlock, flags);
+}
+
+/*
* Input interrupt completion handler.
*/
@@ -913,25 +1006,35 @@ static void hid_irq_in(struct urb *urb,
switch (urb->status) {
case 0: /* success */
+ hid->retry_delay = 0;
hid_input_report(HID_INPUT_REPORT, urb, 1, regs);
break;
case -ECONNRESET: /* unlink */
case -ENOENT:
- case -EPERM:
case -ESHUTDOWN: /* unplug */
- case -EILSEQ: /* unplug timeout on uhci */
+ clear_bit(HID_IN_RUNNING, &hid->iofl);
return;
+ case -EILSEQ: /* protocol error or unplug */
+ case -EPROTO: /* protocol error or unplug */
case -ETIMEDOUT: /* NAK */
- break;
+ clear_bit(HID_IN_RUNNING, &hid->iofl);
+ hid_io_error(hid);
+ return;
default: /* error */
warn("input irq status %d received", urb->status);
}
status = usb_submit_urb(urb, SLAB_ATOMIC);
- if (status)
- err("can't resubmit intr, %s-%s/input%d, status %d",
- hid->dev->bus->bus_name, hid->dev->devpath,
- hid->ifnum, status);
+ if (status) {
+ clear_bit(HID_IN_RUNNING, &hid->iofl);
+ if (status != -EPERM) {
+ err("can't resubmit intr, %s-%s/input%d, status %d",
+ hid->dev->bus->bus_name,
+ hid->dev->devpath,
+ hid->ifnum, status);
+ hid_io_error(hid);
+ }
+ }
}
/*
@@ -1093,8 +1196,9 @@ static void hid_irq_out(struct urb *urb,
case 0: /* success */
break;
case -ESHUTDOWN: /* unplug */
- case -EILSEQ: /* unplug timeout on uhci */
unplug = 1;
+ case -EILSEQ: /* protocol error or unplug */
+ case -EPROTO: /* protocol error or unplug */
case -ECONNRESET: /* unlink */
case -ENOENT:
break;
@@ -1141,8 +1245,9 @@ static void hid_ctrl(struct urb *urb, st
hid_input_report(hid->ctrl[hid->ctrltail].report->type, urb, 0, regs);
break;
case -ESHUTDOWN: /* unplug */
- case -EILSEQ: /* unplug timectrl on uhci */
unplug = 1;
+ case -EILSEQ: /* protocol error or unplug */
+ case -EPROTO: /* protocol error or unplug */
case -ECONNRESET: /* unlink */
case -ENOENT:
case -EPIPE: /* report not available */
@@ -1255,14 +1360,9 @@ static int hid_get_class_descriptor(stru
int hid_open(struct hid_device *hid)
{
- if (hid->open++)
- return 0;
-
- hid->urbin->dev = hid->dev;
-
- if (usb_submit_urb(hid->urbin, GFP_KERNEL))
- return -EIO;
-
+ ++hid->open;
+ if (hid_start_in(hid))
+ hid_io_error(hid);
return 0;
}
@@ -1775,6 +1875,10 @@ static struct hid_device *usb_hid_config
init_waitqueue_head(&hid->wait);
+ INIT_WORK(&hid->reset_work, hid_reset, hid);
+ setup_timer(&hid->io_retry, hid_retry_timeout, (unsigned long) hid);
+
+ spin_lock_init(&hid->inlock);
spin_lock_init(&hid->outlock);
spin_lock_init(&hid->ctrllock);
@@ -1846,11 +1950,16 @@ static void hid_disconnect(struct usb_in
if (!hid)
return;
+ spin_lock_irq(&hid->inlock); /* Sync with error handler */
usb_set_intfdata(intf, NULL);
+ spin_unlock_irq(&hid->inlock);
usb_kill_urb(hid->urbin);
usb_kill_urb(hid->urbout);
usb_kill_urb(hid->urbctrl);
+ del_timer_sync(&hid->io_retry);
+ flush_scheduled_work();
+
if (hid->claimed & HID_CLAIMED_INPUT)
hidinput_disconnect(hid);
if (hid->claimed & HID_CLAIMED_HIDDEV)
@@ -1925,6 +2034,10 @@ static int hid_suspend(struct usb_interf
{
struct hid_device *hid = usb_get_intfdata (intf);
+ spin_lock_irq(&hid->inlock); /* Sync with error handler */
+ set_bit(HID_SUSPENDED, &hid->iofl);
+ spin_unlock_irq(&hid->inlock);
+ del_timer(&hid->io_retry);
usb_kill_urb(hid->urbin);
dev_dbg(&intf->dev, "suspend\n");
return 0;
@@ -1935,10 +2048,8 @@ static int hid_resume(struct usb_interfa
struct hid_device *hid = usb_get_intfdata (intf);
int status;
- if (hid->open)
- status = usb_submit_urb(hid->urbin, GFP_NOIO);
- else
- status = 0;
+ clear_bit(HID_SUSPENDED, &hid->iofl);
+ status = hid_start_in(hid);
dev_dbg(&intf->dev, "resume status %d\n", status);
return status;
}
Index: usb-2.6/drivers/usb/input/hid.h
===================================================================
--- usb-2.6.orig/drivers/usb/input/hid.h
+++ usb-2.6/drivers/usb/input/hid.h
@@ -31,6 +31,8 @@
#include <linux/types.h>
#include <linux/slab.h>
#include <linux/list.h>
+#include <linux/timer.h>
+#include <linux/workqueue.h>
/*
* USB HID (Human Interface Device) interface class code
@@ -370,6 +372,9 @@ struct hid_control_fifo {
#define HID_CTRL_RUNNING 1
#define HID_OUT_RUNNING 2
+#define HID_IN_RUNNING 3
+#define HID_RESET_PENDING 4
+#define HID_SUSPENDED 5
struct hid_input {
struct list_head list;
@@ -393,12 +398,17 @@ struct hid_device {
/* device repo
int ifnum; /* USB
interface number */
unsigned long iofl; /* I/O
flags (CTRL_RUNNING, OUT_RUNNING) */
+ struct timer_list io_retry; /*
Retry timer */
+ unsigned long stop_retry; /* Time
to give up, in jiffies */
+ unsigned int retry_delay; /*
Delay length in ms */
+ struct work_struct reset_work; /* Task
context for resets */
unsigned int bufsize; /* URB
buffer size */
struct urb *urbin; /*
Input URB */
char *inbuf; /*
Input buffer */
dma_addr_t inbuf_dma; /*
Input buffer dma */
+ spinlock_t inlock; /*
Input fifo spinlock */
struct urb *urbctrl; /*
Control URB */
struct usb_ctrlrequest *cr; /*
Control request struct */
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
