MdeModulePkg/XhciDxe: Adjust out-of-range bInterval values #10829
Conversation
jackp780
force-pushed
the
xhcidxe-binterval-adjust
branch
from
0311ace to
5edacea
Compare
jackp780
force-pushed
the
xhcidxe-binterval-adjust
branch
from
5edacea to
5011bd1
Compare
jackp780
force-pushed
the
xhcidxe-binterval-adjust
branch
from
5011bd1 to
2e11101
Compare
|
The change is good to me. |
|
@mergify refresh |
✅ Pull request refreshed |
|
@leiflindholm We made a Mergify config change end of last week that required one additional configuration action that was completed this morning. That should be resolved now and I will monitor. With this change, rebase operations should occur without having to monitor going forward. |
Thanks. I saw #10861 go through and figured I was good to requeue :) |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated You should update or rebase your pull request manually. If you want to requeue this pull request, you can post a |
|
@mergify refresh |
✅ Pull request refreshed |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated You should update or rebase your pull request manually. If you want to requeue this pull request, you can post a |
|
@jackp780 you will have to make your branch writable to the project's maintainer if you want us to merge this. (I have always found this to be a rather dubious requirement, but it is how things work at the moment. Mergify merges the latest master into your branch to perform the CI checks, and this only works if your branch is writable for us) |
It says "Maintainers are allowed to edit this pull request.", I think Mergify (or github) is having a bad day. |
Currently the bInterval value must be calculated differently based on whether the endpoint type is isochronous or interrupt, and whether the device is low, full, high or super speed. Plus, this is duplicated for both XhcInitializeEndpointContext() and XhcInitializeEndpointContext64(). To reduce code complexity and duplication, and for future ease of maintenance, factor this logic out to a separate CalculateInterval() helper function. Signed-off-by: Jack Pham <[email protected]>
When a USB device is enumerated it will report one or more endpoint descriptors which contains a bInterval field which specifies the interval a host should periodically poll for that particular endpoint when scheduling transfers. But the units this value is expressed in (in whole ms, a power of 2 x 1ms, or a power of 2 x 125us) may differ depending on the speed of the device and whether the endpoint is isochronous or interrupt. Some high/super-speed devices, which are supposed to report isoc/int bInterval as a power of 2 x 125us, incorrectly report full-speed bInterval values (that is, in whole units of ms) in their interrupt endpoint descriptors which results in an assertion error due to being out of range of the spec-expected values. Rather than asserting, try to adjust those assuming they were expressed in units of ms with an upper limit of 128ms. Signed-off-by: Jack Pham <[email protected]>
jackp780
force-pushed
the
xhcidxe-binterval-adjust
branch
from
2e11101 to
4dae4f2
Compare
|
@jackp780, do you have branch permissions set on your fork? |
|
@mdkinney, I believe the issue is that the bot account |
You do not need to do anything further. I just wanted to confirm permission there are not more strict than the defaults. |
Description
Some high/super-speed devices incorrectly report full-speed bInterval values in their interrupt endpoint descriptors which results in an assertion error due to being out of range of the spec-expected values.
Rather than asserting, try to adjust those assuming they were expressed in units of ms with an upper limit of 8ms.
Inspiration was taken from the Linux kernel which has long had similar workarounds in its USB core (links to git.kernel.org):
An example device showing incorrect interval values is the Lenovo L32p-30 monitor, which includes an embedded USB hub with built-in peripherals. Attached is an output of the UsbTreeView utility which shows all its descriptor information of the monitor's hub as well as its embedded devices. In particular it contains a "Realtek HID" device which connects as a high-speed USB peripheral and reports an interrupt endpoint descriptor with bInterval=0x16.
lenovo_monitor_usbtreeview.txt
These changes were internally reviewed within Qualcomm by @leiflindholm who had suggested the additional refactoring.
How This Was Tested
This was tested by connecting the host to the USB-C port of the aforementioned monitor (Lenovo L32p-30) device which was triggering the assertion error without this fix, and verified that the fix allowed for successful enumeration.
Integration Instructions
N/A