Backport USB rework and devinfo API addition to 3.3 release #37
Conversation
JIRA: RTOS-937
The reply data was not copied by the responder to the buffer address provided by the sender. JIRA: RTOS-937
This allows usb to be loaded from syspage (e.g. so that umass can be used for rootfs mounting) JIRA: RTOS-937
drvBind always succeeded even when no driver has been found for a device JIRA: RTOS-937
drvBind races with drvAdd. This change ensures that devices won't be orphaned if added by hcd before adding the corresponding driver in the single-driver scenario. This does not fix the race in case of multi-driver scenario, though, hence the "orphaned" todo is left untouched JIRA: RTOS-937
JIRA: RTOS-937
…able JIRA: RTOS-937
JIRA: RTOS-937
Every driver talking to usb host implements some sort of this handling loop, so it's better to move it to libusb to eliminate redundancy. This is also a first step in making the driver logic expressible via handler functions API only JIRA: RTOS-970
It is now possible to substitute driver-side pipe operations (e.g. the hostdriver can now provide its own implementations that does not use msg API and call the host functions directly) JIRA: RTOS-970
This makes it possible to run drivers straight from the usbhost process thereby allowing for various optimizations based on shared memory between device driver and host driver -- ability to bypass msg-passing, as included in this change, being a prime example. It also moves out procdrv implementation details to seperate file from libusb/driver.c JIRA: RTOS-970
Adds options to exclude external driver logic from the usbhost driver if for instance no such driver is ever expected to be run. JIRA: RTOS-970
If hcd became operational before returning from hcd init, it could try to access hcd->roothub while it was still uninitialized JIRA: RTOS-970
JIRA: RTOS-1024
Drivers can now return some useful information back to the usbhost about the state of inserted device JIRA: RTOS-1024
JIRA: RTOS-1024
JIRA: RTOS-1024
JIRA: RTOS-1024
Some devices store information in device descriptors in ASCII (even though string descriptors are to be treated as UTF16 per USB spec...), in which case converting from UTF16 to ASCII no matter what would cause loss of information JIRA: RTOS-1024
JIRA: RTOS-1024
Multifunction (multi-interface) devices may be served by multiple drivers JIRA: RTOS-1024
The usbhost now also creates symlinks to these driver devices named `/dev/usb-<vid>-<pid>-if<ifnum>" that can be looked up on and resolved via `canonicalize_file_name(path)` to acquire the needed oid. JIRA: RTOS-1024
Not resetting caused subsequent msgSend calls to send the modified and incorrect messages JIRA: RTOS-1024
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @adamgreloch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
As gemini-code-assist, I've reviewed this pull request which backports a significant rework of the USB stack and introduces a new event-driven API to the 3.3 release branch. This involves a major refactoring of the core USB host logic and the user-space library (libusb) to support both in-process (internal) and out-of-process (external) USB drivers. A new API for retrieving detailed device information by OID has been added, along with automatic device symlink creation upon driver binding. The changes are marked as breaking, as existing user-space USB drivers will need to be updated to use the new API and driver model.
Highlights
- New Driver Model and Event API: Introduces a new event-driven API and a refactored driver model in
libusband the USB host process. This allows for USB drivers to run either directly within the host process (internal) or as separate user-space processes (external), communicating via messages. The core USB functions now take ausb_driver_tparameter to support this. - Process-based Driver Support: Adds
libusb/procdriver.candlibusb/include/usbprocdriver.hto provide the necessary infrastructure for USB drivers running as separate processes. This includes message handling threads and functions to communicate with the USB host process. - Device Information API: Adds a new API (
libusb/devinfo.c,libusb/include/usbdevinfo.h) allowing user-space applications to retrieve detailed device information (descriptors, strings) using the device's OID. - Automatic Device Symlinks: The USB host process now automatically creates symlinks in
/devfor bound USB interfaces in the format/dev/usb-VID-PID-ifIFACE, making it easier to access specific devices from user-space. - Refactored Device and Driver Management: Significant changes in
usb/dev.candusb/drv.cto manage devices and drivers under the new model, including handling device OIDs, string descriptors (with fallbacks), and the binding/unbinding process for both internal and external drivers. - Updated Logging: New logging headers (
libusb/log.h,usb/log.h) and macros (log_info,log_msg,log_trace,log_error) are introduced and used throughout the codebase for better debugging and information output.
Changelog
Click here to see the changelog
- libusb/Makefile
- Added new source files:
procdriver.c,internal.c,devinfo.c. - Removed wildcard header inclusion.
- Updated copyright year to 2025.
- Added new source files:
- libusb/devinfo.c
- New file implementing
usb_devinfoGetto query device information from the host via messages.
- New file implementing
- libusb/driver.c
- Removed old
usb_connectandusb_eventsWaitfunctions. - Added
usb_driverRegisterandusb_registeredDriverPopfor driver registration. - Modified core USB transfer/configuration functions (
usb_open,usb_urbAlloc,usb_transferControl, etc.) to accept ausb_driver_t *and delegate operations viapipeOps.
- Removed old
- libusb/include/usb.h
- Added
#define USB_CLASS_HID 0x3.
- Added
- libusb/include/usbdevinfo.h
- New file defining
usb_devinfo_desc_tand declaringusb_devinfoGet.
- New file defining
- libusb/include/usbdriver.h
- New file defining core structures for the new API:
usb_driver_t,usb_pipeOps_t,usb_handlers_t, message types (usb_msg_t), and event structures (usb_event_insertion_t). - Declares the new API functions, many taking
usb_driver_t *.
- New file defining core structures for the new API:
- libusb/include/usbprocdriver.h
- New file declaring
usb_driverProcRunfor process-based drivers.
- New file declaring
- libusb/internal.c
- New file implementing
usb_hostLookupto find the USB host device node.
- New file implementing
- libusb/log.h
- New file defining logging macros for
libusb.
- New file defining logging macros for
- libusb/procdriver.c
- New file implementing the process-based driver framework.
- Defines
usbprocdrv_pipeOpsto send messages to the host. - Implements
usb_threadto handle host messages (insertion, deletion, completion). - Implements
usb_connectto register the driver with the host. - Implements
usb_driverProcRunas the entry point for process drivers.
- libusb/usbinternal.h
- New file declaring
usb_hostLookup.
- New file declaring
- usb/Makefile
- Added
libusband$(USB_HOSTDRV_LIBS)toLIBS. - Added
libusbtoDEPS. - Added conditional CFLAG
-DUSB_INTERNAL_ONLY.
- Added
- usb/dev.c
- Added includes for
limits.h,unistd.h,sys/rb.h,log.h. - Introduced
usb_devOid_tandusbdev_common.devOidsto track driver-created device OIDs. - Added
recipientfield tousb_transfer_tinitialization. - Modified string handling to use
usb_lenStr_tand added fallback string functions. - Added
usb_utf16ToAsciihelper. - Implemented
usb_devSymlinksCreateandusb_devSymlinksDestroyfor/devsymlinks. - Added
usb_devOnDrvBindCbcallback for driver binding events. - Updated
usb_devEnumerateto use new string handling, logging, and driver binding with callback. - Added
_usb_devOidFindandusb_devFreeOids. - Modified
usb_devUnbindto free OIDs and destroy symlinks. - Added
usb_devFindDescFromOidto find device info by OID. - Modified
usb_devDisconnectedto accept asilentflag and callusb_devUnbind.
- Added includes for
- usb/dev.h
- Added
usb_lenStr_tstruct. - Changed
usb_iface_t::strtoname(typeusb_lenStr_t). - Changed
usb_iface_t::drivertype tostruct usb_drvpriv *. - Added
usb_dev_oid_tstruct. - Added linked list pointers (
next,prev) tousb_dev_t. - Changed string fields in
usb_dev_ttousb_lenStr_t. - Changed
usb_dev::statusTransfertype tousb_transfer_t *. - Modified
usb_devDisconnectedsignature. - Added
usb_devFindDescFromOiddeclaration.
- Added
- usb/drv.c
- Added includes for
sys/minmax.h,log.h. - Replaced internal usage of
usb_drv_twithusb_drvpriv_t. - Added
drvAddedCondfor driver registration signaling. - Modified driver finding, adding, and pipe/transfer functions to work with
usb_drvpriv_t. - Modified
usb_drvMatchIfaceto wait for drivers if none are registered. - Modified
usb_drvBindto accept a callback and handle internal/external driver insertion. - Modified URB handling functions (
_usb_handleUrbcmd,usb_handleUrbcmd,_usb_handleUrb) to useusb_drvpriv_tand handle recipients. - Added
usb_drvInitto initialize locks/conds. - Added
usblibdrv_*functions andusblibdrv_pipeOps/usblibdrv_transferOpsfor internal driver handling. - Added
usb_libDrvInitandusb_libDrvDestroy.
- Added includes for
- usb/drv.h
- Added
PORT_INTERNALdefine. - Added
usb_drvOnBindCb_ttypedef. - Defined
struct usb_drvprivto distinguish internal/external drivers. - Updated function declarations to use
usb_drvpriv_t *. - Added declarations for
usb_libDrvInitandusb_libDrvDestroy.
- Added
- usb/hcd.c
- Added include for
log.handUSB_LOG_TAG. - Modified
hcd_roothubInitto returnEOK. - Modified
hcd_initto initialize roothub before HCD init and enumerate devices after, updating logging.
- Added include for
- usb/hcd.h
- Modified
hcd_info_tto use a union for physical address or PCI device ID. - Changed
hcd_t::baseandhcd_t::phybasetypes tovolatile uint32_t *.
- Modified
- usb/hub.c
- Removed
sys/msg.hinclude. - Added include for
log.handUSB_LOG_TAG. - Added
HUB_TT_POLL_DELAY_MSdefine. - Added
hub_common.ttsfor tracking TT hubs. - Added
recipientfield tousb_transfer_tinitialization inhub_interruptInit. - Added
hub_isTT,hub_ttAdd,hub_ttRemove. - Renamed
hub_polltohub_requestStatus. - Added
hub_devDisconnecthelper. - Modified
hub_devConnectedandhub_connectstatusto usehub_devDisconnectand updated logging. - Added
hub_ttStatusto poll TT hubs. - Modified
hub_threadto callhub_ttStatusperiodically. - Modified
hub_confto check for TT hubs and add them to the list, and callhub_requestStatus.
- Removed
- usb/hub.h
- Added hub protocol defines (
USB_HUB_PROTO_ROOT,USB_HUB_PROTO_SINGLE_TT).
- Added hub protocol defines (
- usb/log.h
- New file defining logging macros for the USB host process.
- usb/usb.c
- Added includes for
sys/minmax.h,log.h. - Added
usb_common.ustack. - Changed
usb_common.drvstype tousb_drvpriv_t *. - Added
usb_internalDriverInitfor host-side drivers. - Changed
usb_transferCheckreturn type tobool. - Modified
usb_transferSubmitto checkrecipientand initializefinished. - Modified
usb_transferFinishedto usebool urbtrans, checkrecipient, uset->opscallbacks, and handle internal/external transfers. - Removed old
usb_statusthrimplementation. - Modified
usb_msgthrto handleusb_msg_devdesc, use logging, and controlmsgRespondwith a flag. - Added
usbprocdrv_transferOpsandusbprocdrv_transferOpsGet(conditional). - Added new
usb_statusthrimplementation usingt->ops. - Modified
mainto initializeusb_devInit, initialize registered internal drivers, thenhub_init,hcd_init, create/dev/usb, and start threads. - Added
usblibdrv_openimplementation.
- Added includes for
- usb/usbhost.h
- Removed
sys/msg.hinclude. - Included
usbdriver.h. - Changed
usb_transfer::drvtype tostruct usb_drvpriv *. - Added
usb_transfer_tforward declaration. - Added
usb_transferOps_tstruct. - Added declarations for
usblibdrv_transferOpsGetandusbprocdrv_transferOpsGet. - Changed
usb_transfer::finishedtype tovolatile bool. - Added
usb_transfer::recipientfield. - Added
usb_transferunion for internal/external data. - Added
usb_transfer::opsfield. - Removed old
USB_LOGmacro. - Changed
usb_transferCheckreturn type tobool. - Added declaration for
usblibdrv_open.
- Removed
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request backports a significant USB rework and event API addition to the 3.3 release branch. The changes are extensive, introducing new structures for USB drivers, new APIs, and refactoring existing code to support different driver models (internal library-based and external process-based). The architectural changes, like the introduction of usb_driver_t and pipeOps, are positive steps towards modularity.
However, there are a few issues that need attention, particularly a potential build problem in the libusb Makefile and some correctness/consistency concerns detailed below.
Summary of Findings
- Build Configuration: The
libusb/Makefilehas aLOCAL_CFLAGSsetting that might prevent headers inlibusb/include/from being found correctly during compilation, potentially leading to build failures. - Copyright Year: Several new files have the copyright year set to 2025, which should be corrected to the actual year of creation or last significant modification.
- String Conversion: The UTF-16 to ASCII conversion in
usb/dev.cis basic and will lead to data loss for non-ASCII characters in device strings.
Merge Readiness
This pull request introduces significant and valuable changes to the USB stack. However, due to the potential build issue identified in libusb/Makefile (high severity) and other medium severity concerns, I recommend addressing these points before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members after addressing the feedback.
| static unsigned int usb_utf16ToAscii(char *dest, const char *src, unsigned int len) | ||
| { | ||
| unsigned int asciilen = len / 2; | ||
| int i; | ||
|
|
||
| if (len < 2) { | ||
| return 0; | ||
| } | ||
|
|
||
| for (i = 0; i < asciilen; i++) { | ||
| dest[i] = src[i * 2]; | ||
| } | ||
|
|
||
| dest[asciilen] = 0; | ||
|
|
||
| return asciilen; | ||
| } |
There was a problem hiding this comment.
The usb_utf16ToAscii function converts UTF-16 strings to ASCII by taking the low byte of each UTF-16 character (dest[i] = src[i * 2];). This approach will correctly convert characters within the standard ASCII range (U+0000 to U+007F).
However, for any characters outside this range (e.g., accented letters, symbols, characters from non-Latin scripts), this conversion will result in data loss or incorrect representation. While this might be a common simplification in embedded contexts, it could lead to garbled display of manufacturer, product, or serial number strings if they use non-ASCII characters.
Is this limitation acceptable, or should a more robust UTF-16 to a supported character set (like UTF-8, or a locale-specific encoding if applicable) conversion be considered for broader international character support, if feasible within the system constraints?
adamgreloch
changed the title
Description
Fast forwards USB rework and devinfo API addition into the 3.3 release branch.
Motivation and Context
Types of changes
Same as in:
usb: Notify apps about usb device insertion #36
Rework USB architecture #35
How Has This Been Tested?
Checklist:
Special treatment
Cherry-pick USB changes for release 3.3 phoenix-rtos-devices#559