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

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

[CheChun Kuo]

	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.kuo@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

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

[Jiri Kosina]

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.kuo@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

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

[vichy]

hi Jiri:

2014-08-22 15:45 GMT+08:00 Jiri Kosina <jkosina@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.kuo@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,

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

[Alan Stern]

On Fri, 22 Aug 2014, vichy wrote:

> hi Jiri:
> 
> 2014-08-22 15:45 GMT+08:00 Jiri Kosina <jkosina@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.kuo@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,

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);
 	}
 }
 

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

[vichy]

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 */

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

[Alan Stern]

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

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

[vichy]

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,

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

[Alan Stern]

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

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

[Oliver Neukum]

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)) {
> >             xxxxx
> >     } else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> >             xxxxxx
> >     }
> 
> 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

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

[vichy]

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,

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

[vichy]

hi Oliver:

2014-08-25 18:21 GMT+08:00 Oliver Neukum <oneukum@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,

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

[Alan Stern]

On Mon, 25 Aug 2014, vichy wrote:

> hi Oliver:
> 
> 2014-08-25 18:21 GMT+08:00 Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>:
> > 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

[Alan Stern]

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html