Re: ugen(4) Asynchronous USB Requests

[Martin Pieuchot]

On 30/05/16(Mon) 12:15, Phil Vachon wrote:
> On May 30, 2016 at 9:28:52 AM, Martin Pieuchot wrote:
> > On 26/05/16(Thu) 14:55, Phil Vachon wrote:
> > You should get in touch with Grant Czajkowski, he did something similar
> > during the GSoC 2015, see:
> > 
> > https://www.google-melange.com/gsoc/project/details/google/gsoc2015/coolguy1253/5649050225344512
> > 
> 
> Damn, now I wish I had looked harder before I embarked on all this. I
> will reach out to Grant and see how we could maybe work together to
> get this over the finish line to go into an upcoming release. It's
> very helpful to have! I'm guessing Grant's contact info can be found
> in the mailing list archives, so I'll have a look.

It would be *very* helpful indeed :) 

> > > Currently broken things:
> > > - The entire synchronous interface (poll-related changes)
> > 
> > That's fine as long as libusb is update accordingly. But if it doesn't
> > make sense to keep two set of ioctl(2) then, simply get rid of the old
> > ones ;)
> > 
> 
> Loud and clear. I'll see if we can keep the current interface as
> intact as possible (based on your comment below), but if there are
> 'appendices' that remain, they will be removed.

Well the current interface does not necessarily need to stay.  As long
as the following components are adapted to work with the new ABI,
there's no problem in changing it:

usr.sbin/usbdevs
usr.sbin/usbhidaction
usr.sbin/usbhidctl
lib/libusbhid

and the port devel/libusb1

I'd actually prefer to have, in the end, fewer ioctl(2), which mean
fewer different code paths and a smaller API to maintain.

> > > - Cancelling pending transfers
> > 
> > That's the hard part of the problem. Grant's work includes a working
> > solution, but modifying the HC driver for that should be avoided.
> > 
> 
> This is one thing I'm not 100% understanding -- my plan (though Grant
> has already gone through this) was to expose the pipe's abort()
> method through a similar usbi_* function. I presumed this should be
> callable safely at splusb(), since this is how a pipe goes about
> cleaning itself up? There was a change to uhci(4) that seems to insert
> a USB task for every transfer that is to be cancelled, is this the HC
> driver change you're referring to?

I'm referring to his diff.  The problem with the pipe's abort() function
is that it abort *all* the pending transfers on a given pipe and wait
until this is done.  I'm not sure that's the semantic asked by libusb.

> I am guessing that my presumption that abort() is safely callable at
> splusb() is wrong?

You don't event need to raise the SPL, usbd_abort_pipe() do it for you.

> The other thing that I was trying to avoid in cancellation was having
> complexities around identifying pending operations. Originally I
> hacked something together using the address of the source/target
> userspace buffer for identifying the operation. But the same buffer
> could be reused for multiple transactions so this does not make
> sense. The other idea I had was to provide a transaction 'handle'
> returned by the USB_ASYNC_SUBMIT ioctl(2) that would then be used to
> later reference the operation. Both approaches have lots of downsides,
> but since libusb is already doing bookkeeping on the programmer's
> behalf, the latter approach might not be too onerous.

Since you won't have a high number of in-flight transfer, iterating over
a small list to compare a cookie is perfectly ok performance wise.  So
definitively the second approach.  Once again, see how Grant solved
that.  I'm not saying that's the only or best approach, but there are
some ideas and working code.

> > > - Isochronous transfers
> > 
> > That's not important in the first place.
> > 
> 
> I didn't think so, but I was thinking of providing a kernel-managed
> queue for isochronous transfers. Then userspace would populate the
> queue with transfers to be submitted after the next isochronous
> transfer completes. Something similar could be done for interrupt
> endpoints too, which would allow for the timing 'requirements' to be
> met more readily for such things. 

I'd be happy to see a diff for that, but I believe it's bit too early
considering that there's nothing for control and bulk transfers in-tree.

> > > - Kqueue (maybe? I haven't tested it thoroughly yet)
> > 
> > Not sure if we should keep support it. If libusb's handle_event doesn't
> > need kqueue(2) support after your modifications then it can go.
> > 
> 
> It _could_ be used I suppose, since the libusb user could register to
> receive notification of all new fds, and use kqueue(2) rather than
> poll(2) to receive notification of completed operations. I was
> planning to support it, based on that.

Well you can discus that with libusb people.  They are really friendly
and I guess will be interested to offload a bit of their OpenBSD support
to somebody else ;)

Re: ugen(4) Asynchronous USB Requests

[Phil Vachon]

Hi Martin,

Thank you very much for the information and commentary!

On May 30, 2016 at 9:28:52 AM, Martin Pieuchot wrote:

> Hello Phil,
> 
> On 26/05/16(Thu) 14:55, Phil Vachon wrote:
> > Hi all,
> >
> > -- snip --
> 
> You should get in touch with Grant Czajkowski, he did something similar
> during the GSoC 2015, see:
> 
> https://www.google-melange.com/gsoc/project/details/google/gsoc2015/coolguy1253/5649050225344512
> 

Damn, now I wish I had looked harder before I embarked on all this. I
will reach out to Grant and see how we could maybe work together to
get this over the finish line to go into an upcoming release. It's
very helpful to have! I'm guessing Grant's contact info can be found
in the mailing list archives, so I'll have a look.

> >
> > Currently broken things:
> > - The entire synchronous interface (poll-related changes)
> 
> That's fine as long as libusb is update accordingly. But if it doesn't
> make sense to keep two set of ioctl(2) then, simply get rid of the old
> ones ;)
> 

Loud and clear. I'll see if we can keep the current interface as
intact as possible (based on your comment below), but if there are
'appendices' that remain, they will be removed.

> > - Cancelling pending transfers
> 
> That's the hard part of the problem. Grant's work includes a working
> solution, but modifying the HC driver for that should be avoided.
> 

This is one thing I'm not 100% understanding -- my plan (though Grant
has already gone through this) was to expose the pipe's abort()
method through a similar usbi_* function. I presumed this should be
callable safely at splusb(), since this is how a pipe goes about
cleaning itself up? There was a change to uhci(4) that seems to insert
a USB task for every transfer that is to be cancelled, is this the HC
driver change you're referring to?

I am guessing that my presumption that abort() is safely callable at
splusb() is wrong?

The other thing that I was trying to avoid in cancellation was having
complexities around identifying pending operations. Originally I
hacked something together using the address of the source/target
userspace buffer for identifying the operation. But the same buffer
could be reused for multiple transactions so this does not make
sense. The other idea I had was to provide a transaction 'handle'
returned by the USB_ASYNC_SUBMIT ioctl(2) that would then be used to
later reference the operation. Both approaches have lots of downsides,
but since libusb is already doing bookkeeping on the programmer's
behalf, the latter approach might not be too onerous.

> > - Isochronous transfers
> 
> That's not important in the first place.
> 

I didn't think so, but I was thinking of providing a kernel-managed
queue for isochronous transfers. Then userspace would populate the
queue with transfers to be submitted after the next isochronous
transfer completes. Something similar could be done for interrupt
endpoints too, which would allow for the timing 'requirements' to be
met more readily for such things. 

> > - Kqueue (maybe? I haven't tested it thoroughly yet)
> 
> Not sure if we should keep support it. If libusb's handle_event doesn't
> need kqueue(2) support after your modifications then it can go.
> 

It _could_ be used I suppose, since the libusb user could register to
receive notification of all new fds, and use kqueue(2) rather than
poll(2) to receive notification of completed operations. I was
planning to support it, based on that.

> > - Locking is somewhat heavy-handed in places
> 
> Please removing your locking code. All ioctl(2) are serialized by the
> KERNEL_LOCK(). The only problem is if your code can sleep, in which
> case the mutex you're using does not help.
> 

Perfect, will do. I guess I was worried about concurrent modification
of the queue of pending ops, but if all ioctl(2) are serialized as
such, then there's no sense in keeping any locking around. This was
one thing that I'm admittedly not comfortable with in OpenBSD, and
need to learn more about.

> > - There's a hard cap of 16 asynchronous operations in-flight per
> > endpoint at this time. This could be readily made configurable.
> 
> Nice.
> 
> > My current test cases are a trivial "smoke test" of the API, as well as 
> > using libusb and the libimobiledevice/usbmux and ipheth-pair tools. Of 
> > course, this requires a modified libusb, which can be found at:
> > https://github.com/pvachon/libusb - see the openbsd-6.0 branch
> 
> Nice, I'd really suggest you to look at Grant's work, hopefully with him
> because his work also solve some other issues, like timeout handling
> without deprecating the existing interface.
> 
> > and some light modifications to usbmuxd:
> > https://github.com/pvachon/usbmuxd
> 
> Nice, I'd suggest you to submit a port for usbmuxd :)
> 
> > Otherwise, upstream libimobiledevice, libusbmux, etc. can all be used 
> > as-is.
> >

I'll make ports for all of these, seems there are none that exist as of today.
Makes sense, since they didn't work before.

Re: ugen(4) Asynchronous USB Requests

[Martin Pieuchot]

Hello Phil,

On 26/05/16(Thu) 14:55, Phil Vachon wrote:
> Hi all,
> 
> In order to make iPhone USB tethering work with OpenBSD, I ended up 
> going a bit down the proverbial rabbit's hole.
> 
> After initially playing around with trying to build a userspace 
> 'driver' using a tap interface, I found that ugen(4) was entirely 
> blocking. I had started hacking around this using separate 
> reader/writer threads, but realized that ugen(4) could be fairly 
> readily made capable of submitting and tracking multiple asynchronous 
> operations on a single endpoint without too much effort.
> 
> Thus this patch was born. At this point, I'm really looking for 
> comments as to what the appetite would be for such a drastic change 
> (while attempting to maintain backwards compatibility for code that 
> depends on the existing behavior), as well as feedback on the interface 
> and code itself.

You should get in touch with Grant Czajkowski, he did something similar
during the GSoC 2015, see:

https://www.google-melange.com/gsoc/project/details/google/gsoc2015/coolguy1253/5649050225344512

> The major changes are:
>  - Asynchronous USB operations are supported through 3 new ioctls:
>   USB_ASYNC_SUBMIT - submit a new asynchronous operation
>   USB_ASYNC_CANCEL - cancel a pending operation (TBD)
>   USB_ASYNC_FINISH - retrieve the next completed operation
>    All three operations act on a new 'struct usb_async_op'.
>  - Control, Bulk and Interrupt endpoints all are supported through the
>    same API, with Control operations requiring the setup packet be in
>    the first bytes of the data packet.
> 
> Currently broken things:
>  - The entire synchronous interface (poll-related changes)

That's fine as long as libusb is update accordingly.  But if it doesn't
make sense to keep two set of ioctl(2) then, simply get rid of the old
ones ;)

>  - Cancelling pending transfers

That's the hard part of the problem.  Grant's work includes a working
solution, but modifying the HC driver for that should be avoided.

>  - Isochronous transfers

That's not important in the first place.  

>  - Kqueue (maybe? I haven't tested it thoroughly yet)

Not sure if we should keep support it.  If libusb's handle_event doesn't
need kqueue(2) support after your modifications then it can go.

>  - Locking is somewhat heavy-handed in places

Please removing your locking code.  All ioctl(2) are serialized by the
KERNEL_LOCK().  The only problem is if your code can sleep, in which
case the mutex you're using does not help.

>  - There's a hard cap of 16 asynchronous operations in-flight per
>    endpoint at this time. This could be readily made configurable.

Nice.

> My current test cases are a trivial "smoke test" of the API, as well as 
> using libusb and the libimobiledevice/usbmux and ipheth-pair tools. Of 
> course, this requires a modified libusb, which can be found at:
>     https://github.com/pvachon/libusb - see the openbsd-6.0 branch

Nice, I'd really suggest you to look at Grant's work, hopefully with him
because his work also solve some other issues, like timeout handling
without deprecating the existing interface.

> and some light modifications to usbmuxd:
>     https://github.com/pvachon/usbmuxd

Nice, I'd suggest you to submit a port for usbmuxd :)

> Otherwise, upstream libimobiledevice, libusbmux, etc. can all be used 
> as-is.
> 
> Any thoughts are appreciated.
> 
> Index: sys/dev/usb/ugen.c
> ===
> RCS file: /cvs/src/sys/dev/usb/ugen.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 ugen.c
> --- sys/dev/usb/ugen.c24 May 2016 05:35:01 -  1.94
> +++ sys/dev/usb/ugen.c26 May 2016 17:16:55 -
> @@ -10,6 +10,8 @@
>   * by Lennart Augustsson (lenn...@augustsson.net) at
>   * Carlstedt Research & Technology.
>   *
> + * Copyright (C) 2016 Phil Vachon 
> + *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions
>   * are met:
> @@ -50,61 +52,97 @@
>  #include 
>  #include 
>  
> +#define EPDEV(_ep) (&(_ep)->sc->sc_dev)
> +#define SCDEV(_sc) (&(_sc)->sc_dev)
> +
>  #ifdef UGEN_DEBUG
>  #define DPRINTF(x)   do { if (ugendebug) printf x; } while (0)
>  #define DPRINTFN(n,x)do { if (ugendebug>(n)) printf x; } while (0)
> +#define UPRINTF(_dev, msg, ...) do { (void)printf("%s: " msg "\n", 
> (_dev)->dv_xname, ##__VA_ARGS__); } while (0)
>  int  ugendebug = 0;
>  #else
>  #define DPRINTF(x)
>  #define DPRINTFN(n,x)
> +#define UPRINTF(...)
>  #endif
>  
>  #define  UGEN_CHUNK  128 /* chunk size for read */
>  #define  UGEN_IBSIZE 1020/* buffer size */
>  #define  UGEN_BBSIZE 1024
>  
> -#define  UGEN_NISOFRAMES 500 /* 0.5 seconds worth */
> -#define UGEN_NISOREQS6   /* number of outstanding xfer requests 
> */
> -#define