hi alan and all:
> I recently posted (but did not submit) a more comprehensive solution to
> this and other related problems. For example, HID devices typically
> run at low speed or full speed. When attached through a hub to an EHCI
> controller (very common on modern systems), unplugging the device
> causes -EPIPE errors, so the driver calls usb_clear_halt and restarts
> the interrupt pipe. Of course, the same failure occurs again, so
> there's a lengthy flurry of useless error messages.
>
> This patch changes the driver so that after usb_clear_halt fails, it
> will try to do a port reset. And if that fails, the driver will be
> unbound from the device. This avoids infinite loops.
>
> What do you think?
>
> Alan Stern
>
>
>
> Index: usb-3.16/drivers/hid/usbhid/hid-core.c
> ===================================================================
> --- usb-3.16.orig/drivers/hid/usbhid/hid-core.c
> +++ usb-3.16/drivers/hid/usbhid/hid-core.c
> @@ -116,40 +116,24 @@ static void hid_reset(struct work_struct
> struct usbhid_device *usbhid =
> container_of(work, struct usbhid_device, reset_work);
> struct hid_device *hid = usbhid->hid;
> - int rc = 0;
> + int rc;
>
> if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
> dev_dbg(&usbhid->intf->dev, "clear halt\n");
> rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
> clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
> - hid_start_in(hid);
> - }
> -
> - else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> - dev_dbg(&usbhid->intf->dev, "resetting device\n");
> - rc = usb_lock_device_for_reset(hid_to_usb_dev(hid), usbhid->intf);
> if (rc == 0) {
> - rc = usb_reset_device(hid_to_usb_dev(hid));
> - usb_unlock_device(hid_to_usb_dev(hid));
> + hid_start_in(hid);
> + } else {
> + dev_dbg(&usbhid->intf->dev,
> + "clear-halt failed: %d\n", rc);
> + set_bit(HID_RESET_PENDING, &usbhid->iofl);
> }
> - clear_bit(HID_RESET_PENDING, &usbhid->iofl);
> }
>
> - switch (rc) {
> - case 0:
> - if (!test_bit(HID_IN_RUNNING, &usbhid->iofl))
> - hid_io_error(hid);
> - break;
> - default:
> - hid_err(hid, "can't reset device, %s-%s/input%d, status %d\n",
> - hid_to_usb_dev(hid)->bus->bus_name,
> - hid_to_usb_dev(hid)->devpath,
> - usbhid->ifnum, rc);
> - /* FALLTHROUGH */
> - case -EHOSTUNREACH:
> - case -ENODEV:
> - case -EINTR:
> - break;
> + if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> + dev_dbg(&usbhid->intf->dev, "resetting device\n");
> + usb_queue_reset_device(usbhid->intf);
> }
> }
from your patch, I have some questions:
a. in Alan's version, if both HID_CLEAR_HALT and HID_RESET_PENDING are
set, hid_reset will both "clear ep halt" and "reset devcie".
But in original one, even HID_CLEAR_HALT and HID_RESET_PENDING are
both set, hid_reset only do one of them.
is there any special reason in original hid_reset to use below flow?
if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
xxxxx
} else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
xxxxxx
}
b. in original hid_reset, if "Clear halt ep" or "Rest device" success
or none of those flags raise up, it will call hid_io_error for later
resending the urb. Shall we need to call "hid_io_error(hid);" in the
end if "clear halt" or "reset device" success?
c. why we chose to use usb_queue_reset_device instead of usb_reset_device()?
d. shall we "useusb_lock_device_for_reset(hid_to_usb_dev(hid),
usbhid->intf)" or "spin_lock_irq(&usbhid->lock)" before calling
usb_queue_reset_device?
I append patch for explaining my questions.
Appreciate your kind help,
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 256b102..aa321f9 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -116,18 +116,22 @@ static void hid_reset(struct work_struct *work)
struct usbhid_device *usbhid =
container_of(work, struct usbhid_device, reset_work);
struct hid_device *hid = usbhid->hid;
- int rc = 0;
+ int rc;
if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
dev_dbg(&usbhid->intf->dev, "clear halt\n");
rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
- if (rc == 0)
+ if (rc == 0) {
hid_start_in(hid);
+ } else {
+ dev_dbg(&usbhid->intf->dev,
+ "clear-halt failed: %d\n", rc);
+ set_bit(HID_RESET_PENDING, &usbhid->iofl);
+ }
}
- else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
- dev_dbg(&usbhid->intf->dev, "resetting device\n");
+ if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
rc = usb_lock_device_for_reset(hid_to_usb_dev(hid),
usbhid->intf);
if (rc == 0) {
rc = usb_reset_device(hid_to_usb_dev(hid));
@@ -136,22 +140,8 @@ static void hid_reset(struct work_struct *work)
clear_bit(HID_RESET_PENDING, &usbhid->iofl);
}
- switch (rc) {
- case 0:
- if (!test_bit(HID_IN_RUNNING, &usbhid->iofl))
- hid_io_error(hid);
- break;
- default:
- hid_err(hid, "can't reset device, %s-%s/input%d, status %d\n",
- hid_to_usb_dev(hid)->bus->bus_name,
- hid_to_usb_dev(hid)->devpath,
- usbhid->ifnum, rc);
- /* FALLTHROUGH */
- case -EHOSTUNREACH:
- case -ENODEV:
- case -EINTR:
- break;
- }
+ if (!test_bit(HID_IN_RUNNING, &usbhid->iofl) || (rc == 0))
+ hid_io_error(hid);
}
/* Main I/O error handler */
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
