Re: Call for testing of ehci(4), ohci(4), slhci(4), uhci(4) data toggle patch

[Yorick Hardy <yhardy%NetBSD.org@localhost>]

On 2010-08-09, Matthias Drochner wrote:
> 
> There is a remaining problem with data toggle handling, at least
> with bulk endpoints: The toggle state must persist if a usb
> pipe is closed and opened again, so it needs to be stored
> outside the host controller's soft pipe state.
> The appended patch fixes this for me, at least on uhci/ehci.
> (tested only on uhci - this fixes annoying timeouts with
> some device accessed through ugen/libusb)
> 
> Can someone have a look at it?

That seems to do what OpenBSD does, patch proposed here:

 http://mail-index.netbsd.org/current-users/2010/05/17/msg013433.html

I think Jonathan might have a better patch.

I don't understand all the differences between our USB code
and OpenBSD's yet, so I did not incorporate the rest of their changes.

> best regards
> Matthias
> 
> 
> 
> ------------------------------------------------------------------------------------------------
> ------------------------------------------------------------------------------------------------
> Forschungszentrum Juelich GmbH
> 52425 Juelich
> Sitz der Gesellschaft: Juelich
> Eingetragen im Handelsregister des Amtsgerichts Dueren Nr. HR B 3498
> Vorsitzender des Aufsichtsrats: MinDirig Dr. Karl Eugen Huthmacher
> Geschaeftsfuehrung: Prof. Dr. Achim Bachem (Vorsitzender),
> Dr. Ulrich Krafft (stellv. Vorsitzender), Prof. Dr.-Ing. Harald Bolt,
> Prof. Dr. Sebastian M. Schmidt
> ------------------------------------------------------------------------------------------------
> ------------------------------------------------------------------------------------------------

Content-Description: utog.txt
> #
> # old_revision [1d86a40e92abd84616fbb24c167ab7721d5e3715]
> #
> # patch "sys/dev/usb/ehci.c"
> #  from [e80a4d6ee7911d95a79104579abf15c38efd41d9]
> #    to [f72802102c17253ec4a89ef019aa49fbc82f5cf8]
> # 
> # patch "sys/dev/usb/uhci.c"
> #  from [7dd7c7747d374e882d3b33e24f7b59657857acf2]
> #    to [a63cfa191b06e1166582f06e19a114b660d3c7d1]
> # 
> # patch "sys/dev/usb/usb_subr.c"
> #  from [26d889890f4245a9a806aa74382505a54eb339ae]
> #    to [f9a3827440aa3897e79d81769ac5e7f3269c05bc]
> # 
> # patch "sys/dev/usb/usbdivar.h"
> #  from [502afde4a010f8ad432133b3962d295c9bdb8fc1]
> #    to [4dbee31ca0d7222630d71eafea84d98416219a65]
> #
> ============================================================
> --- sys/dev/usb/ehci.c        e80a4d6ee7911d95a79104579abf15c38efd41d9
> +++ sys/dev/usb/ehci.c        f72802102c17253ec4a89ef019aa49fbc82f5cf8
> @@ -1543,7 +1546,8 @@ ehci_open(usbd_pipe_handle pipe)
>       if (sc->sc_dying)
>               return (USBD_IOERROR);
>  
> -     epipe->nexttoggle = 0;
> +     /* toggle state needed for bulk endpoints */
> +     epipe->nexttoggle = pipe->endpoint->datatoggle;
>  
>       if (addr == sc->sc_addr) {
>               switch (ed->bEndpointAddress) {
> @@ -3516,8 +3520,10 @@ ehci_device_bulk_close(usbd_pipe_handle 
>  ehci_device_bulk_close(usbd_pipe_handle pipe)
>  {
>       ehci_softc_t *sc = pipe->device->bus->hci_private;
> +     struct ehci_pipe *epipe = (struct ehci_pipe *)pipe;
>  
>       DPRINTF(("ehci_device_bulk_close: pipe=%p\n", pipe));
> +     pipe->endpoint->datatoggle = epipe->nexttoggle;
>       ehci_close_pipe(pipe, sc->sc_async_head);
>  }
>  
> ============================================================
> --- sys/dev/usb/uhci.c        7dd7c7747d374e882d3b33e24f7b59657857acf2
> +++ sys/dev/usb/uhci.c        a63cfa191b06e1166582f06e19a114b660d3c7d1
> @@ -2177,6 +2177,8 @@ uhci_device_bulk_close(usbd_pipe_handle 
>       uhci_softc_t *sc = dev->bus->hci_private;
>  
>       uhci_free_sqh(sc, upipe->u.bulk.sqh);
> +
> +     pipe->endpoint->datatoggle = upipe->nexttoggle;
>  }
>  
>  usbd_status
> @@ -3185,7 +3187,8 @@ uhci_open(usbd_pipe_handle pipe)
>                    ed->bEndpointAddress, sc->sc_addr));
>  
>       upipe->aborting = 0;
> -     upipe->nexttoggle = 0;
> +     /* toggle state needed for bulk endpoints */
> +     upipe->nexttoggle = pipe->endpoint->datatoggle;
>  
>       if (pipe->device->address == sc->sc_addr) {
>               switch (ed->bEndpointAddress) {
> ============================================================
> --- sys/dev/usb/usb_subr.c    26d889890f4245a9a806aa74382505a54eb339ae
> +++ sys/dev/usb/usb_subr.c    f9a3827440aa3897e79d81769ac5e7f3269c05bc
> @@ -494,6 +494,7 @@ usbd_fill_iface_data(usbd_device_handle 
>                       }
>               }
>               ifc->endpoints[endpt].refcnt = 0;
> +             ifc->endpoints[endpt].datatoggle = 0;
>               p += ed->bLength;
>       }
>  #undef ed
> @@ -1104,6 +1107,9 @@ usbd_new_device(device_t parent, usbd_bu
>       USETW(dev->def_ep_desc.wMaxPacketSize, 64);
>       dev->def_ep_desc.bInterval = 0;
>  
> +     /* doesn't matter, just don't let it uninitialized */
> +     dev->def_ep.datatoggle = 0;
> +
>       dev->quirks = &usbd_no_quirk;
>       dev->address = USB_START_ADDR;
>       dev->ddesc.bMaxPacketSize = 0;
> ============================================================
> --- sys/dev/usb/usbdivar.h    502afde4a010f8ad432133b3962d295c9bdb8fc1
> +++ sys/dev/usb/usbdivar.h    4dbee31ca0d7222630d71eafea84d98416219a65
> @@ -46,6 +46,7 @@ struct usbd_endpoint {
>  struct usbd_endpoint {
>       usb_endpoint_descriptor_t *edesc;
>       int                     refcnt;
> +     int datatoggle;
>  };
>  
>  struct usbd_bus_methods {


-- 
Kind regards,

Yorick Hardy