On Fri, Jun 8, 2012 at 2:24 PM, Huajun Li <huajun.li.lee@xxxxxxxxx> wrote:
> On Fri, Jun 8, 2012 at 1:22 PM, Ming Lei <tom.leiming@xxxxxxxxx> 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@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.
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
--
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
