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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACVXFVPj-4J2_UuOQ9NCW7wV_SLG50a-pEYqhd7K9sXuGmDmoQ@mail.gmail.com>
Date:	Fri, 8 Jun 2012 13:22:30 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Huajun Li <huajun.li.lee@...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 1:04 PM, Huajun Li <huajun.li.lee@...il.com> wrote:
> Ming, thanks for your comments.

Welcome, :-)

>
> On Fri, Jun 8, 2012 at 11:29 AM, Ming Lei <tom.leiming@...il.com> wrote:
>> On Tue, Jun 5, 2012 at 10:12 PM, Huajun Li <huajun.li.lee@...il.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@...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.

>
>>
>>> +               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
--
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