[PATCH] usbnet: Activate halt interrupt endpoint before re-submit URB

[PATCH] usbnet: Activate halt interrupt endpoint before re-submit URB

[Huajun Li]

intr_complete() submits URB even the interrupt endpoint stalls.
This patch will try to activate the endpoint once the exception
occurs, and then re-submit the URB if the endpoint works again.

Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>
---
 drivers/net/usb/usbnet.c   |   31 +++++++++++++++++++++++++++++++
 include/linux/usb/usbnet.h |   13 +++++++------
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index ac2e493..314aaea 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -198,6 +198,10 @@ static void intr_complete (struct urb *urb)
 			  "intr shutdown, code %d\n", status);
 		return;

+	case -EPIPE:
+		usbnet_defer_kevent(dev, EVENT_STS_HALT);
+		return;
+
 	/* NOTE:  not throttling like RX/TX, since this endpoint
 	 * already polls infrequently
 	 */
@@ -964,6 +968,33 @@ fail_halt:
 		}
 	}

+	if (test_bit(EVENT_STS_HALT, &dev->flags)) {
+		unsigned pipe;
+		struct usb_endpoint_descriptor *desc;
+
+		desc = &dev->status->desc;
+		pipe = usb_rcvintpipe(dev->udev,
+			desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
+		status = usb_autopm_get_interface(dev->intf);
+		if (status < 0)
+			goto fail_sts;
+		status = usb_clear_halt(dev->udev, pipe);
+		if (status < 0) {
+			usb_autopm_put_interface(dev->intf);
+fail_sts:
+			netdev_err(dev->net,
+				"can't clear intr halt, status %d\n", status);
+		} else {
+			clear_bit(EVENT_STS_HALT, &dev->flags);
+			status = usb_submit_urb(dev->interrupt, GFP_KERNEL);
+			if (status != 0)
+				netif_err(dev, timer, dev->net,
+					"intr resubmit --> %d\n", status);
+
+			usb_autopm_put_interface(dev->intf);
+		}
+	}
+
 	/* tasklet could resubmit itself forever if memory is tight */
 	if (test_bit (EVENT_RX_MEMORY, &dev->flags)) {
 		struct urb	*urb = NULL;
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index f87cf62..81b4473 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -62,12 +62,13 @@ struct usbnet {
 	unsigned long		flags;
 #		define EVENT_TX_HALT	0
 #		define EVENT_RX_HALT	1
-#		define EVENT_RX_MEMORY	2
-#		define EVENT_STS_SPLIT	3
-#		define EVENT_LINK_RESET	4
-#		define EVENT_RX_PAUSED	5
-#		define EVENT_DEV_ASLEEP 6
-#		define EVENT_DEV_OPEN	7
+#		define EVENT_STS_HALT	2
+#		define EVENT_RX_MEMORY	3
+#		define EVENT_STS_SPLIT	4
+#		define EVENT_LINK_RESET	5
+#		define EVENT_RX_PAUSED	6
+#		define EVENT_DEV_ASLEEP 7
+#		define EVENT_DEV_OPEN	8
 };

 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.9.5

Re: [PATCH] usbnet: Activate halt interrupt endpoint before re-submit URB

[David Miller]

From: Huajun Li <huajun.li.lee@gmail.com>
Date: Wed, 13 Jun 2012 20:50:31 +0800

> intr_complete() submits URB even the interrupt endpoint stalls.
> This patch will try to activate the endpoint once the exception
> occurs, and then re-submit the URB if the endpoint works again.
> 
> Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>

Review from USB experts would be appreciated.

Thanks.

Re: [PATCH] usbnet: Activate halt interrupt endpoint before re-submit URB

[Oliver Neukum]

Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller:
> From: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Wed, 13 Jun 2012 20:50:31 +0800
> 
> > intr_complete() submits URB even the interrupt endpoint stalls.
> > This patch will try to activate the endpoint once the exception
> > occurs, and then re-submit the URB if the endpoint works again.
> > 
> > Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Review from USB experts would be appreciated.

The code implements a minimum error handler correctly.
Did you observe a stall in actual hardware or is this a just
in case patch?

	Regards
		Oliver
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Huajun Li]

On Mon, Jun 18, 2012 at 3:23 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller:
>> From: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Date: Wed, 13 Jun 2012 20:50:31 +0800
>>
>> > intr_complete() submits URB even the interrupt endpoint stalls.
>> > This patch will try to activate the endpoint once the exception
>> > occurs, and then re-submit the URB if the endpoint works again.
>> >
>> > Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Review from USB experts would be appreciated.
>
> The code implements a minimum error handler correctly.
> Did you observe a stall in actual hardware or is this a just
> in case patch?
>

This one is just a patch, thanks for your comments.

>        Regards
>                Oliver
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Oliver Neukum]

Am Montag, 18. Juni 2012, 18:25:15 schrieb Huajun Li:
> On Mon, Jun 18, 2012 at 3:23 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller:
> >> From: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Date: Wed, 13 Jun 2012 20:50:31 +0800
> >>
> >> > intr_complete() submits URB even the interrupt endpoint stalls.
> >> > This patch will try to activate the endpoint once the exception
> >> > occurs, and then re-submit the URB if the endpoint works again.
> >> >
> >> > Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> Review from USB experts would be appreciated.
> >
> > The code implements a minimum error handler correctly.
> > Did you observe a stall in actual hardware or is this a just
> > in case patch?
> >
> 
> This one is just a patch, thanks for your comments.

Then I am inclined to say that this is not fully thought through
If the endpoint stalls, the device has detected an error.
We have no idea how to generically handle this error.
We might come into a vicious circle. Could you add a sanity limit for the
amount of halts we clear?

	Regards
		Oliver
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Ming Lei]

On Tue, Jun 19, 2012 at 12:51 AM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:

>
> Then I am inclined to say that this is not fully thought through
> If the endpoint stalls, the device has detected an error.
> We have no idea how to generically handle this error.
> We might come into a vicious circle.

Why does it come into a vicious circle? If the interrupt endpoint is
HALTed, the clear halt event is scheduled to call usb_clear_halt(int pipe),
and if it returns successfully, the pipe shouldn't be halted any more,
so the later usb_submit_urb() should be OK on the interrupt endpoint.

The similar handling policy is taken for RX/TX endpoints in case they
are halted.

> Could you add a sanity limit for the
> amount of halts we clear?

Looks it is not needed, at least clearing HALT on Rx/Tx endpoints
works well without the limit.

Thanks,
-- 
Ming Lei
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Oliver Neukum]

Am Montag, 18. Juni 2012, 18:25:15 schrieb Huajun Li:
> On Mon, Jun 18, 2012 at 3:23 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> > Am Montag, 18. Juni 2012, 01:30:17 schrieb David Miller:
> >> From: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Date: Wed, 13 Jun 2012 20:50:31 +0800
> >>
> >> > intr_complete() submits URB even the interrupt endpoint stalls.
> >> > This patch will try to activate the endpoint once the exception
> >> > occurs, and then re-submit the URB if the endpoint works again.
> >> >
> >> > Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>
> >> Review from USB experts would be appreciated.
> >
> > The code implements a minimum error handler correctly.
> > Did you observe a stall in actual hardware or is this a just
> > in case patch?
> >
> 
> This one is just a patch, thanks for your comments.

Very well, on second thought, this patch makes sense.
Could you resend and I'll ack?

	Regards
		Oliver
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Ming Lei]

On Wed, Jun 20, 2012 at 4:00 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>
> Very well, on second thought, this patch makes sense.
> Could you resend and I'll ack?

BTW, maybe it is better to add below

    usbnet_defer_kevent(dev, EVENT_STS_HALT);

for -EPIPE returned from usb_urb_submit if it will be resent.

thanks,
-- 
Ming Lei
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Oliver Neukum]

Am Mittwoch, 20. Juni 2012, 10:07:55 schrieb Ming Lei:
> On Wed, Jun 20, 2012 at 4:00 PM, Oliver Neukum <oneukum@suse.de> wrote:
> >
> > Very well, on second thought, this patch makes sense.
> > Could you resend and I'll ack?
> 
> BTW, maybe it is better to add below
> 
>     usbnet_defer_kevent(dev, EVENT_STS_HALT);
> 
> for -EPIPE returned from usb_urb_submit if it will be resent.

Why? If it failed once it'll probably also fail the next time.
In that case we'd need to do something more intrusive
like resetting the device, but that cannot be done well
in the generic usbnet part.

	Regards
		Oliver

Re: [PATCH] usbnet: Activate halt interrupt endpoint before re-submit URB

[Ming Lei]

On Wed, Jun 20, 2012 at 4:58 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> Am Mittwoch, 20. Juni 2012, 10:07:55 schrieb Ming Lei:
>> BTW, maybe it is better to add below
>>
>>     usbnet_defer_kevent(dev, EVENT_STS_HALT);
>>
>> for -EPIPE returned from usb_urb_submit if it will be resent.
>
> Why? If it failed once it'll probably also fail the next time.

-EPIPE just means the endpoint is halted, either from usb_urb_submit
or urb->status, so the HALT should be cleared in the situation.

> In that case we'd need to do something more intrusive
> like resetting the device, but that cannot be done well
> in the generic usbnet part.

IMO, resetting is not needed for -EPIPE, but may be needed for
-EPROTO failure.

Thanks,
-- 
Ming Lei
--
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] usbnet: Activate halt interrupt endpoint before re-submit URB

[Oliver Neukum]

Am Mittwoch, 20. Juni 2012, 12:15:25 schrieb Ming Lei:
> On Wed, Jun 20, 2012 at 4:58 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Mittwoch, 20. Juni 2012, 10:07:55 schrieb Ming Lei:
> >> BTW, maybe it is better to add below
> >>
> >>     usbnet_defer_kevent(dev, EVENT_STS_HALT);
> >>
> >> for -EPIPE returned from usb_urb_submit if it will be resent.
> >
> > Why? If it failed once it'll probably also fail the next time.
> 
> -EPIPE just means the endpoint is halted, either from usb_urb_submit
> or urb->status, so the HALT should be cleared in the situation.

It probably was halted and cleared. However that you cleared
a halt doesn't mean that the reason for stalling went away.
So you must cope with an endpoint being halted again right after
it was cleared.

> > In that case we'd need to do something more intrusive
> > like resetting the device, but that cannot be done well
> > in the generic usbnet part.
> 
> IMO, resetting is not needed for -EPIPE, but may be needed for
> -EPROTO failure.

We don't need it for a single failure, but what else would we do
if we keep getting -EPIPE?

	Regards
		Oliver

Re: [PATCH] usbnet: Activate halt interrupt endpoint before re-submit URB

[Ming Lei]

On Wed, Jun 20, 2012 at 6:21 PM, Oliver Neukum <oneukum@suse.de> wrote:

> It probably was halted and cleared. However that you cleared
> a halt doesn't mean that the reason for stalling went away.
> So you must cope with an endpoint being halted again right after
> it was cleared.

I only suggested we should handle -EPIPE for usb_submit_urb
on interrupt endpoint, maybe it is the 1st handling, at least it is
per USB spec.

Also from implementation of usb gadget device, generally
ClearFeature(HALT) is to clear the some halt related flag of
endpoint hardware.

Looks the reasons of interrupt endpoint stalling is invisible
for usbnet driver, so it is not easy to handle the situation
you described(halted and cleared repeatedly).

>
>> > In that case we'd need to do something more intrusive
>> > like resetting the device, but that cannot be done well
>> > in the generic usbnet part.
>>
>> IMO, resetting is not needed for -EPIPE, but may be needed for
>> -EPROTO failure.
>
> We don't need it for a single failure, but what else would we do
> if we keep getting -EPIPE?

Suppose the case will happen, what is the appropriate actions
usbnet should take on the failure? I am not sure RESET can deal
with it.

Also is it a actual failure case or only a theory case?

Thanks,
-- 
Ming Lei