| 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
| ||
|
Message-ID: <CA+v9cxYvvRVWAUHX=b-P022pFbG1M7S=SZtOo2zfJnhAx5_+sQ@mail.gmail.com>
Date: Fri, 8 Jun 2012 13:04:51 +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
Ming, thanks for your comments.
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.
>> 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@...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.
>
>> + 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@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> 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