Skip to content
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

[Bug]: Improve HID Descriptor Parsing Compatibility #338

Closed
1 task done
makubacki opened this issue Oct 19, 2023 · 1 comment · Fixed by #339
Closed
1 task done

[Bug]: Improve HID Descriptor Parsing Compatibility #338

makubacki opened this issue Oct 19, 2023 · 1 comment · Fixed by #339
Labels
state:needs-triage Needs to triaged to determine next steps type:bug Something isn't working urgency:low Little to no impact

Comments

@makubacki
Copy link
Member

Is there an existing issue for this?

  • I have searched existing issues

Current Behavior

Recently support was added in HidPkg for a Rust-based HID stack in #324.

It's been observed that a USB stall error is encountered with HID devices in some cases. We suspect those devices do not implement the full set of descriptor reads from the HID specification.

An example of such a problem on the QEMU emulated platform:

XhcCheckUrbResult: STALL_ERROR! Completecode = 6
Recovery Halted Slot = 1,Dci = 1
XhcResetEndpoint: Slot = 0x1, Dci = 0x1
XhcControlTransfer: error - Device Error, transfer - 2
[UsbGetFullHidDescriptor] unexpected failure reading HID descriptor: Device Error, Status: 0x2

ASSERT_EFI_ERROR (Status = Device Error)
ASSERT [UsbHidDxe] HidPkg\UsbHidDxe\UsbHidDxe.c(726): !(((INTN)(RETURN_STATUS)(Status)) < 0)

That QEMU Q35 platform can be used to verify changes.

Expected Behavior

A USB stall error should not be encountered as frequently trying to read HID descriptors from HID class devices.

At least in the case of QEMU, we found that (hw/usb/dev-hid.c) all descriptor type requests from the HID 1.1 specification are not supported.

image

Steps To Reproduce

  1. Boot QemuQ35Pkg firmware with default settings using the --FlashRom command
  2. Review the boot log in Build/BUILDLOG_QemuQ35Pkg_Run.txt (boots to EFI shell by default)

Build Environment

- OS(s): N/A
- Tool Chain(s): N/A
- Targets Impacted:
  - QEMU virtual platform

Version Information

- PR: #324
- Mu Plus Release v2023020002.0.2

Urgency

Low

Are you going to fix this?

Someone else needs to fix it

Do you need maintainer feedback?

No maintainer feedback needed

Anything else?

No response

@makubacki makubacki added type:bug Something isn't working state:needs-triage Needs to triaged to determine next steps labels Oct 19, 2023
@github-actions github-actions bot added urgency:low Little to no impact state:needs-owner Needs an issue owner to be assigned labels Oct 19, 2023
@makubacki
Copy link
Member Author

Assigning to @joschock to investigate reworking some of the retrieval logic.

@makubacki makubacki removed the state:needs-owner Needs an issue owner to be assigned label Oct 19, 2023
makubacki pushed a commit that referenced this issue Oct 20, 2023
Resolves #338 

## Description

Updates the algorithm used to read the HID descriptor from HID devices.
Empirical testing indicates that some devices do not support reading the
HID descriptor via the class-specific Get_Report() method described in
USB HID 1.11. This changes the HID read to read the entire configuration
descriptor and parse the HID descriptor out of the larger structure, and
gives compatibility with a broader range of devices.

- [x] Impacts functionality?
Supports a broader range of devices.
- [ ] Impacts security?
- [ ] Breaking change?
- [ ] Includes tests?
- [ ] Includes documentation?

## How This Was Tested

Verified against emulated USB devices in QEMU.

## Integration Instructions

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:needs-triage Needs to triaged to determine next steps type:bug Something isn't working urgency:low Little to no impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant