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

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

 



On Fri, Jun 8, 2012 at 1:22 PM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> On Fri, Jun 8, 2012 at 1:04 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote:
>> Ming, thanks for your comments.
>
> Welcome, :-)
>
>>
>> On Fri, Jun 8, 2012 at 11:29 AM, Ming Lei <tom.leiming@xxxxxxxxx> wrote:
>>> On Tue, Jun 5, 2012 at 10:12 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

  Powered by Linux