Dev #59
Dev #59
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors hub and device handling in the xHCI backend to use a richer topology representation and the Speed enum throughout, and introduces hub-depth and TT (Transaction Translator) awareness for LS/FS devices behind hubs.
Changes:
- Adjusts
usb_if::host::hub::Speedutilities, including decoding from wPortStatus and mapping to xHCI slot values. - Replaces the old
RouteStringabstraction with aHub/HubInfomodel that tracks parent hub, depth, port, speed, and TT characteristics, and reworks device addressing (DeviceAddressInfoand xHCI device enumeration) to derive route strings and TT info from this topology. - Updates hub backends (
HubOp,XhciRootHub,HubDevice), kmod core (Core), and Cargo test aliases to align with the new model and support hub-depth configuration for SuperSpeed hubs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
usb-if/src/host/hub.rs |
Adjusts USB2 hub status decoding (from_usb2_hub_status) and refines Speed ↔ xHCI value mappings; note the super-speed bitmask change. |
usb-host/src/backend/ty/mod.rs |
Simplifies DeviceOp::update_hub and HubParams by removing the hub speed field, since hub speed is now tracked via HubInfo. |
usb-host/src/backend/kmod/xhci/hub.rs |
Updates XhciRootHub to the new HubOp::init(HubInfo) -> HubInfo API and sets the root hub speed to SuperSpeedPlus. |
usb-host/src/backend/kmod/xhci/host.rs |
Cleans up new_device by removing route-string-specific debug output that no longer applies. |
usb-host/src/backend/kmod/xhci/device.rs |
Changes Device.port_speed from raw u8 to Speed, updates interval calculation to match on Speed, and reworks address/TT logic to use HubInfo topology and HubParams. |
usb-host/src/backend/kmod/mod.rs |
Redefines DeviceAddressInfo to carry parent_hub, port_id, and a BTreeMap<Id<Hub>, HubInfo> instead of a RouteString and precomputed TT info; fixes the test-dwc alias to run the test_dwc test in the test_hub crate. |
usb-host/src/backend/kmod/kcore.rs |
Introduces Core::hub_infos() and rewires device probing to build DeviceAddressInfo from PortChangeInfo and HubInfo, and to construct Hub instances with topology-aware HubInfo. |
usb-host/src/backend/kmod/hub/mod.rs |
Replaces the old RouteString type with the Hub struct and HubInfo/UsbTt data structures, and updates HubOp to return an updated HubInfo from init. |
usb-host/src/backend/kmod/hub/device.rs |
Extends hub configuration to derive TT think time/multi-TT from descriptors, build HubParams, and, for SuperSpeed external hubs, send a SET_HUB_DEPTH control request; integrates with the new HubInfo model. |
.cargo/config.toml |
Points the test-dwc alias at the test_hub crate’s test_dwc test target so the alias actually runs existing tests. |
Comments suppressed due to low confidence (1)
usb-if/src/host/hub.rs:283
from_usb2_hub_status's implementation and documentation no longer match and the new mask0x0600makes theSuperSpeedbranch effectively unreachable. The comment above states that bit 11 (0x0800) indicates SuperSpeed, but the code now checksraw & 0x0600, which overlaps the LS/HS bits already handled in the earlierif/else ifbranches, so this condition will never be true for valid wPortStatus values and SuperSpeed ports will be incorrectly reported asFull. Please restore a correct mask (e.g.0x0800) or otherwise align the bit decoding with the USB 2.0 spec, and keep the comment in sync with the implementation.
/// 根据 USB 2.0 规范（第 11.24.2.7 节）：
/// - Bit 9 (0x0200): Low Speed
/// - Bit 10 (0x0400): High Speed
/// - Bit 11 (0x0800): SuperSpeed (USB 3.0)
/// - 默认: Full Speed
pub fn from_usb2_hub_status(raw: u16) -> Self {
if (raw & 0x0200) != 0 {
Speed::Low
} else if (raw & 0x0400) != 0 {
Speed::High
} else if (raw & 0x0600) != 0 {
Speed::SuperSpeed
} else {
Speed::Full
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct Hub { | ||
| pub info: HubInfo, | ||
| pub backend: Box<dyn HubOp>, | ||
| } | ||
| impl Hub { | ||
| pub fn new( | ||
| backend: Box<dyn HubOp>, | ||
| infos: &BTreeMap<Id<Hub>, HubInfo>, | ||
| port_id: u8, | ||
| parent: Option<Id<Hub>>, | ||
| ) -> Self { | ||
| let slot_id; | ||
| let mut hub_depth = 0; | ||
| if parent.is_none() { | ||
| hub_depth = -1; | ||
| slot_id = 0; | ||
| } else { | ||
| slot_id = backend.slot_id(); | ||
| let mut current_parent = parent; | ||
| while let Some(p) = current_parent { | ||
| let parent = infos.get(&p).expect("parent hub info must exist"); | ||
| if parent.hub_depth == -1 { | ||
| break; | ||
| } | ||
|
|
||
| hub_depth += 1; | ||
| current_parent = infos.get(&p).and_then(|info| info.parent); | ||
| } | ||
| } | ||
|
|
||
| let depth = target_depth.expect("route string is full"); | ||
| let shift = (depth - 1) * 4; | ||
| let mask = 0x0F << shift; | ||
| self.0 = (self.0 & !mask) | (((hub_port as u32) & 0x0F) << shift); | ||
| } | ||
|
|
||
| pub fn route_port_ids(&self) -> impl Iterator<Item = u8> + '_ { | ||
| (0..5).filter_map(move |depth| { | ||
| let port = ((self.0 >> (depth * 4)) & 0x0F) as u8; | ||
| if port == 0 { None } else { Some(port) } | ||
| }) | ||
| } | ||
|
|
||
| pub fn to_port_path_string(self, root_hub_port: u8) -> String { | ||
| let mut parts = vec![root_hub_port.to_string()]; | ||
| for port in self.route_port_ids() { | ||
| parts.push(port.to_string()); | ||
| } | ||
| parts.join(".") | ||
| } | ||
| } | ||
|
|
||
| impl Debug for RouteString { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| let mut iter = self.route_port_ids(); | ||
| if let Some(first) = iter.next() { | ||
| write!(f, "{first}")?; | ||
| for port in iter { | ||
| write!(f, ".{port}")?; | ||
| } | ||
| Self { | ||
| backend, | ||
| info: HubInfo { | ||
| parent, | ||
| port_id, | ||
| slot_id, | ||
| hub_depth, | ||
| speed: Speed::Full, | ||
| tt: UsbTt { | ||
| multi: false, | ||
| think_time_ns: 0, | ||
| }, | ||
| }, | ||
| } | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Removing the RouteString type here breaks the #[cfg(test)] unit tests at the bottom of this file, which still reference RouteString (via use super::RouteString;). Those tests will now fail to compile because the type no longer exists; either update or remove the tests, or retain a minimal RouteString (possibly behind cfg(test)) to keep the test module valid.
| #[derive(Debug, Clone)] | ||
| pub struct HubInfo { | ||
| /// 若为 None, 则表示 Root Hub | ||
| pub parent: Option<Id<Hub>>, | ||
| /// Roothub 为 `None` | ||
| pub route_string: Option<RouteString>, | ||
| pub backend: Box<dyn HubOp>, | ||
| pub slot_id: u8, | ||
| pub hub_depth: isize, | ||
| pub speed: Speed, | ||
| pub port_id: u8, | ||
| pub tt: UsbTt, | ||
| } |
There was a problem hiding this comment.
HubInfo.speed is initialized here to Speed::Full but is never updated for external hubs, even though it is later used in xhci::device::Device::address to find the first high-speed hub when setting TT information (matches!(parent_hub.speed, Speed::High)). As a result, parent_hub.speed stays Full for non-root hubs and the TT parent selection for LS/FS devices behind HS hubs will be wrong. Consider setting info.speed based on the hub's actual protocol/speed in HubDevice::configure (e.g. from device_protocol) so that the topology information in HubInfo is accurate.
| pub(crate) async fn init(&mut self, host: &mut Xhci, info: &DeviceAddressInfo) -> Result { | ||
| // Keep the raw PORTSC.PortSpeed encoding for interval calculations | ||
| self.port_speed = info.port_speed.to_xhci_portsc_value(); | ||
| self.port_speed = info.port_speed; | ||
| // let speed = info.port_speed.to_xhci_portsc_value(); |
There was a problem hiding this comment.
The comment in Device::init still says "Keep the raw PORTSC.PortSpeed encoding for interval calculations", but port_speed has been changed to store the logical Speed enum instead of the raw xHCI PORTSC.PortSpeed value. This stale comment is now misleading and could cause future changes to assume port_speed is a raw encoding again; please update or remove the comment to reflect the new semantics.
1 participant
No description provided.