Hi,

On 05/13/2013 02:58 AM, Pete Batard wrote:
> Sooooo, instead of looking at Hans' patch, as I should have, I decided to 
> spend some time on BOS and EP Companions, with the result being the proposed 
> solution above.
>
> Seems to work nicely against an FX3 device (also tested against an USB 3.0 
> hub), as per the output sample at the end. Tested with Linux and Windows.
>
> Now, for the usual stream of notes:
>
> - Yes, I'm deviating from the libusb and freebsd implementation.  Why? 
> Because these sure could use some improvements.
 >  First, there's no way we want to expose a parse_descriptor function, as it 
 > doesn't align with anything we do

It is unfortunately that we will be deviating from what the freebsd 
implementation has done here,
but they have not even send a courtesy mail that they were doing this, and they 
are supposed to be
an implementation of libusb for freebsd, not us being an implementation of 
freebsd-libusb for all
other platforms.

And I agree that their implementation is not pretty, so I would like to suggest 
that we go our
own way.

With that said, I would have liked the new descriptor structures to match, so 
that they can add
our version if the API, while keeping their own. But they have picked some 
really inconsistent
non spec compliant names for various bits, which Pete has rightfully corrected 
in his patch,
so my conclusion is bad luck for the FreeBSD libusb people. And if any FreeBSD 
people are
reading along: Next time please coordinate API changes, and post API proposals 
for review
on the libusb[x]-devel list!

> and second, not having the EP companions being part of the EP descriptor 
> struct is just nonsense.

If we were to design this from scratch I would completely agree. But we're not 
and we have
to maintain ABI and API compatibility, so I've to strongly NACK this part of 
your patch:

@@ -531,6 +585,9 @@ struct libusb_endpoint_descriptor {
         /** For audio devices only: the address if the synch endpoint */
         uint8_t  bSynchAddress;

+       /** SuperSpeed Endpoint Companion descriptor */
+       struct libusb_ss_endpoint_companion_descriptor *ss_endpoint_companion;
+
         /** Extra descriptors. If libusbx encounters unknown endpoint descripto
          * it will store them here, should you wish to parse them. */
         const unsigned char *extra;

Put this on your things to cleanup for libusb-2.x list, because we are simply 
NOT going
to make this change in libusb[x]-1.x.y

Instead I suggest we add a:

int LIBUSB_CALL libusb_get_ss_endpoint_companion_descriptor(struct 
libusb_endpoint_descriptor *endpoint_descriptor, struct 
libusb_ss_endpoint_companion_descriptor **ss_endpoint_companion)

Which will search libusb_endpoint_descriptor.extra for an 
ss_endpoint_companion_descriptor
and return that, or an error if it is not found (no need to make the user do 
the searching
like the freebsd code does).

> - I played with caching the BOS. But unless we go querying the OS directly 
> for it (rather than issue a generic transfer as we're doing now), this isn't 
> gonna fly, as it'd mean opening each device in sequence, get a handle, and 
> the whole shebang (detach driver anyone?). So, as opposed to our other 
> get_###_descriptor() calls, get_bos_descriptor() _is_ blocking and will 
> submit traffic (and requires a handle rather than a dev). My previous 
> proposal did rely on the OS to query the BOS, so would have been more 
> suitable for caching, but of course that'd multiply implementations, so I 
> guess we'll just
> go for this common one. Maybe in 2.0 I'll switch to caching the BOS, and 
> align get_bos_desc() to take a dev rather than a handle. We'll see.
>

Agreed, the Linux kernel does cache the bos, but currently their does not seem 
to be a way
to get to the cached bos from userspace, so we need to do some work on the 
linux kernel side
there too, nothing I cannot write a patch for :)

No idea if the other OS-es do give us access to cached bos descriptors.

> - I'm issuing a double request for the BOS (header for size, then full BOS). 
> Yeah, we could get away with using a large enough buffer, since you'll be 
> hard pressed to find a BOS that's more than 128 bytes in size right now, but 
> hey, we're supposed to be generic where we can...

Ack.

> - Since libusb and BSD went with unrolling the BOS, I added the Container ID. 
> This is used on every single USB 3.0 HUB, and I'm pretty sure we'll be asked 
> by our users to enable access to it. Whenever there's an ID, there's somebody 
> who's going to want to read it. Of course, going with an unrolled BOS means 
> that whatever additional caps are added we'll have to add them in libusbx as 
> well. Oh, and just for the record, I haven't bothered with the Wireless cap.

Ack.

> - Since I discovered that my USB 3.0 hub uses the same VID:PID for the 2.0 
> and 3.0 hub sub-hubs (seen as 2 separate devices)

My usb-3 hub does that too, and it has the brilliant guid in its containerID of:
{00000000-0000-0000-0000-000000000000}

Now that is going to be so useful if I've more then 1 such hub ...

 > and of course the 2.0 one is the one that comes first, I'm thinking of 
 > introducing a libusb_open_with_vid_pid_instance() call, that would add an 
 > instance parameter that would be an entirely volatile number corresponding 
 > to the order in which libusbx sees devices with same VID:PID (with no 
 > guarantee whatsoever on that order being preserved between sessions).

I'm not enthusiastic about the notion of a libusb_open_with_vid_pid_instance(), 
it
feels too much like adding more duct-tape to a broken concept (opening by
vid-pid is inherently broken as vid-pid are not necessarily unique, as
this example clearly shows).

I'm not saying I would nack a patch adding this, but I don't think it is a good 
idea.

###

So my plan forward with the BOS / Endpoint Companion USB 3.0 descriptors is too
merge Pete's patch into my darwin-integration tree, with the part I NACK-ed 
above
fixed using the solution I suggested above.

Regards,

Hans

------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel

Reply via email to