On Wed, 20 Aug 2014, David Laight wrote:
> > On Tue, 19 Aug 2014, David Laight wrote:
> >
> > > From: Alan Stern
> > >
> > > > Since the device is plugged into a hub, the hub knows about the
> > > > disconnection. But it can't inform the kernel until the host
> > > > controller polls it, which occurs at 128-ms intervals.
> > >
> > > That rather explains the delay in the error reporting.
> > > I rather hoped it would be a little quicker.
> > > I presume the kernel can't force a status poll after a URB failure?
> >
> > I suppose that could be added. It doesn't seem to be terribly
> > important. Rapidly detecting disconnects is a separate issue from too
> > much noise in the log when a disconnect occurs.
>
> Indeed...
> There isn't much point tracing URB failures if the reason is a disconnect.
> I know the logic would get contorted, but it would be more user-friendly
> to only report the disconnect - even if it isn't checked until after the
> first reset fail.
The error messages could be changed to debug messages. But maybe we
can do a better job of reducing them. Does the patch below help?
> > > The usbhid driver (at least the version I'm using ubuntu 3.11.0-26)
> > > is a bit noisy on usb unplugs.
> >
> > Yeah, there's an error message every time a reset fails (a call to
> > hid_err() in drivers/hid/usbhid/hid-core.c:hid_reset()). The resets
> > are supposed to occur at increasing intervals; I'm not sure if that's
> > working as intended. It might be preferable not to retry at all after
> > a reset fails.
>
> I think they are at increasing intervals, but still quite frequent.
No, the intervals are not increasing. And the reason is because
despite what the messages say, the system isn't trying to reset the HID
device. Instead it's trying to clear an endpoint halt.
The same subroutine in the usbhid driver is used for both clearing
halts and performing resets. If anything goes wrong, it logs the
"can't reset device" message, regardless of what it was actually
trying to do.
Unlike resets, the driver does not use increasing delays for clearing
halts. In fact, it doesn't delay at all; it tries to clear the halt as
soon as possible.
It looks like we need to be a little more careful here. When a
clear-halt fails, we should reset instead of attempting to resume
normal operations. That's what this patch tries to do.
> > It doesn't help that your device has two HID interfaces, which means
> > you get double the number of log message.
>
> Actually I got some devices plugged elsewhere.
> The smsc hub could easily have keyboard, mouse and its I2C/serial
> active (all are hid) as well as the ethernet.
> My keyboard has two input devices (NFI why, it doesn't have any unusual keys).
I think the second input device is used for some of the normal keys, or
maybe for the LEDs. Lots of keyboards do this.
> > > Possibly it has been build with one too many DEBUG options enabled.
> > > Most of the usb stack has dynamic debug enabled.
> >
> > Dynamic debugging doesn't do anything about error messages, though.
>
> Yes, they seem to have missed the trick of having a few levels of
> messages - so that annoying error messages can be disabled, or debug
> and diagnostic (normal path) messages enabled in bulk.
What do you mean? We _do_ have levels of messages -- that's the
difference between a debug message and an error message. With dynamic
debugging, debug messages can be enabled in bulk.
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);
}
}
--
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
