Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux