Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
On Fri, 2014-08-22 at 14:23 -0400, Alan Stern wrote:
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.
Why? If we reset, why bother clearing a halt? Especially as this
may mean waiting the full 5 seconds for a timeout.
is there any special reason in original hid_reset to use below flow?
if (test_bit(HID_CLEAR_HALT, usbhid-iofl)) {
x
} else if (test_bit(HID_RESET_PENDING, usbhid-iofl)) {
xx
}
No special reason. We probably never thought that both flags would be
set.
Yes. And I'd say if this ever happens HID_RESET_PENDING must have
priority and implicitly clears a halt.
Regards
Oliver
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
hi Alan: usb_unbind_and_rebind_marked_interfaces is called if config parameter is not null, it seems no matter post_reset routine fail or not. Yes, that's right. I should have said: Because the post_reset routine failed, usb_unbind_and_rebind_marked_interfaces indirectly calls usbhid_disconnect. Even post_reset routine failed, usb_unbind_and_rebind_marked_interfaces will still indirectly calls usbhid_disconnect, right? I don't remember the entire call chain. It was pretty long. hid_destroy_device calls hid_remove_device, which calls device_del, which calls lots of other things. If you really want to see all the details, put a dump_stack() call in usbhid_close and examine what it prints in the kernel log when you unplug an HID device. I found what you mentioned ^^ Thanks for your kind help, -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
hi Oliver: 2014-08-25 18:21 GMT+08:00 Oliver Neukum oneu...@suse.de: On Fri, 2014-08-22 at 14:23 -0400, Alan Stern wrote: 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. Why? If we reset, why bother clearing a halt? Especially as this may mean waiting the full 5 seconds for a timeout. I think what Alan mean is IF CLEAR HALT fail, we reset the device. That is what below In that case mean. In that case we want to do both things. BR, -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
On Mon, 25 Aug 2014, vichy wrote: hi Oliver: 2014-08-25 18:21 GMT+08:00 Oliver Neukum oneu...@suse.de: On Fri, 2014-08-22 at 14:23 -0400, Alan Stern wrote: 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. Why? If we reset, why bother clearing a halt? Especially as this may mean waiting the full 5 seconds for a timeout. I think what Alan mean is IF CLEAR HALT fail, we reset the device. That is what below In that case mean. In that case we want to do both things. Exactly. Suppose initially HID_CLEAR_HALT is set and HID_RESET_PENDING is off. If the usb_clear_halt call fails, we want to recover by performing a reset. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
On Mon, 25 Aug 2014, vichy wrote: hi Alan: usb_unbind_and_rebind_marked_interfaces is called if config parameter is not null, it seems no matter post_reset routine fail or not. Yes, that's right. I should have said: Because the post_reset routine failed, usb_unbind_and_rebind_marked_interfaces indirectly calls usbhid_disconnect. Even post_reset routine failed, usb_unbind_and_rebind_marked_interfaces will still indirectly calls usbhid_disconnect, right? If the pre_reset and post_reset routines both return 0, usb_unbind_and_rebind_marked_interfaces will _not_ call usbhid_disconnect. Otherwise it will. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
hi Alan:
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.
usb_unbind_and_rebind_marked_interfaces is called if config
parameter is not null,
it seems no matter post_reset routine fail or not.
That routine indirectly calls usbhid_disconnect.
Why that routine, usb_reset_device, will call usbhid_disconnect?
No matter port reset success or fail, usbhid_disconnect should not be
called except that did happen disconnection.
usbhid_disconnect calls usbhid_close by way of
hid_destroy_device.
below is the content of hid_destroy and hid_remove_device, but I
cannot find where usbhid_close be called by way of hid_destroy_device.
static void hid_remove_device(struct hid_device *hdev)
{
if (hdev-status HID_STAT_ADDED) {
device_del(hdev-dev);
hid_debug_unregister(hdev);
hdev-status = ~HID_STAT_ADDED;
}
kfree(hdev-dev_rdesc);
hdev-dev_rdesc = NULL;
hdev-dev_rsize = 0;
}
void hid_destroy_device(struct hid_device *hdev)
{
hid_remove_device(hdev);
put_device(hdev-dev);
}
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.
thanks for your help,
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
On Sun, 24 Aug 2014, vichy wrote:
hi Alan:
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.
usb_unbind_and_rebind_marked_interfaces is called if config
parameter is not null,
it seems no matter post_reset routine fail or not.
Yes, that's right. I should have said: Because the post_reset routine
failed, usb_unbind_and_rebind_marked_interfaces indirectly calls
usbhid_disconnect.
That routine indirectly calls usbhid_disconnect.
Why that routine, usb_reset_device, will call usbhid_disconnect?
No matter port reset success or fail, usbhid_disconnect should not be
called except that did happen disconnection.
usbhid_disconnect gets called when the driver is unbound, regardless of
whether the device was unplugged. The kernel unbinds a driver when a
device is reset but the driver isn't able to handle the reset.
usbhid_disconnect calls usbhid_close by way of
hid_destroy_device.
below is the content of hid_destroy and hid_remove_device, but I
cannot find where usbhid_close be called by way of hid_destroy_device.
static void hid_remove_device(struct hid_device *hdev)
{
if (hdev-status HID_STAT_ADDED) {
device_del(hdev-dev);
hid_debug_unregister(hdev);
hdev-status = ~HID_STAT_ADDED;
}
kfree(hdev-dev_rdesc);
hdev-dev_rdesc = NULL;
hdev-dev_rsize = 0;
}
void hid_destroy_device(struct hid_device *hdev)
{
hid_remove_device(hdev);
put_device(hdev-dev);
}
I don't remember the entire call chain. It was pretty long.
hid_destroy_device calls hid_remove_device, which calls device_del,
which calls lots of other things. If you really want to see all the
details, put a dump_stack() call in usbhid_close and examine what it
prints in the kernel log when you unplug an HID device.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
HID IR device will not response to any command send from host when it
is finishing paring and tring to reset itself.
During this period of time, usb_cleaer_halt will be fail and if hid_start_in
soon again, we has the possibility trap in infinite loop.
Signed-off-by: CheChun Kuo vichy@gmail.com
---
drivers/hid/usbhid/hid-core.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 79cf503..256b102 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work)
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);
+ if (rc == 0)
+ hid_start_in(hid);
}
else if (test_bit(HID_RESET_PENDING, usbhid-iofl)) {
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
On Fri, 22 Aug 2014, CheChun Kuo wrote: HID IR device will not response to any command send from host when it is finishing paring and tring to reset itself. During this period of time, usb_cleaer_halt will be fail and if hid_start_in soon again, we has the possibility trap in infinite loop. Signed-off-by: CheChun Kuo vichy@gmail.com --- drivers/hid/usbhid/hid-core.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 79cf503..256b102 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work) 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); + if (rc == 0) + hid_start_in(hid); } I'd need a more detailed explanation for this patch, as I don't understand neither the text in the changelog nor the patch itself. Namely: - which IR devices are showing this behavior? - where does the infinite loop come from? hid_reset() should error out properly if usb_clear_halt() fails and HID_IN_RUNNING flag is not set Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
hi Jiri: 2014-08-22 15:45 GMT+08:00 Jiri Kosina jkos...@suse.cz: On Fri, 22 Aug 2014, CheChun Kuo wrote: HID IR device will not response to any command send from host when it is finishing paring and tring to reset itself. During this period of time, usb_cleaer_halt will be fail and if hid_start_in soon again, we has the possibility trap in infinite loop. Signed-off-by: CheChun Kuo vichy@gmail.com --- drivers/hid/usbhid/hid-core.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 79cf503..256b102 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -122,7 +122,8 @@ static void hid_reset(struct work_struct *work) 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); + if (rc == 0) + hid_start_in(hid); } I'd need a more detailed explanation for this patch, as I don't understand neither the text in the changelog nor the patch itself. Namely: - which IR devices are showing this behavior? The USB hid device that will transform IR signal to usb command when user press volume up/down, voice recording, etc. - where does the infinite loop come from? hid_reset() should error out properly if usb_clear_halt() fails and HID_IN_RUNNING flag is not set Below I briefly draw the timing when this issue happen i) hid_irq_in get URB status as -EPIPE ii) set HID_CLEAR_HALT flag and schedule hid_reset work iii) hid_reset call usb_clear_halt and hid_start_in again iv) if device still doesn't response host command, it will go to i) above and keep looping thanks for your help, -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
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)) {
x
} else if (test_bit(HID_RESET_PENDING, usbhid-iofl)) {
xx
}
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);
+
Re: [PATCH 1/1] HID: usbhid: add usb_clear_halt determination for next hid_start_in
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)) {
x
} else if (test_bit(HID_RESET_PENDING, usbhid-iofl)) {
xx
}
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
