Cherry-pick USB changes for release 3.3

[adamgreloch]

Description

Cherry-picks into the 3.3 release branch the following:

Minor device driver fixes:

• umass: access requests in FIFO order
• umass: use strncpy
• umass: remove intermediate buffer
• umass: fix umass_mountFromDev error paths
• umass/srv: wait for rootfs instead of console
• umass: decrease number of worker threads and make it configurable
• usbacm: use idtree for device id allocation
• usbacm: remove unused drvport, rename usbacm_freeAll to _usbacm_freeAll

USB rework, devinfo API addition:

• umass: fix snprintf not being checked for truncation
• usbacm: check condCreate return value
• umass: fix incorrect error handling and a resulting deadlock on device insertion
• usbacm: set line coding on device insertion
• usbacm: adapt to events API
• umass: adapt to events API
• storage/umass: handle quirky usb sticks
• _targets/imx*: build usb drivers in lib versions
• tty/usbacm: fix device insertion/deletion race conditions
• tty/usbacm: adapt to new usb API
• storage/umass: misra
• storage/umass: refactor to compile driver as static-lib
• storage/umass: use new procDrvRun API
• storage/umass: implement ext2 mounting
• usb/ehci: adapt hcd driver to ia32 hw
• usb/ehci: rename per-subfamily phy src to per-family-subfamily
• usb/ehci: adapt ehci to work on ia32
• usb/ehci: adapt to hcd_info_t struct change

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

• Already covered by automatic testing.
• New test added: (add PR link here).
• Tested by hand on: (list targets here).

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing linter checks and tests passed.
• My changes generate no new compilation warnings for any of the targets.

Special treatment

• This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  Backport USB rework and devinfo API addition to 3.3 release phoenix-rtos-usb#37
• I will merge this PR by myself when appropriate.

[gemini-code-assist]

Hello @adamgreloch, I'm Gemini Code Assist¹! 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

Hello team,

Gemini here, providing a summary for this pull request. This PR is a cherry-pick aimed at bringing significant USB-related changes, including a rework of the Mass Storage (UMASS) and CDC ACM (USBACM) drivers and the EHCI host controller driver, into the 3.3 release branch. It also incorporates additions related to a new USB event API and driver structure.

Highlights

• USB Driver Rework (UMASS & USBACM): The UMASS and USBACM drivers have been refactored to align with a new USB driver structure. This involves splitting the drivers into static libraries (libusbdrv-umass, libusbdrv-usbacm) and separate binary server components (umass, usbacm). The server components handle driver registration and main execution flow, while the core driver logic resides in the libraries.
• UMASS Driver Enhancements: The UMASS driver now includes definitions for the SCSI transparent command set, improved bulk transfer handling with retries and SCSI sense request support, and device management using an idtree instead of a linked list. It also introduces support for mounting filesystems on partitions, with specific compile-time support added for the ext2 filesystem.
• USBACM Driver Enhancements: The USBACM driver has been updated to support setting line coding parameters. Device management has been refined using a linked list with reference counting and a dedicated list for devices pending freeing.
• EHCI Host Controller Driver Updates: The EHCI driver has been refactored to work with a new HCD structure. Register access now explicitly uses the operational register base (opbase). Interrupt handling has been improved, including clearing specific status bits and handling system errors. QTD allocation logic has been adjusted, and the PHY layer initialization has been updated for various architectures (imx6ull, imxrt106x, imxrt117x, ia32). A new ia32-specific PHY layer implementation has been added, including PCI configuration and BIOS ownership handling.
• Target Makefile Updates: The Makefiles for the armv7a7-imx6ull, armv7m7-imxrt106x, and ia32-generic targets have been updated to include the newly introduced static libraries (libusbdrv-umass, libusbdrv-usbacm) in their default component lists.

Changelog

Click here to see the changelog

• _targets/Makefile.armv7a7-imx6ull
  • Added libusbdrv-umass and libusbdrv-usbacm to DEFAULT_COMPONENTS.
• _targets/Makefile.armv7m7-imxrt106x
  • Added libusbdrv-umass and libusbdrv-usbacm to DEFAULT_COMPONENTS for imxrt117x and imxrt106x targets.
• _targets/Makefile.ia32-generic
  • Added libusbehci, umass, and libusbdrv-umass to DEFAULT_COMPONENTS.
• storage/umass/Makefile
  • Updated copyright year to 2024.
  • Split umass into a static library libusbdrv-umass and a binary umass.
  • Added srv.c to the sources for the umass binary.
  • Added conditional dependency on libext2 for the ia32 target family.
• storage/umass/scsi.h
  • New file: Added definitions for SCSI transparent command set structures (scsi_cdb10_t, scsi_cdb6_t, scsi_sense_t, scsi_inquiry_t) and opcodes (SCSI_REQUEST_SENSE, SCSI_INQUIRY).
• storage/umass/srv.c
  • New file: Implemented the umass driver server entry point (main).
  • Added command line argument parsing for mounting as rootfs (-r).
  • Pops the registered USB driver and runs the driver process.
• storage/umass/umass.c
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Removed unused include limits.h (line 17).
  • Added includes for posix/idtree.h, libext2.h (conditional), umass.h, and scsi.h (lines 29-42).
  • Increased UMASS_N_MSG_THREADS to 2 and added UMASS_N_POOL_THREADS (4) (lines 44-45).
  • Added retry constants UMASS_TRANSMIT_RETRIES and UMASS_INIT_RETRIES (lines 47-48).
  • Added logging and debug macros (lines 62-65).
  • Defined function prototypes for device access, threads, and filesystem callbacks (lines 89-111).
  • Added structures for filesystem registration (umass_fs_t), partitions (umass_part_t), and request queue (umass_req_t) (lines 112-136).
  • Updated umass_dev struct to use idnode_t, include usb_driver_t *drv, and embed umass_part_t part (lines 140-155). Removed linked list pointers (prev, next), id, partOffset, partSize (lines 144, 153).
  • Updated umass_common struct to use idtree_t devices, add rqueue, rlock, rcond, mount_root, and rbtree_t fss (lines 160-170). Updated stack sizes (lines 173-176). Removed lastId (line 165).
  • Refactored umass_transmit to _umass_transmit and added retry logic and SCSI sense request handling (lines 217-286).
  • Added SCSI specific functions umass_scsiTest, umass_scsiRequestSense, umass_scsiInquiry, and _umass_scsiInit (lines 316-390).
  • Refactored umass_check to _umass_check (line 393).
  • Added filesystem registration function umass_registerfs (conditional) and comparison function umass_cmpfs (lines 185-211).
  • Added umass_getfs function (lines 428-435).
  • Renamed umass_read to umass_readFromDev and umass_write to umass_writeToDev (lines 458, 490). Added wrapper functions umass_read and umass_write that lookup device by id (lines 537, 549).
  • Refactored umass_devFind to _umass_devFind (line 531).
  • Added mount related functions umass_mountFromDev and umass_mount (lines 561, 607).
  • Added thread functions umass_fsthr and umass_poolthr for handling filesystem messages (lines 619, 656).
  • Updated umass_msgthr to handle mtMount message type and use the new read/write functions (lines 721-733).
  • Refactored umass_devAlloc to _umass_devAlloc using idtree (line 759). Added _umass_devFree (line 751).
  • Added umass_mountRoot function (conditional) (line 789).
  • Refactored umass_handleInsertion to take usb_driver_t and usb_event_insertion_t, use new dev alloc/free, add SCSI init, and add root mount logic (lines 812-904).
  • Refactored umass_handleDeletion to take usb_driver_t and use idtree iteration (lines 907-932).
  • Removed the main function (lines 438-496).
  • Added umass_init, umass_destroy, umass_driver struct, and umass_register constructor (lines 941-1040).
• storage/umass/umass.h
  • New file: Added common header for UMASS driver.
  • Defined umass_args_t struct for driver arguments.
• tty/usbacm/Makefile
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Split usbacm into a static library libusbdrv-usbacm and a binary usbacm.
  • Added srv.c to the sources for the usbacm binary.
• tty/usbacm/srv.c
  • New file: Implemented the usbacm driver server entry point (main).
  • Pops the registered USB driver and runs the driver process.
• tty/usbacm/usbacm.c
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Removed USBACM_N_UMSG_THREADS define (lines 39-41).
  • Added USBACM_SET_LINE_DEFAULT_RATE define (lines 55-57).
  • Updated usbacm_dev struct to include usb_driver_t *drv and usb_cdc_line_coding_t line (lines 97, 99). Removed linked list pointers (prev, next) (line 94).
  • Updated usbacm_common struct to remove ustack and add usbacm_dev_t *devicesToFree (lines 104, 110).
  • Updated _usbacm_rxStop to use dev->drv for usb_transferAsync (line 156).
  • Added usbacm_setLine function (lines 266-277).
  • Updated usbacm_handleCompletion to take usb_driver_t *drv and return int (line 280). Used drv for usb_transferAsync (line 329).
  • Updated _usbacm_rxStart to use dev->drv for usb_transferAsync (lines 350, 356).
  • Updated usbacm_write to use dev->drv for usb_transferBulk (line 425).
  • Updated usbacm_devAlloc to add the device to the list prematurely and initialize rfcnt (lines 454-456).
  • Updated _usbacm_urbsAlloc to use dev->drv for usb_urbAlloc and usb_urbFree (lines 469, 475, 478, 482).
  • Updated _usbacm_close to use dev->drv for usb_urbFree (lines 530, 539).
  • Updated usbacm_handleInsertion to take usb_driver_t *drv and usb_event_insertion_t *event (line 662). Used drv for usb_modeswitchHandle (line 675) and usb_open (lines 685, 699, 705, 712). Added call to usbacm_setLine (line 733). Populated event fields (lines 763-765). Added error handling and device removal on failure (lines 684-759).
  • Updated usbacm_handleDeletion to take usb_driver_t *drv (line 771). Used usbacm_common.devicesToFree (line 797). Added mutex lock/unlock (lines 781, 807).
  • Removed usbthr function (lines 744-775).
  • Removed the main function (lines 777-826).
  • Added usbacm_init, usbacm_destroy, usbacm_driver struct, and usbacm_register constructor (lines 813-878).
• usb/ehci/Makefile
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Updated LOCAL_SRCS for the PHY file name to include target family (phy-$(TARGET_FAMILY)-$(TARGET_SUBFAMILY).c) (line 9).
  • Added conditional compilation flag EHCI_IMX for imx targets (lines 11-13).
• usb/ehci/ehci-hub.c
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Added conditional compilation (#ifdef EHCI_IMX) for bDeviceProtocol in hub_devDesc (lines 42-47).
  • Updated ehci_resetPort to use ehci->opbase (line 95), added more detailed reset logic for non-IMX targets (lines 112-131), and added debug logs (lines 98, 127, 130).
  • Updated ehci_getPortStatus to use ehci->opbase (line 153) and added conditional speed reporting for IMX (lines 187-195).
  • Updated ehci_clearPortFeature to use ehci->opbase (line 232).
  • Updated ehci_getDesc to use ehci->base for hcsparams access (line 350).
  • Updated ehci_getHubStatus to use ehci->opbase (line 368) and added debug logs (lines 369, 374).
  • Updated ehci_roothubReq to use ehci->opbase for usbintr access (lines 388-389).
• usb/ehci/ehci.c
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Added include for stdbool.h (line 26).
  • Added conditional EHCI_PERIODIC_SIZE based on EHCI_IMX (lines 40-44).
  • Added conditional ehci_memDmb implementation for IMX/non-IMX (lines 53-57).
  • Updated ehci_startAsync and ehci_stopAsync to use ehci->opbase (lines 65, 68, 77, 79).
  • Added ehci_qtdDump debug function (lines 174-199).
  • Updated ehci_qtdAlloc to initialize buf_hi (lines 227, 235, 247) and use EHCI_QH_NBUFS (lines 233, 246).
  • Updated ehci_qhAlloc to initialize buf and buf_hi fields in qh->hw (lines 326-329).
  • Updated ehci_irqHandler to use ehci->opbase (lines 551, 553, 559), clear USBSTS_FRI (line 553), and removed the portsc update (lines 508-509 in old code).
  • Added ehci_printIrq debug function (conditional) (lines 641-660).
  • Updated ehci_irqThread to print debug IRQ status (conditional) (lines 673-675), handle USBSTS_SEI (lines 680-684), and clear status bits after handling (lines 681, 688, 695).
  • Updated ehci_qtdAdd to use 1 - dt for toggle bit logic (line 712).
  • Updated ehci_transferEnqueue condition for data stage (line 773).
  • Updated ehci_init to use log_error for error messages (lines 874, 879, 885, 893, 899, 905, 911, 917, 925). Added alignment check for hcd->base (lines 936-940). Set ehci->base (line 943). Determined and set ehci->opbase based on caplength or EHCI_IMX define (lines 945-954). Updated interrupt handling setup (lines 956-962). Added conditional controller halt/wait for non-IMX (lines 969-974). Updated controller reset command and wait (lines 978-980). Updated host mode setting conditional for IMX/non-IMX (lines 982-989). Updated interrupt enable to include USBSTS_SEI (line 992). Added conditional frame list size setting for IMX (lines 997-1000). Updated controller run command and wait (lines 1003-1007). Added sleep after config flag (line 1013). Added debug log at the end (line 1017).
• usb/ehci/ehci.h
  • Updated copyright year to 2024 and added author Adam Greloch.
  • Added debug defines (EHCI_DEBUG, EHCI_DEBUG_IRQ, EHCI_DEBUG_QTD) and logging macros (log_msg, log_error, log_debug) (lines 17-27).
  • Updated EHCI_INTRMASK to include USBSTS_SEI (line 44).
  • Added new USBCMD defines (USBCMD_RUN, USBCMD_HCRESET, USBCMD_PSE, USBCMD_LRESET) (lines 46-51).
  • Added HCCPARAMS_64BIT_ADDRS define (line 127).
  • Added conditional register enums based on EHCI_IMX (lines 139-175).
  • Added EHCI_QH_NBUFS define (line 191).
  • Updated qtd struct to include buf_hi array (line 187).
  • Updated qh struct to use EHCI_QH_NBUFS for buffer arrays and include buf_hi (lines 203-204).
  • Updated ehci_t struct to include volatile uint32_t *base and volatile uint32_t *opbase (lines 243-244). Added a comment about stack alignment (line 229).
• usb/ehci/phy-armv7a7-imx6ull.c
  • Updated hcd_info_t struct initialization to use a nested phy struct for address and clock (lines 38-41).
  • Updated phy_initClock and phy_disableClock to access clock via hcd->info->phy.clk (lines 91, 106).
  • Updated phy_init to access PHY address via hcd->info->phy.addr for mmap (line 127).
• usb/ehci/phy-armv7m7-imxrt106x.c
  • Updated hcd_info_t struct initialization to use a nested phy struct for address and clock (lines 37-40).
  • Updated phy_init to access PHY address via hcd->info->phy.addr and HCD address via hcd->info->hcdaddr (lines 107-108). Updated clock access to use hcd->info->phy.clk (line 110).
• usb/ehci/phy-armv7m7-imxrt117x.c
  • Updated hcd_info_t struct initialization to use a nested phy struct for address and clock for both OTG1 and OTG2 (lines 43-46, 55-58).
  • Updated phy_init to access clock via hcd->info->phy.clk (line 172) and PHY/HCD addresses via hcd->info->phy.addr and hcd->info->hcdaddr (lines 178-179).
• usb/ehci/phy-ia32-generic.c
  • New file: Implemented the EHCI PHY layer for ia32.
  • Defined constants for EHCI PCI class/progif and HCCPARAMS EECP offset (lines 28-31).
  • Implemented hcd_getInfo to scan PCI devices for EHCI controllers and populate hcd_info_t (lines 34-97).
  • Implemented phy_config to configure PCI device (busmaster, memory space, interrupts) and take controller ownership from BIOS if necessary (lines 100-155).
  • Implemented phy_enableHighSpeedDisconnect (empty function) (lines 158-160).
  • Implemented phy_init to mmap the HCD base address and call phy_config (lines 163-179).

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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request introduces substantial and important updates to the USB stack, including a rework of umass and usbacm drivers to a new event API, and enhancements to the EHCI driver with added ia32 support. The changes are largely well-structured and aim for improved robustness and maintainability.

Key improvements include:

• Refactoring of umass and usbacm drivers to use a new USB driver registration and process model.
• Enhanced error handling in umass, including SCSI sense data retrieval and command retries.
• More sophisticated threading model in umass using a request queue and worker pool.
• EHCI driver updates to support ia32, including PCI device discovery and ownership handoff, and better distinction between capability and operational registers.

While the core logic appears to be a significant step forward, there are critical areas related to resource management (especially in driver deinitialization) and thread robustness that need to be addressed. These are crucial for the stability expected in a release branch.

Summary of Findings

• Resource Cleanup in Driver destroy Functions: Both umass_destroy (in storage/umass/umass.c) and usbacm_destroy (in tty/usbacm/usbacm.c) are stubs or marked as TODO. Their respective init functions allocate resources (mutexes, condition variables, ports) and start threads. These must be properly cleaned up in the destroy functions to prevent resource leaks and ensure driver stability, especially if drivers can be unloaded or restarted. This is considered a high-priority issue.
• EHCI System Error Handling: In usb/ehci/ehci.c, the handling for USBSTS_SEI (System Error Interrupt) includes a TODO cleanup/reset after death. System errors can leave the controller in an undefined state, and robust handling (e.g., attempting a controller reset) is crucial for stability. This is a high-priority concern for a release branch.
• Thread Behavior on malloc Failure: In storage/umass/umass.c, the umass_fsthr thread continues in a loop without delay if malloc fails. This could lead to busy-waiting. Adding a short sleep would be beneficial.
• Potentially Unused Port in usbacm: The usbacm_common.drvport in tty/usbacm/usbacm.c appears to be a leftover from the previous driver model and might be unused. If confirmed, it should be removed.

Merge Readiness

This pull request introduces significant and valuable improvements to the USB stack. However, due to the critical nature of the identified issues related to resource management in driver destroy functions and EHCI system error handling, I recommend that these high-severity issues be addressed before merging into the 3.3 release branch. The medium-severity items should also be considered for improvement.

As an AI, I am not authorized to approve pull requests. Please ensure these changes are thoroughly tested and reviewed by other maintainers before merging.

[nalajcie]

throws exception on DTR project, introduces additional warning - please fix:

[adamgreloch]

Corrected regression on DTR and reverted the change that introduced the warning.

[anglov]

LGTM +2