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.
> 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()...
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'm amazed this works... Better not strain it too much.
Attached are changes I propose on top of the patch. Alan, your call.
-- Pete
diff -urp -X dontdiff linux-2.6.16-rc1-hid-stern/drivers/usb/input/hid-core.c
linux-2.6.16-rc1-hid-stern-f1/drivers/usb/input/hid-core.c
--- linux-2.6.16-rc1-hid-stern/drivers/usb/input/hid-core.c 2006-01-24
16:43:46.000000000 -0800
+++ linux-2.6.16-rc1-hid-stern-f1/drivers/usb/input/hid-core.c 2006-01-24
17:18:13.000000000 -0800
@@ -926,11 +926,12 @@ static int hid_start_in(struct hid_devic
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)) {
+ spin_unlock_irqrestore(&hid->inlock, flags);
rc = usb_submit_urb(hid->urbin, mem_flags);
if (rc != 0)
clear_bit(HID_IN_RUNNING, &hid->iofl);
- }
- spin_unlock_irqrestore(&hid->inlock, flags);
+ } else
+ spin_unlock_irqrestore(&hid->inlock, flags);
return rc;
}
@@ -957,15 +958,13 @@ static void hid_reset(void *_hid)
if (rc_lock)
usb_unlock_device(hid->dev);
}
- kfree(hid->reset_work);
clear_bit(HID_RESET_PENDING, &hid->iofl);
if (rc == 0) {
hid->retry_delay = 0;
if (hid_start_in(hid, GFP_KERNEL))
hid_io_error(hid);
- } else if (rc != -ENODEV && rc != -EHOSTUNREACH &&
- rc != -EINTR)
+ } 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,
@@ -994,20 +993,13 @@ static void hid_io_error(struct hid_devi
/* Retries failed, so do a port reset */
if (!test_and_set_bit(HID_RESET_PENDING, &hid->iofl)) {
- hid->reset_work = kmalloc(sizeof(struct work_struct),
- GFP_ATOMIC);
- if (hid->reset_work) {
- INIT_WORK(hid->reset_work, hid_reset, hid);
- if (schedule_work(hid->reset_work) != 0)
- goto done;
- kfree(hid->reset_work);
- }
+ 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));
+ mod_timer(&hid->io_retry, jiffies + msecs_to_jiffies(hid->retry_delay));
done:
spin_unlock_irqrestore(&hid->inlock, flags);
}
@@ -1378,8 +1370,8 @@ static int hid_get_class_descriptor(stru
int hid_open(struct hid_device *hid)
{
++hid->open;
- if (hid_start_in(hid, GFP_KERNEL) != 0)
- return -EIO;
+ if (hid_start_in(hid, GFP_KERNEL))
+ hid_io_error(hid);
return 0;
}
@@ -1892,6 +1884,7 @@ 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);
diff -urp -X dontdiff linux-2.6.16-rc1-hid-stern/drivers/usb/input/hid.h
linux-2.6.16-rc1-hid-stern-f1/drivers/usb/input/hid.h
--- linux-2.6.16-rc1-hid-stern/drivers/usb/input/hid.h 2006-01-24
16:43:46.000000000 -0800
+++ linux-2.6.16-rc1-hid-stern-f1/drivers/usb/input/hid.h 2006-01-24
17:12:02.000000000 -0800
@@ -400,7 +400,7 @@ struct hid_device {
/* device repo
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 */
+ struct work_struct reset_work; /* Task
context for resets */
unsigned int bufsize; /* URB
buffer size */
-------------------------------------------------------
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
