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: <CACVXFVMwhoYyJHAsK0xF0poZxoZv8ENHhquPVAiGnsow5-FH8Q@mail.gmail.com>
Date:	Fri, 8 Jun 2012 11:29:24 +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 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.

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

Is the "XactErr" msg printed just after switching to cdc_ether interface
by changing configuration?

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

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

> +                       status = usb_submit_urb(dev->interrupt, GFP_KERNEL);

Before submitting urb, usb_autopm_get_interface is required to wakeup device.

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