Message-ID: <CA+v9cxZkWSeT_Qs+amOxRoW9Fmpgp6HY3Uj0dBGTOAqrbkQ5Hg@mail.gmail.com>
Date:	Fri, 8 Jun 2012 23:54:57 +0800
From:	Huajun Li <huajun.li.lee@...il.com>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	"David S. Miller" <davem@...emloft.net>, linux-usb@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH] usbnet: Activate the halt interrupt endpoint to fix
 endless "XactErr" error

On Fri, Jun 8, 2012 at 9:56 PM, Ming Lei <tom.leiming@...il.com> wrote:
> On Fri, Jun 8, 2012 at 2:24 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
>> On Fri, Jun 8, 2012 at 1:22 PM, Ming Lei <tom.leiming@...il.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@...il.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists