On Sat, 23 Aug 2014, vichy wrote:
> 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.
Yes. In my patch, the clear-halt handler will turn on the
HID_RESET_PENDING bit if something goes wrong. In that case we want to
do both things.
> 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
> }
No special reason. We probably never thought that both flags would be
set.
> 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.
No, the clear-halt part calls hid_start_in when everything works. It
doesn't call hid_io_error, because hid_start_in turns on the
HID_IN_RUNNING flag.
> Shall we need to call "hid_io_error(hid);" in the
> end if "clear halt" or "reset device" success?
In the clear-halt case, we don't have to because we call hid_start_in.
In the reset device case, we don't have to because hid_post_reset calls
hid_start_in if there are no errors.
> c. why we chose to use usb_queue_reset_device instead of usb_reset_device()?
I originally tried using usb_reset_device, and it caused a deadlock:
Unplug the HID device.
I/O error occurs. hid_io_error schedules reset_work.
The reset_work callback routine is hid_reset. It calls
usb_reset_device.
The reset fails because the device is gone. At the end of the
reset, hid_post_reset is called.
hid_post_reset returns 1 because hid_get_class_descriptor
fails.
Because the post_reset routine failed, usb_reset_device calls
usb_unbind_and_rebind_marked_interfaces.
That routine indirectly calls usbhid_disconnect.
usbhid_disconnect calls usbhid_close by way of
hid_destroy_device.
usbhid_close calls hid_cancel_delayed_stuff.
hid_cancel_delayed_stuff calls cancel_work_sync for reset_work.
So the reset_work routine tries to cancel itself synchronously while it
is running. usb_queue_reset_device was carefully written to avoid such
deadlocks.
> 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?
No. usb_queue_reset_device takes care of the locking.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
