-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for SuperSpeed+ Capability Descriptors #1428
Conversation
f511795
to
aaea055
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the one issue. LGTM. Will approve once it is fixed.
@tormodvolden This is simple enough it can probably go into 1.0.27. I don't see any issues with the APIs. Needs a second set of eyes though. |
I pinged the author to see about getting these resolved. If they can get to it soon it is worth considering for 1.0.27 otherwise let's put it in 1.0.28. |
We shouldn't try to rush such API changes into 1.0.27. This was proposed even after the RC. It seems the code has been reviewed, but has the API itself been scrutinized? I see no discussion in #1429 where the author suggests alternatives. Code we can fix after the release also, new APIs not so much. |
In #1429 the author even says this is a WIP PR. |
I have addressed the actionable comment. As for the API, I ended up going with something in line with the way |
3193c47
to
91a07d6
Compare
I have addressed the comments:
|
You underestimate the compiler. :)
|
I also find the macro shorter and easier to read. See the difference:
|
I disagree. But wait. More importantly, does this need byte swapping or not? The code I originally commented on does no byte swapping: uint32_t* attributes_pointer = (uint32_t*)(((uint8_t*)dev_cap) + LIBUSB_BT_SSPLUS_USB_DEVICE_CAPABILITY_SIZE); memcpy() would also not swap. But Do we want swapping here or not? We have to be clear on that first. |
Yes we need to swap. I added parsing for the descriptor in xusb example. |
We need to byteswap here because we are parsing LE data and putting it into a value. If it is not byte swapped it will be wrong on BE systems. |
examples/xusb.c
Outdated
printf(" attributes : %02X\n", ssplus_usb_device_cap->bmAttributes); | ||
printf(" supported functionality: %04X\n", ssplus_usb_device_cap->wFunctionalitySupport); | ||
|
||
int num_sublink_attributes = (ssplus_usb_device_cap->bmAttributes & LIBUSB_SSPLUS_ATTRIBUTE_SSCA_MASK) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love having this as a variable, but bmAttributes
is unsigned, so num_sublink_attributes
and i
should also be too (unint32_t
).
libusb/libusb.h
Outdated
/** An array of sublink speed attributes. See \ref bmAttributes for | ||
* the number of attributes (ssac+1). | ||
*/ | ||
uint32_t* bmSublinkSpeedAttr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another thought here: could this be a flexible array member, since it's at the end and variable length? Because then we could tag its length a la #1409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no opinion about this. I will let the admin decice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the comments above this struct you say "This descriptor is documented in section 9.6.2.5 of the USB 3.1 specification". Does it mean it has to match some preexisting/predefined structure?
The fact that modern compilers can do some range checking of flexible array members is very valuable, and I think makes it preferable over a raw uint32_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, did this go away? I don't seem to find "bmSublinkSpeedAttr" anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is renaned. I asked what the maintainer preferred and they opted for a struct exposing parsed values (vs raw values from the descriptor) so we don't need to stick to spec names.
d6d48c6
to
260dc2b
Compare
I find out that my Dell WD19TB Thunderbolt Docking Station supports 10Gbps. Unfortunately I can not test under Windows because of another issue, even after I change the driver to WinUSB. There is a draft patch from @tormodvolden to address #1302, I will see if I can adopt that patch here. But it does not seem to be easy... Maybe I should test the dock under Linux. |
Reference output from USBview (Microsoft) and USB Device Tree Viewer.
|
And this is the output from Linux (Ubuntu 20.04). This PR seems to work fine.
Linux lsusb output for reference as well.
|
Thank you so much for the test @mcuee ! It looks like there was only one problem with |
Sorry but what does this The new test results:
|
This is a field from the SuperSpeedPlus USB Device Capability (see specs)
|
Tested under macOS 14.3.
|
So there is a segfault issue under macOS. libusb 1.0.27 release does not have this issue.
|
Debug log using Apple Address Sanitizer.
|
I am unable to repro with the devices I have. I have enabled ASAN but no error is being reported either. I wonder if this is actually being introduced by this PR (I don't see the ssplus descriptor in ASAN report). Since you are able to repro, can you try to change From
to
It should completely disable all ssplus stuff and tell us if this PR is responsible or not. |
Indeed this has nothing to do with this PR. Rather, I believe this is because this PR is not catching up with git and thus affected by issue #1386. I will rebase to git and then check again later. I do not have the docking station with me now. Note to myself -- from the suggestion by @tormodvolden.
|
I also rebased by PR. |
Thanks. Now it works fine under macOS 14.3.1 (Mac Mini M1 2020).
Full debug info just for reference
|
@hjelmn has approved this PR. Please review as well. Thanks. |
Anything else blocking merging this CL? |
See specs in USB 3.1 specs in section: 9.6.2.5 SuperSpeedPlus USB Device Capability
@hjelmn Just replied and he is fine with the interface. |
See specs in USB 3.1 specs in section:
9.6.2.5 SuperSpeedPlus USB Device Capability
Fix issue #1429