[PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

There prints endless "XactErr" error msg once switch my device to the
configuration
 which needs cdc_ether driver, the root cause is the interrupt endpoint halts.
Maybe this is a common issue, so fix it by activating the endpoint
once the error occurs.

Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
 include/linux/usb/usbnet.h |   15 ++++++++-------
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9f58330..f13922b 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
 			  "intr shutdown, code %d\n", status);
 		return;

+	case -EPIPE:
+	case -EPROTO:
+		usbnet_defer_kevent(dev, EVENT_STS_HALT);
+		return;
+
 	/* NOTE:  not throttling like RX/TX, since this endpoint
 	 * already polls infrequently
 	 */
@@ -967,6 +972,34 @@ 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);
+		usb_autopm_put_interface(dev->intf);
+
+		if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
+fail_sts:
+			netdev_err(dev->net,
+				"can't clear intr halt, status %d\n", status);
+		} else {
+			clear_bit(EVENT_STS_HALT, &dev->flags);
+			memset(dev->interrupt->transfer_buffer, 0,
+				dev->interrupt->transfer_buffer_length);
+			status = usb_submit_urb(dev->interrupt, GFP_KERNEL);
+			if (status != 0)
+				netif_err(dev, timer, dev->net,
+					"intr resubmit --> %d\n", status);
+		}
+	}
+
 	/* 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 76f4396..c0bcb61 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -62,13 +62,14 @@ 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_WAKING 6
-#		define EVENT_DEV_ASLEEP 7
-#		define EVENT_DEV_OPEN	8
+#		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_WAKING 7
+#		define EVENT_DEV_ASLEEP 8
+#		define EVENT_DEV_OPEN	9
 };

 static inline struct usb_driver *driver_of(struct usb_interface *intf)
-- 
1.7.9.5
--
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 the halt interrupt endpoint to fix endless "XactErr" error

[David Miller]

From: Huajun Li <huajun.li.lee@gmail.com>
Date: Tue, 5 Jun 2012 22:12:17 +0800

> There prints endless "XactErr" error msg once switch my device to the
> configuration
>  which needs cdc_ether driver, the root cause is the interrupt endpoint halts.
> Maybe this is a common issue, so fix it by activating the endpoint
> once the error occurs.
> 
> Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>

A USB expert needs to review this as I lack the knowledge to adequately
go over this patch.

Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

On Fri, Jun 8, 2012 at 5:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Huajun Li <huajun.li.lee@gmail.com>
> Date: Tue, 5 Jun 2012 22:12:17 +0800
>
>> There prints endless "XactErr" error msg once switch my device to the
>> configuration
>>  which needs cdc_ether driver, the root cause is the interrupt endpoint halts.
>> Maybe this is a common issue, so fix it by activating the endpoint
>> once the error occurs.
>>
>> Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>
>
> A USB expert needs to review this as I lack the knowledge to adequately
> go over this patch.


Update the patch according to Ming and Alan's comments, it only
handles "-EPIPE" error of interrupt endpoint. Welcome comments:
=============================================

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9f58330..bd8ebef 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -537,6 +537,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
 	 */
@@ -967,6 +971,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 76f4396..c0bcb61 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -62,13 +62,14 @@ 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_WAKING 6
-#		define EVENT_DEV_ASLEEP 7
-#		define EVENT_DEV_OPEN	8
+#		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_WAKING 7
+#		define EVENT_DEV_ASLEEP 8
+#		define EVENT_DEV_OPEN	9
 };

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

Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Bjørn Mork]

Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index 76f4396..c0bcb61 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -62,13 +62,14 @@ 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_WAKING 6
> -#		define EVENT_DEV_ASLEEP 7
> -#		define EVENT_DEV_OPEN	8
> +#		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_WAKING 7
> +#		define EVENT_DEV_ASLEEP 8
> +#		define EVENT_DEV_OPEN	9
>  };

Why do you renumber all of these instead of adding the new
EVENT_STS_HALT to the end of the list?


Bjørn
--
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 the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

On Tue, Jun 12, 2012 at 4:47 AM, Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org> wrote:
> Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>
>> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
>> index 76f4396..c0bcb61 100644
>> --- a/include/linux/usb/usbnet.h
>> +++ b/include/linux/usb/usbnet.h
>> @@ -62,13 +62,14 @@ 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_WAKING 6
>> -#            define EVENT_DEV_ASLEEP 7
>> -#            define EVENT_DEV_OPEN   8
>> +#            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_WAKING 7
>> +#            define EVENT_DEV_ASLEEP 8
>> +#            define EVENT_DEV_OPEN   9
>>  };
>
> Why do you renumber all of these instead of adding the new
> EVENT_STS_HALT to the end of the list?
>

Thanks for your comments.

I think it's nice to sort these mask codes by their purposes.
--
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 the halt interrupt endpoint to fix endless "XactErr" error

[Ming Lei]

On Tue, Jun 5, 2012 at 10:12 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> There prints endless "XactErr" error msg once switch my device to the

The "XactErr" error msg means that there are some transfer error in the bus,
such as timeout, bad CRC, wrong PID, etc. Generally, -EPROTO is returned
to driver if this kind of error happened.

> configuration
>  which needs cdc_ether driver, the root cause is the interrupt endpoint halts.

How do you switch your configuration? by writing to
/sys/.../bConfigurationValue?

Is the "XactErr" msg printed just after switching to cdc_ether interface
by changing configuration?

> Maybe this is a common issue, so fix it by activating the endpoint
> once the error occurs.
>
> Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
>  include/linux/usb/usbnet.h |   15 ++++++++-------
>  2 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 9f58330..f13922b 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
>                          "intr shutdown, code %d\n", status);
>                return;
>
> +       case -EPIPE:
> +       case -EPROTO:

It is good to handle EPIPE error here, but looks it is no sense to
clear halt for
bus transfer failure. At least, no clear halt is done for returning -EPROTO from
rx/tx transfer currently.

> +               usbnet_defer_kevent(dev, EVENT_STS_HALT);
> +               return;
> +
>        /* NOTE:  not throttling like RX/TX, since this endpoint
>         * already polls infrequently
>         */
> @@ -967,6 +972,34 @@ 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);
> +               usb_autopm_put_interface(dev->intf);
> +
> +               if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
> +fail_sts:
> +                       netdev_err(dev->net,
> +                               "can't clear intr halt, status %d\n", status);
> +               } else {
> +                       clear_bit(EVENT_STS_HALT, &dev->flags);
> +                       memset(dev->interrupt->transfer_buffer, 0,
> +                               dev->interrupt->transfer_buffer_length);

The above is not necessary.

> +                       status = usb_submit_urb(dev->interrupt, GFP_KERNEL);

Before submitting urb, usb_autopm_get_interface is required to wakeup device.

> +                       if (status != 0)
> +                               netif_err(dev, timer, dev->net,
> +                                       "intr resubmit --> %d\n", status);
> +               }
> +       }
> +
>        /* 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 76f4396..c0bcb61 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -62,13 +62,14 @@ 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_WAKING 6
> -#              define EVENT_DEV_ASLEEP 7
> -#              define EVENT_DEV_OPEN   8
> +#              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_WAKING 7
> +#              define EVENT_DEV_ASLEEP 8
> +#              define EVENT_DEV_OPEN   9
>  };
>
>  static inline struct usb_driver *driver_of(struct usb_interface *intf)
> --
> 1.7.9.5
> --
> 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


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 the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

Ming, thanks for your comments.

On Fri, Jun 8, 2012 at 11:29 AM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jun 5, 2012 at 10:12 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> There prints endless "XactErr" error msg once switch my device to the
>
> The "XactErr" error msg means that there are some transfer error in the bus,
> such as timeout, bad CRC, wrong PID, etc. Generally, -EPROTO is returned
> to driver if this kind of error happened.
>
Yes, you pointed out the why there printed the error.
However, how can this error(-EPROTO) happen? Actually, maybe it is
caused by malfunction device, bad usb cable, or other issues. In my
case, it is because the endpoint halts.

>> configuration
>>  which needs cdc_ether driver, the root cause is the interrupt endpoint halts.
>
> How do you switch your configuration? by writing to
> /sys/.../bConfigurationValue?
>

Maybe.
I were using a third party application to switch the configurations,
so I think it should use this way to do it unless there is other
approach.

> Is the "XactErr" msg printed just after switching to cdc_ether interface
> by changing configuration?
>

Yes, just as I mentioned in my original email.
And it did not work even I removed the driver and re-installed it.

>> Maybe this is a common issue, so fix it by activating the endpoint
>> once the error occurs.
>>
>> Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
>>  include/linux/usb/usbnet.h |   15 ++++++++-------
>>  2 files changed, 41 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>> index 9f58330..f13922b 100644
>> --- a/drivers/net/usb/usbnet.c
>> +++ b/drivers/net/usb/usbnet.c
>> @@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
>>                          "intr shutdown, code %d\n", status);
>>                return;
>>
>> +       case -EPIPE:
>> +       case -EPROTO:
>
> It is good to handle EPIPE error here, but looks it is no sense to
> clear halt for
> bus transfer failure. At least, no clear halt is done for returning -EPROTO from
> rx/tx transfer currently.

Just as I said above, there is different issue can cause -EPROTO, at
least, for my case it is because the interrupt endpoint is not active.
If the error occurs, the driver need try to fix it instead of just
printing an error msg.

>
>> +               usbnet_defer_kevent(dev, EVENT_STS_HALT);
>> +               return;
>> +
>>        /* NOTE:  not throttling like RX/TX, since this endpoint
>>         * already polls infrequently
>>         */
>> @@ -967,6 +972,34 @@ 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);
>> +               usb_autopm_put_interface(dev->intf);
>> +
>> +               if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
>> +fail_sts:
>> +                       netdev_err(dev->net,
>> +                               "can't clear intr halt, status %d\n", status);
>> +               } else {
>> +                       clear_bit(EVENT_STS_HALT, &dev->flags);
>> +                       memset(dev->interrupt->transfer_buffer, 0,
>> +                               dev->interrupt->transfer_buffer_length);
>
> The above is not necessary.

Ming, do you mean the above one line, or others ?

>
>> +                       status = usb_submit_urb(dev->interrupt, GFP_KERNEL);
>
> Before submitting urb, usb_autopm_get_interface is required to wakeup device.
>

Got it. Will check its context, thanks.

>> +                       if (status != 0)
>> +                               netif_err(dev, timer, dev->net,
>> +                                       "intr resubmit --> %d\n", status);
>> +               }
>> +       }
>> +
>>        /* 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 76f4396..c0bcb61 100644
>> --- a/include/linux/usb/usbnet.h
>> +++ b/include/linux/usb/usbnet.h
>> @@ -62,13 +62,14 @@ 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_WAKING 6
>> -#              define EVENT_DEV_ASLEEP 7
>> -#              define EVENT_DEV_OPEN   8
>> +#              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_WAKING 7
>> +#              define EVENT_DEV_ASLEEP 8
>> +#              define EVENT_DEV_OPEN   9
>>  };
>>
>>  static inline struct usb_driver *driver_of(struct usb_interface *intf)
>> --
>> 1.7.9.5
>> --
>> 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
>
>
> 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 the halt interrupt endpoint to fix endless "XactErr" error

[Ming Lei]

On Fri, Jun 8, 2012 at 1:04 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> Ming, thanks for your comments.

Welcome, :-)

>
> On Fri, Jun 8, 2012 at 11:29 AM, Ming Lei <tom.leiming@gmail.com> wrote:
>> On Tue, Jun 5, 2012 at 10:12 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>> There prints endless "XactErr" error msg once switch my device to the
>>
>> The "XactErr" error msg means that there are some transfer error in the bus,
>> such as timeout, bad CRC, wrong PID, etc. Generally, -EPROTO is returned
>> to driver if this kind of error happened.
>>
> Yes, you pointed out the why there printed the error.
> However, how can this error(-EPROTO) happen? Actually, maybe it is
> caused by malfunction device, bad usb cable, or other issues. In my
> case, it is because the endpoint halts.

If so, looks mistaken value is returned from the host controller driver,
but not sure if your device is buggy. What is your host controller?

Also suppose your device is buggy, and the correct solution should
be addding quirk for the driver to clear halt before the 1st submit status
urb.

>
>>> configuration
>>>  which needs cdc_ether driver, the root cause is the interrupt endpoint halts.
>>
>> How do you switch your configuration? by writing to
>> /sys/.../bConfigurationValue?
>>
>
> Maybe.
> I were using a third party application to switch the configurations,
> so I think it should use this way to do it unless there is other
> approach.

I just have tried to switch configuration by sysfs interface on the g_multi
and don't trigger the error.

>
>> Is the "XactErr" msg printed just after switching to cdc_ether interface
>> by changing configuration?
>>
>
> Yes, just as I mentioned in my original email.
> And it did not work even I removed the driver and re-installed it.
>
>>> Maybe this is a common issue, so fix it by activating the endpoint
>>> once the error occurs.
>>>
>>> Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>
>>> ---
>>>  drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
>>>  include/linux/usb/usbnet.h |   15 ++++++++-------
>>>  2 files changed, 41 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>> index 9f58330..f13922b 100644
>>> --- a/drivers/net/usb/usbnet.c
>>> +++ b/drivers/net/usb/usbnet.c
>>> @@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
>>>                          "intr shutdown, code %d\n", status);
>>>                return;
>>>
>>> +       case -EPIPE:
>>> +       case -EPROTO:
>>
>> It is good to handle EPIPE error here, but looks it is no sense to
>> clear halt for
>> bus transfer failure. At least, no clear halt is done for returning -EPROTO from
>> rx/tx transfer currently.
>
> Just as I said above, there is different issue can cause -EPROTO, at
> least, for my case it is because the interrupt endpoint is not active.
> If the error occurs, the driver need try to fix it instead of just
> printing an error msg.

One problem in your patch is that if the  -EPROTO is caused by bad cable
or interference, clean halt may not be sent to device successfully, and will
cause -EPROTO further.

>
>>
>>> +               usbnet_defer_kevent(dev, EVENT_STS_HALT);
>>> +               return;
>>> +
>>>        /* NOTE:  not throttling like RX/TX, since this endpoint
>>>         * already polls infrequently
>>>         */
>>> @@ -967,6 +972,34 @@ 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);
>>> +               usb_autopm_put_interface(dev->intf);
>>> +
>>> +               if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
>>> +fail_sts:
>>> +                       netdev_err(dev->net,
>>> +                               "can't clear intr halt, status %d\n", status);
>>> +               } else {
>>> +                       clear_bit(EVENT_STS_HALT, &dev->flags);
>>> +                       memset(dev->interrupt->transfer_buffer, 0,
>>> +                               dev->interrupt->transfer_buffer_length);
>>
>> The above is not necessary.
>
> Ming, do you mean the above one line, or others ?

Yes, it is the above line.



Thanks,
-- 
Ming Lei

Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

On Fri, Jun 8, 2012 at 1:22 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jun 8, 2012 at 1:04 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Ming, thanks for your comments.
>
> Welcome, :-)
>
>>
>> On Fri, Jun 8, 2012 at 11:29 AM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On Tue, Jun 5, 2012 at 10:12 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> There prints endless "XactErr" error msg once switch my device to the
>>>
>>> The "XactErr" error msg means that there are some transfer error in the bus,
>>> such as timeout, bad CRC, wrong PID, etc. Generally, -EPROTO is returned
>>> to driver if this kind of error happened.
>>>
>> Yes, you pointed out the why there printed the error.
>> However, how can this error(-EPROTO) happen? Actually, maybe it is
>> caused by malfunction device, bad usb cable, or other issues. In my
>> case, it is because the endpoint halts.
>
> If so, looks mistaken value is returned from the host controller driver,
> but not sure if your device is buggy. What is your host controller?
>
Nothing related to HC.
I tried to find out the endpoint state, but found it was halt. I think
this is the root cause.

> Also suppose your device is buggy, and the correct solution should
> be addding quirk for the driver to clear halt before the 1st submit status
> urb.
>
I ever worked out a patch just as you said and it could work.
However, if this can be fixed by common framework just like usbnet.c,
and there is no sideeffect, then why not.

>>
>>>> configuration
>>>>  which needs cdc_ether driver, the root cause is the interrupt endpoint halts.
>>>
>>> How do you switch your configuration? by writing to
>>> /sys/.../bConfigurationValue?
>>>
>>
>> Maybe.
>> I were using a third party application to switch the configurations,
>> so I think it should use this way to do it unless there is other
>> approach.
>
> I just have tried to switch configuration by sysfs interface on the g_multi
> and don't trigger the error.
>
The driver is common one, but not just for a specific device.

>>
>>> Is the "XactErr" msg printed just after switching to cdc_ether interface
>>> by changing configuration?
>>>
>>
>> Yes, just as I mentioned in my original email.
>> And it did not work even I removed the driver and re-installed it.
>>
>>>> Maybe this is a common issue, so fix it by activating the endpoint
>>>> once the error occurs.
>>>>
>>>> Signed-off-by: Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
>>>>  include/linux/usb/usbnet.h |   15 ++++++++-------
>>>>  2 files changed, 41 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>> index 9f58330..f13922b 100644
>>>> --- a/drivers/net/usb/usbnet.c
>>>> +++ b/drivers/net/usb/usbnet.c
>>>> @@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
>>>>                          "intr shutdown, code %d\n", status);
>>>>                return;
>>>>
>>>> +       case -EPIPE:
>>>> +       case -EPROTO:
>>>
>>> It is good to handle EPIPE error here, but looks it is no sense to
>>> clear halt for
>>> bus transfer failure. At least, no clear halt is done for returning -EPROTO from
>>> rx/tx transfer currently.
>>
>> Just as I said above, there is different issue can cause -EPROTO, at
>> least, for my case it is because the interrupt endpoint is not active.
>> If the error occurs, the driver need try to fix it instead of just
>> printing an error msg.
>
> One problem in your patch is that if the  -EPROTO is caused by bad cable
> or interference, clean halt may not be sent to device successfully, and will
> cause -EPROTO further.

What's your opinion to handle "-EPROTO" error in usbnet.c?
Please check usbnet.c again, when "-EPROTO" occurs, it just pints
error msg and re-submit the interrupt URB, and then causes endless
"EactErr" error msg.

At least, this patch lets the driver try to fix the error before
resubmit the URB.

>
>>
>>>
>>>> +               usbnet_defer_kevent(dev, EVENT_STS_HALT);
>>>> +               return;
>>>> +
>>>>        /* NOTE:  not throttling like RX/TX, since this endpoint
>>>>         * already polls infrequently
>>>>         */
>>>> @@ -967,6 +972,34 @@ 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);
>>>> +               usb_autopm_put_interface(dev->intf);
>>>> +
>>>> +               if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
>>>> +fail_sts:
>>>> +                       netdev_err(dev->net,
>>>> +                               "can't clear intr halt, status %d\n", status);
>>>> +               } else {
>>>> +                       clear_bit(EVENT_STS_HALT, &dev->flags);
>>>> +                       memset(dev->interrupt->transfer_buffer, 0,
>>>> +                               dev->interrupt->transfer_buffer_length);
>>>
>>> The above is not necessary.
>>
>> Ming, do you mean the above one line, or others ?
>
> Yes, it is the above line.
>

Then not sure whether the buffer will be tainted without this line.
>
>
> 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 the halt interrupt endpoint to fix endless "XactErr" error

[Alan Stern]

On Fri, 8 Jun 2012, Huajun Li wrote:

> > If so, looks mistaken value is returned from the host controller driver,
> > but not sure if your device is buggy. What is your host controller?
> >
> Nothing related to HC.
> I tried to find out the endpoint state, but found it was halt. I think
> this is the root cause.

No, it isn't.  Endpoint halt causes a -EPIPE error, not -EPROTO.  
-EPROTO indicates that the device's firmware has crashed.

> What's your opinion to handle "-EPROTO" error in usbnet.c?
> Please check usbnet.c again, when "-EPROTO" occurs, it just pints
> error msg and re-submit the interrupt URB, and then causes endless
> "EactErr" error msg.

One possibility is to wait for a little while before resubmitting the 
URB, and after 10 failures in a row, attempt 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] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

On Fri, Jun 8, 2012 at 9:43 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 8 Jun 2012, Huajun Li wrote:
>
>> > If so, looks mistaken value is returned from the host controller driver,
>> > but not sure if your device is buggy. What is your host controller?
>> >
>> Nothing related to HC.
>> I tried to find out the endpoint state, but found it was halt. I think
>> this is the root cause.
>
> No, it isn't.  Endpoint halt causes a -EPIPE error, not -EPROTO.
> -EPROTO indicates that the device's firmware has crashed.
>
>> What's your opinion to handle "-EPROTO" error in usbnet.c?
>> Please check usbnet.c again, when "-EPROTO" occurs, it just pints
>> error msg and re-submit the interrupt URB, and then causes endless
>> "EactErr" error msg.
>
> One possibility is to wait for a little while before resubmitting the
> URB, and after 10 failures in a row, attempt a reset.
>
Alan, thanks for your proposal.
You mean reset the device after 10 failures, right ?

BTW, I ever tried to sleep several seconds before submitting the 1st
interrupt URB, but it did not work.

Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Alan Stern]

On Fri, 8 Jun 2012, Huajun Li wrote:

> > One possibility is to wait for a little while before resubmitting the
> > URB, and after 10 failures in a row, attempt a reset.
> >
> Alan, thanks for your proposal.
> You mean reset the device after 10 failures, right ?

Yes.  But I don't know if it will help your problem.

> BTW, I ever tried to sleep several seconds before submitting the 1st
> interrupt URB, but it did not work.

As Ming Lei said, it sounds like your device isn't working right.

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] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

On Sat, Jun 9, 2012 at 12:13 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Fri, 8 Jun 2012, Huajun Li wrote:
>
>> > One possibility is to wait for a little while before resubmitting the
>> > URB, and after 10 failures in a row, attempt a reset.
>> >
>> Alan, thanks for your proposal.
>> You mean reset the device after 10 failures, right ?
>
> Yes.  But I don't know if it will help your problem.
>

usb_queue_reset_device() could make it work again, I ever tried this.

And I tried usb_clear_halt() and found this also made it work, so
drafted this patch.

>> BTW, I ever tried to sleep several seconds before submitting the 1st
>> interrupt URB, but it did not work.
>
> As Ming Lei said, it sounds like your device isn't working right.
>

Yes.
However, if add a quirk to fix the issue,  I did not find a elegant
solution, at least based on current usbnet/cdc_ether framework.
--
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 the halt interrupt endpoint to fix endless "XactErr" error

[Ming Lei]

On Fri, Jun 8, 2012 at 2:24 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Fri, Jun 8, 2012 at 1:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> If so, looks mistaken value is returned from the host controller driver,
>> but not sure if your device is buggy. What is your host controller?
>>
> Nothing related to HC.
> I tried to find out the endpoint state, but found it was halt. I think
> this is the root cause.

I mean that your HCD should not return -EPROTO if only the interrupt
endpoint's HALT feature is set, and it should return -EPIPE.

>
>> Also suppose your device is buggy, and the correct solution should
>> be addding quirk for the driver to clear halt before the 1st submit status
>> urb.
>>
> I ever worked out a patch just as you said and it could work.
> However, if this can be fixed by common framework just like usbnet.c,
> and there is no sideeffect, then why not.

There is side effect, at least sending out the command of
clear feature(HALT) is mistaken in logic if  -EPROTO is returned for
the endpoint.

>>
>> I just have tried to switch configuration by sysfs interface on the g_multi
>> and don't trigger the error.
>>
> The driver is common one, but not just for a specific device.

The problem is that your device is a specific buggy device, and the interrupt
endpoint shouldn't be set HALT after SetConfiguration(), see 9.4.5 of USB 2.0
spec.

So it is reasonable to add a quirk to fix the problem for the device, that has
document benefits, also considered that the device is a very specific case.

>
>>>
>>>> Is the "XactErr" msg printed just after switching to cdc_ether interface
>>>> by changing configuration?
>>>>
>>>
>>> Yes, just as I mentioned in my original email.
>>> And it did not work even I removed the driver and re-installed it.
>>>
>>>>> Maybe this is a common issue, so fix it by activating the endpoint
>>>>> once the error occurs.
>>>>>
>>>>> Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>
>>>>> ---
>>>>>  drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
>>>>>  include/linux/usb/usbnet.h |   15 ++++++++-------
>>>>>  2 files changed, 41 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>>> index 9f58330..f13922b 100644
>>>>> --- a/drivers/net/usb/usbnet.c
>>>>> +++ b/drivers/net/usb/usbnet.c
>>>>> @@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
>>>>>                          "intr shutdown, code %d\n", status);
>>>>>                return;
>>>>>
>>>>> +       case -EPIPE:
>>>>> +       case -EPROTO:
>>>>
>>>> It is good to handle EPIPE error here, but looks it is no sense to
>>>> clear halt for
>>>> bus transfer failure. At least, no clear halt is done for returning -EPROTO from
>>>> rx/tx transfer currently.
>>>
>>> Just as I said above, there is different issue can cause -EPROTO, at
>>> least, for my case it is because the interrupt endpoint is not active.
>>> If the error occurs, the driver need try to fix it instead of just
>>> printing an error msg.
>>
>> One problem in your patch is that if the  -EPROTO is caused by bad cable
>> or interference, clean halt may not be sent to device successfully, and will
>> cause -EPROTO further.
>
> What's your opinion to handle "-EPROTO" error in usbnet.c?
> Please check usbnet.c again, when "-EPROTO" occurs, it just pints
> error msg and re-submit the interrupt URB, and then causes endless
> "EactErr" error msg.

Yes, it should be bug, but clear feature(HALT) is not correct for the situation.

>
> At least, this patch lets the driver try to fix the error before
> resubmit the URB.
>
>>
>>>
>>>>
>>>>> +               usbnet_defer_kevent(dev, EVENT_STS_HALT);
>>>>> +               return;
>>>>> +
>>>>>        /* NOTE:  not throttling like RX/TX, since this endpoint
>>>>>         * already polls infrequently
>>>>>         */
>>>>> @@ -967,6 +972,34 @@ 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);
>>>>> +               usb_autopm_put_interface(dev->intf);
>>>>> +
>>>>> +               if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
>>>>> +fail_sts:
>>>>> +                       netdev_err(dev->net,
>>>>> +                               "can't clear intr halt, status %d\n", status);
>>>>> +               } else {
>>>>> +                       clear_bit(EVENT_STS_HALT, &dev->flags);
>>>>> +                       memset(dev->interrupt->transfer_buffer, 0,
>>>>> +                               dev->interrupt->transfer_buffer_length);
>>>>
>>>> The above is not necessary.
>>>
>>> Ming, do you mean the above one line, or others ?
>>
>> Yes, it is the above line.
>>
>
> Then not sure whether the buffer will be tainted without this line.

It isn't necessary,  the buffer should include valid data if URB->status
returns zero.


Thanks,
-- 
Ming Lei

Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Huajun Li]

On Fri, Jun 8, 2012 at 9:56 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Fri, Jun 8, 2012 at 2:24 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>> On Fri, Jun 8, 2012 at 1:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> If so, looks mistaken value is returned from the host controller driver,
>>> but not sure if your device is buggy. What is your host controller?
>>>
>> Nothing related to HC.
>> I tried to find out the endpoint state, but found it was halt. I think
>> this is the root cause.
>
> I mean that your HCD should not return -EPROTO if only the interrupt
> endpoint's HALT feature is set, and it should return -EPIPE.
>
>>
>>> Also suppose your device is buggy, and the correct solution should
>>> be addding quirk for the driver to clear halt before the 1st submit status
>>> urb.
>>>
>> I ever worked out a patch just as you said and it could work.
>> However, if this can be fixed by common framework just like usbnet.c,
>> and there is no sideeffect, then why not.
>
> There is side effect, at least sending out the command of
> clear feature(HALT) is mistaken in logic if  -EPROTO is returned for
> the endpoint.
>
>>>
>>> I just have tried to switch configuration by sysfs interface on the g_multi
>>> and don't trigger the error.
>>>
>> The driver is common one, but not just for a specific device.
>
> The problem is that your device is a specific buggy device, and the interrupt
> endpoint shouldn't be set HALT after SetConfiguration(), see 9.4.5 of USB 2.0
> spec.
>
> So it is reasonable to add a quirk to fix the problem for the device, that has
> document benefits, also considered that the device is a very specific case.
>

If add a quirk to fix this issue, it need copy usbnet_cdc_bind() and
write a new xxx_cdc_bind() just to let it clear the halt. What's your
proposal?

BTW, the device can work well if I modprobe  cdc_ether driver after
changed the configuration.

>>
>>>>
>>>>> Is the "XactErr" msg printed just after switching to cdc_ether interface
>>>>> by changing configuration?
>>>>>
>>>>
>>>> Yes, just as I mentioned in my original email.
>>>> And it did not work even I removed the driver and re-installed it.
>>>>
>>>>>> Maybe this is a common issue, so fix it by activating the endpoint
>>>>>> once the error occurs.
>>>>>>
>>>>>> Signed-off-by: Huajun Li <huajun.li.lee@gmail.com>
>>>>>> ---
>>>>>>  drivers/net/usb/usbnet.c   |   33 +++++++++++++++++++++++++++++++++
>>>>>>  include/linux/usb/usbnet.h |   15 ++++++++-------
>>>>>>  2 files changed, 41 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
>>>>>> index 9f58330..f13922b 100644
>>>>>> --- a/drivers/net/usb/usbnet.c
>>>>>> +++ b/drivers/net/usb/usbnet.c
>>>>>> @@ -537,6 +537,11 @@ static void intr_complete (struct urb *urb)
>>>>>>                          "intr shutdown, code %d\n", status);
>>>>>>                return;
>>>>>>
>>>>>> +       case -EPIPE:
>>>>>> +       case -EPROTO:
>>>>>
>>>>> It is good to handle EPIPE error here, but looks it is no sense to
>>>>> clear halt for
>>>>> bus transfer failure. At least, no clear halt is done for returning -EPROTO from
>>>>> rx/tx transfer currently.
>>>>
>>>> Just as I said above, there is different issue can cause -EPROTO, at
>>>> least, for my case it is because the interrupt endpoint is not active.
>>>> If the error occurs, the driver need try to fix it instead of just
>>>> printing an error msg.
>>>
>>> One problem in your patch is that if the  -EPROTO is caused by bad cable
>>> or interference, clean halt may not be sent to device successfully, and will
>>> cause -EPROTO further.
>>
>> What's your opinion to handle "-EPROTO" error in usbnet.c?
>> Please check usbnet.c again, when "-EPROTO" occurs, it just pints
>> error msg and re-submit the interrupt URB, and then causes endless
>> "EactErr" error msg.
>
> Yes, it should be bug, but clear feature(HALT) is not correct for the situation.
>
>>
>> At least, this patch lets the driver try to fix the error before
>> resubmit the URB.
>>
>>>
>>>>
>>>>>
>>>>>> +               usbnet_defer_kevent(dev, EVENT_STS_HALT);
>>>>>> +               return;
>>>>>> +
>>>>>>        /* NOTE:  not throttling like RX/TX, since this endpoint
>>>>>>         * already polls infrequently
>>>>>>         */
>>>>>> @@ -967,6 +972,34 @@ 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);
>>>>>> +               usb_autopm_put_interface(dev->intf);
>>>>>> +
>>>>>> +               if (status < 0 && status != -EPIPE && status != -ESHUTDOWN) {
>>>>>> +fail_sts:
>>>>>> +                       netdev_err(dev->net,
>>>>>> +                               "can't clear intr halt, status %d\n", status);
>>>>>> +               } else {
>>>>>> +                       clear_bit(EVENT_STS_HALT, &dev->flags);
>>>>>> +                       memset(dev->interrupt->transfer_buffer, 0,
>>>>>> +                               dev->interrupt->transfer_buffer_length);
>>>>>
>>>>> The above is not necessary.
>>>>
>>>> Ming, do you mean the above one line, or others ?
>>>
>>> Yes, it is the above line.
>>>
>>
>> Then not sure whether the buffer will be tainted without this line.
>
> It isn't necessary,  the buffer should include valid data if URB->status
> returns zero.
>

That's great, thanks.

>
> Thanks,
> --
> Ming Lei

Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix endless "XactErr" error

[Ming Lei]

On Sun, Jun 10, 2012 at 7:13 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Jun 10, 2012 at 5:53 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Sun, Jun 10, 2012 at 9:07 AM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>>
>>> However, the other interface can work, it is of  "bInterfaceClass
>>>  10 CDC Data". I can set IP to usb0 and it work fine.
>>>
>>> BTW, the device has 2 interfaces, one is of "bInterfaceClass        10
>>> CDC Data" which includes bulk in/out endpoint for rx/tx;  the other is
>>> "bInterfaceClass         2 Communications" which contains the
>>> interrupt endpoint. From info dumped by "lsusb -v", the device is
>>> compliance with CDC protocol.
>>
>> Both the two interfaces belongs to cdc_ether device, see cdc spec
>> and usbnet_generic_cdc_bind(), which will make cdc_ether dirver to
>> claim data interface.
>>
>
> Yes, you are right.
>
>>>
>>>>>> BTW, the device can work well if I modprobe  cdc_ether driver after
>>>>>> changed the configuration.
>>>>
>>>> Maybe just the delay introduced by 'modprobe cdc_ether' fixes the
>>>> problem.
>>
>> If you can change the firmware, please find the bug and fix it. Otherwise,
>> you can add a simple quirk in cdc_ether.c for your device.
>>
> Yes, this solution can solve the issue directly.
>
> However,  besides "-EPIPE", I think usbnet.c should also handle the
> error of "-EPROTO".

Yes, you can clear halt for -EPIPE, and take Alan's suggestion to
reset device if the error of "-EPROTO" is got.


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