List root hubs function #77

tuna-f1sh · 2024-08-23T13:17:16Z

Considered this as a discussion but thought a PR is easier to show implementation. I understand the removal of root hubs from the list_devices function (since they are not devices) but I wonder if you would be open to a separate function, specifically to get the root hubs?

I realise the ambitions of this crate is for low level implementation of user space drivers but I feel a lot of people are going to use it as a replacement of libusb too. Whilst root hubs are an phoney device and an abstraction of the Host Controller, I think they can be helpful:

To understand details of a Host Controller that a device is attached too, perhaps diagnose why a device has enumerated at a slow speed for example.
For profiling a complete picture of the system bus.

On Linux it's easy to add this feature since I believe that's where the whole root hub concept came from and they are in sysfs. On the other platforms, I've parsed what data exists in a similar manor to how the Linux 'devices' return their descriptor. The Windows API does actually have mention of root hubs in a couple of places. I've tried to be explicit in the docs on where the data comes from.

To provide some context, I've been working on replacing libusb with nusb in my lsusb compatiable system USB profiler cyme. So for my use case it is specifically profiling the system USB. With the addition of this list_root_hubs function, the output is actually more verbose than libusb because that only returns root hub information on Linux (develop PR).

I understand if this is outside the scope of what you want to or too woolly do but hope since it's not impacting the other code and should be quite maintainable it's of interest. The existing code you've written in this crate (not exposed) make it easy to add but not outside the crate context. Open to changes if you like the idea but think it could be done differently - not everything I'm totally happy with.

martinling · 2024-08-23T14:57:02Z

I can see this being quite useful on the application side too, for helping to identify where on the system a device is connected, or in bug reporting for identifying issues/behaviour that seem to be specific to certain hardware.

Hacking this information into DeviceInfo seems like an ugly hack though (albeit one that mirrors the "root hub" hack that the OS was already doing).

I reckon this would be better as list_host_controllers, returning a purpose-designed HostControllerInfo type, if Kevin considers it in-scope in the first place.

kevinmehall · 2024-08-26T16:44:19Z

I agree with @martinling, it should return a dedicated HostControllerInfo or BusInfo type with applicable fields, instead of trying to come up with values for all the DeviceInfo fields. The question is, what fields make sense?

bus_id (to correlate with devices)
driver name
speed? (looks like this might be difficult on Windows)
PCI IDs? Would definitely be better to present them as PCI VID / PID rather than conflating with USB VID/PID, but what is the actual use case? I could see a system profiler wanting to show the PCI topology and where USB fits in it, but that's definitely getting out of scope. And on some devices a USB controller won't be connected via PCI.

tuna-f1sh · 2024-08-27T10:42:02Z

Thanks for the feedback. A specific type was something I debated over too. One I considered was a enum HostController return type so that it can contain a RootHub(DeviceInfo) on Linux and HostController(HostControllerInfo) on other systems but this seemed unfriendly.

How about something like this:

pub struct HostControllerInfo {
    #[cfg(target_os = "linux")]
    pub(crate) path: SysfsPath,
    /// System ID for the host controller
    pub(crate) bus_id: String,
    /// PCI vendor ID
    pub(crate) vendor_id: u16,
    /// PCI device ID
    pub(crate) device_id: u16,
    /// PCI hardware revision
    pub(crate) revision: u16,
    /// PCI subsystem vendor ID
    pub(crate) subsystem_vendor_id: u16,
    /// PCI subsystem device ID
    pub(crate) subsystem_device_id: u16,
    /// The phony root hub device
    #[cfg(target_os = "linux")]
    pub(crate) root_hub: DeviceInfo,

    #[cfg(target_os = "linux")]
    pub(crate) busnum: u8,

    #[cfg(target_os = "windows")]
    pub(crate) instance_id: OsString,

    #[cfg(target_os = "windows")]
    pub(crate) location_paths: Vec<OsString>,

    #[cfg(target_os = "windows")]
    pub(crate) parent_instance_id: OsString,

    #[cfg(target_os = "windows")]
    pub(crate) devinst: crate::platform::DevInst,

    #[cfg(target_os = "windows")]
    pub(crate) driver: Option<String>,
}

That way on Linux the root hub device can still be obtained if desired. In answer to the above:

bus_id - same as DeviceInfo a String so works cross-platform and can be matched to devices.
driver - included on Windows like DeviceInfo. I think on Linux it requires udev. macOS does have the 'CFBundleIdentifier' so could be added.
speed - I suggest dropping this as it can only really be best guessed and on Linux can be obtained from the included root_hub. Could be a getter for this.
I put PCI IDs in. Use case I have is that using 'pci.ids' one can resolve the manufacturer and device name. As @martinling mentions, there is a possible application use case here whereby one might wish to know the kind of host controller being used. You're right it might not always be PCI - maybe mention of PCI should be dropped.

One issue I see with the above is that on Linux two root hubs can share the same host controller - different USB versions for example. Would we return two of the same HostControllerInfo in this case?

tuna-f1sh · 2024-08-27T10:48:13Z

Actually I have a idea for the multiple root hubs issue: rather than include the root_hub as part of the struct, the impl HostControllerInfo could have a root_hubs() on Linux. The root hubs for the host controller are listed on the sysfs path /sys/bus/pci/devices/SYSFS_NAME/usbX.

martinling · 2024-08-27T15:09:16Z

I would suggest putting all the PCI-specific info in a PciInfo type and having a single Option<PciInfo> field in HostControllerInfo. That avoids a whole set of issues about what values should be provided or expected in the case where the host controller is not a PCI device.

tuna-f1sh · 2024-09-02T14:36:05Z

I'm happier with this and so ready for review again. I opted for a BusInfo type rather than HostControllerInfo because there can be more than one system bus per Host Controller; two BusInfo can have the same PciInfo. So it's still an abstraction of sorts but more portable than root hubs.

src/platform/linux_usbfs/enumeration.rs

tuna-f1sh · 2024-09-09T08:02:10Z

@martinling @kevinmehall not sure if you get notified by the change of status for this - sorry if this is a double notification! Would be good to get some feedback and potentially get this merged if you are happy.

martinling

This looks pretty good but I think it could still do with some API tweaks.

martinling · 2024-09-09T10:26:31Z

src/enumeration.rs

+    /// System provider class name for the bus
+    pub(crate) provider_class: Option<String>,
+    /// System class name for the bus
+    pub(crate) class_name: Option<String>,


Although these two fields are populated on all systems, the names and their descriptions only make sense in the context of macOS. What's being put in these fields on Windows and Linux doesn't match the description here. I think we should just make these two fields #[cfg(target_os = "macos")] only.

Another option would be to name these two fields something more generic. At the moment though, the text that gets put here is so inconsistent in nature that I just don't think it's useful to expose it through a common API. Here's what I see for an XHCI controller on each system (provider_class, class_name):

macOS: AppleARMIODevice, AppleT1803USBXHCI
Linux: Linux 6.1.0-22-amd64 xhci-hcd, xHCI Host Controller
Windows: (Standard USB HUBs), USB Root Hub (USB 3.0)

The only actual information to be found in these strings is:

The fact it's an XHCI / USB3 controller, which we already have an enum for.

The class names in use on macOS - but those should be in macOS-specific fields.

The driver name in use on Linux - but that should go in a Linux-specific field.

The Linux kernel version - but programs have better ways to get that info than through nusb.

The USB Root Hub (USB 3.0) string is somewhat useful because it corresponds to what the user will see in Device Manager, but again, the place for that would be a Windows-specific field.

Also note that by making these fields OS-specific, we can avoid the need to make them Option.

My suggestion here would be:

/// System provider class name for the bus' host controller. #[cfg(target_os = "macos")] pub(crate) provider_class_name: String, /// System class name for the bus' host controller. #[cfg(target_os = "macos")] pub(crate) class_name: String, /// Kernel driver name for the bus' host controller. #[cfg(target_os = "linux")] pub(crate) driver_name: String, #[cfg(target_os = "windows")] /// Description of the root hub. This is how the bus will appear in Device Manager. pub(crate) root_hub_description: String,

And then similarly for the public accessor methods.

Agreed, but on the other hand, I'm somewhat skeptical of having the majority of fields on this type be specific to one OS. Is any nusb user going to actually write OS-specific code against these? Or would it be better to consolidate them down to one name or description field that is a human-readable display name, not intended to map to any specific property?

I'd be wary of getting into the business of generating human-readable text, because downstream application authors are never going to all agree on exactly what that text should look like, and that's before you even get into the can of worms that is localization.

We're already in the situation of having to consider parsing some of these details out of strings "helpfully" generated by the OS; it'd be good not to add another layer of that on top.

I think it's better to just stick to just reporting the known facts: this is bus N on an XHCI controller handled by driver Y, etc, and then everyone's free to generate text in any language to suit their preferences from the facts available.

I had this conflict myself too, all good points. I went for the middle ground of:

Separating class_name, provider_class_name to macOS with accessor methods.

Adding the field for root_hub_description on Windows.

Made driver universal since it can be read from the pci driver symblic link filename. This is actually better than relying on the sysfs manufacturer field I think.

Created a system_name() (maybe better as just name()?) accessor method for all platforms, which returns name on macOS, root_hub_description on Windows and the root hub product string on Linux.

src/enumeration.rs

martinling · 2024-09-09T11:39:36Z

src/platform/windows_winusb/enumeration.rs

+        devinst,
+        driver: Some(driver).filter(|s| !s.is_empty()),
+        bus_id,
+        controller: None,


It would be great if we could figure out a way to fill in the controller type on Windows.

I considered parsing the device name on Windows; whether it has >= 3.0 (XHCI), 2.0 (EHCI) else OHCI. Should be accurate but it feels a bit dirty. I guess I could do this with a platform caveat covering this on Windows in the docs?

This maybe should be looking at GUID_DEVINTERFACE_USB_HOST_CONTROLLER instead of GUID_DEVINTERFACE_USB_HUB and filtering for root hubs -- In a quick look at Device Manager, the host controller properties seem a lot more relevant than the root hub's, including several that say XHCI (don't know which of those would be safest to try to parse into the enum, though).

It's a good point, the Host Controller contains a Service that seems to contain the controller type. I've updated to use this 366b2ab

Iterating over GUID_DEVINTERFACE_USB_HUB then getting the parent host controller like the Linux root hub exploration (rather than iterating over GUID_DEVINTERFACE_USB_HUB) still makes because a host controller can have more than one 'bus'. Iterating over the root hubs is the best way to build a BusInfo type I believe.

kevinmehall

Sorry I haven't had a chance to test this yet, but here are some initial comments (and thanks @martinling for jumping in).

To be honest, I'm wondering if your system profiler use case would be better served by grabbing some of the sysfs / cfgmgr32 / iokit bits and writing a new library that is explicitly an abstraction over the device hierarchy beyond USB. Specifically, I'm leaning towards saying no to the PCI parts here, because it's quite a bit of code and API surface that, well, isn't USB. Along those lines, people commonly ask for ways to find mass storage device drive letters / mount points, or serial port names corresponding to USB devices. Since libusb/nusb are focused on userspace device drivers for vendor class devices, those OS drivers and devices above and below USB seem out of scope, but there's a valid use case there that I don't think is served in a cross-platform way.

There's definitely some kind of BusInfo that makes sense in nusb though, and this is a useful exploration of what might belong on it.

src/platform/windows_winusb/enumeration.rs

src/enumeration.rs

kevinmehall · 2024-09-09T14:36:53Z

src/platform/macos_iokit/enumeration.rs

+    Ok(usb_service_iter(kAppleUSBXHCI)?
+        .filter_map(|h| probe_bus(h, UsbController::XHCI))
+        .chain(
+            usb_service_iter(kAppleUSBEHCI)?
+                .filter_map(|h| probe_bus(h, UsbController::EHCI))
+                .chain(
+                    usb_service_iter(kAppleUSBOHCI)?
+                        .filter_map(|h| probe_bus(h, UsbController::OHCI))
+                        .chain(
+                            usb_service_iter(kAppleUSBVHCI)?
+                                .filter_map(|h| probe_bus(h, UsbController::VHCI)),
+                        ),
+                ),
+        ))


[...].into_iter().flatten() would be a better way to write this than the nested .chain()

You mean like?

Ok([ usb_service_iter(kAppleUSBXHCI)? .filter_map(|h| probe_bus(h, UsbControllerType::XHCI)) .collect::<Vec<_>>(), usb_service_iter(kAppleUSBEHCI)? .filter_map(|h| probe_bus(h, UsbControllerType::EHCI)) .collect::<Vec<_>>(), usb_service_iter(kAppleUSBOHCI)? .filter_map(|h| probe_bus(h, UsbControllerType::OHCI)) .collect::<Vec<_>>(), usb_service_iter(kAppleUSBVHCI)? .filter_map(|h| probe_bus(h, UsbControllerType::VHCI)) .collect::<Vec<_>>(), ] .into_iter() .flatten())

I was under the impression the chain is better because it keeps the iterator lazy? Or maybe I misunderstood and you're not suggesting collecting each step.

kevinmehall · 2024-09-09T14:43:44Z

src/platform/windows_winusb/enumeration.rs

+        devinst,
+        driver: Some(driver).filter(|s| !s.is_empty()),
+        bus_id,
+        controller: None,


This maybe should be looking at GUID_DEVINTERFACE_USB_HOST_CONTROLLER instead of GUID_DEVINTERFACE_USB_HUB and filtering for root hubs -- In a quick look at Device Manager, the host controller properties seem a lot more relevant than the root hub's, including several that say XHCI (don't know which of those would be safest to try to parse into the enum, though).

src/platform/linux_usbfs/enumeration.rs

kevinmehall · 2024-09-09T14:57:25Z

src/enumeration.rs

+    /// System provider class name for the bus
+    pub(crate) provider_class: Option<String>,
+    /// System class name for the bus
+    pub(crate) class_name: Option<String>,


Agreed, but on the other hand, I'm somewhat skeptical of having the majority of fields on this type be specific to one OS. Is any nusb user going to actually write OS-specific code against these? Or would it be better to consolidate them down to one name or description field that is a human-readable display name, not intended to map to any specific property?

src/enumeration.rs

martinling

This looks better; the XHCI controller type is detected on Windows for me now, and the fields make a lot more sense.

I like the system_name field; I think that's a good compromise for including something that's human-readable and not OS-specific, but which matches descriptions visible elsewhere, rather than being something nusb has to invent.

However, the system_name values on macOS are a bit odd. The names I see on a Macbook M1 running 12.6.7 are usb-drd0 and usb-drd1. In System Information, these buses are both listed as USB 3.1 Bus. If we could get that latter string somehow that would be much more appropriate.

tuna-f1sh · 2024-09-10T14:32:41Z

Thanks. Re the system_name on macOS. I agree it is a bit odd and reading IONameMatch perhaps not the best placed. But then again it is loosely descriptive like the other platforms.

I searched for where System Information gets the name and can't get a good answer. I'm lead to believe it's a generated human readable name based on the bus information. It's not listed with ioreg -p IOUSB -d 2 -l for example but with system_profiler SPUSBDataType -json it appears under a '_name' key - suggesting it's generated. I don't think we want to go down that road for the points you mentioned.

tuna-f1sh · 2024-09-12T15:16:26Z

I've made the controller type on Windows much more robust and put a note regarding where the system_name() comes form on macOS. I'm not sure there is much more to tweak here.

I understand your thoughts @kevinmehall regarding the PciInfo. On the other hand, I don't feel like it adds much overhead when already gathering the data but does help describe what the bus 'sits' on.

martinling · 2024-09-12T15:46:35Z

A compromise on the PciInfo issue could be to just include a field in BusInfo that identifies the parent device, in whatever OS-specific form is appropriate.

On Linux for instance, this could just be the value of parent_path in list_buses, as everything in PciInfo can be obtained from there.

That would give programs a way to "connect" the USB information from nusb to the rest of the system's device hierarchy, without needing to complicate the nusb API with non-USB details.

kevinmehall · 2024-09-17T15:53:09Z

A compromise on the PciInfo issue could be to just include a field in BusInfo that identifies the parent device, in whatever OS-specific form is appropriate.

Yeah I think that's the way to go, and BusInfo getters for instance_id, sysfs_path, and registry_id kind of already have that covered for the three platforms. That's the root hub or host controller itself, not the parent, but anything that can use those can also find the parent PCI device easily enough.

tuna-f1sh · 2024-09-20T13:25:29Z

Great. I've removed the PciInfo and related bits. I did add the specific parent paths for Linux and Windows since they are grabbed in the parsing anyway for some fields c309c47 - can drop this if you want.

Happy to rebase too, unless you're going to squash merge.

Co-authored-by: Martin Ling <[email protected]>

tuna-f1sh · 2024-09-23T12:08:41Z

I've also rebased with main to include the new Android support. Something I'm not fully sure of though: The path is still included in the DeviceInfo but not as a getter method. I'm guessing this is intentional and hidden for compatibility as the sysfs_path is not supported on Android? I used the same logic for the BusInfo.

kevinmehall · 2024-09-24T00:45:29Z

Yeah, all of the functionality around listing devices and now buses aren't expected to work on (un-rooted) Android because it doesn't allow access to sysfs. Instead, you request device permissions and get a file descriptor to the device via Java APIs, and can use that with the new Device::from_fd API. I'm guessing he didn't notice that all of DeviceInfo comes from sysfs too, and I commented on the PR because I'm also confused by that.

tuna-f1sh · 2024-09-27T05:35:52Z

How do you want to move forwards on this @kevinmehall? The suggestion makes sense to me. I've done this for the BusInfo and list_buses - not touched the DeviceInfo as not sure if you'd prefer a separate PR for that.

kevinmehall · 2024-09-28T17:27:01Z

Thanks!

tuna-f1sh marked this pull request as draft August 29, 2024 07:03

tuna-f1sh force-pushed the root_hub branch 5 times, most recently from 80251ea to 3efdb54 Compare September 2, 2024 14:32

tuna-f1sh marked this pull request as ready for review September 2, 2024 14:36

tuna-f1sh commented Sep 2, 2024

View reviewed changes

src/platform/linux_usbfs/enumeration.rs Show resolved Hide resolved

tuna-f1sh force-pushed the root_hub branch from 6ab0807 to 4de15c1 Compare September 3, 2024 14:41

martinling suggested changes Sep 9, 2024

View reviewed changes

kevinmehall reviewed Sep 9, 2024

View reviewed changes

martinling reviewed Sep 9, 2024

View reviewed changes

src/enumeration.rs Outdated Show resolved Hide resolved

tuna-f1sh force-pushed the root_hub branch 3 times, most recently from f20302a to 1f67bf5 Compare September 10, 2024 12:34

martinling reviewed Sep 10, 2024

View reviewed changes

tuna-f1sh force-pushed the root_hub branch from 62b6766 to cf7b8be Compare September 18, 2024 15:10

tuna-f1sh requested a review from martinling September 20, 2024 13:25

tuna-f1sh and others added 17 commits September 23, 2024 13:34

a more applied doc list_buses example

c767591

macos: add CFData parsing of revision and name

602cfb0

rename UsbController; parse from instance_id on Win

a0ed0ce

pr feedback

5a4ee26

add system_name for all platforms

8694962

tidy linux readlink and parent_path

60076e6

win: probe parent service for concrete controller_type

9bdf074

mac: and_then not flatten, note on IONameMatch

d0b37bb

remove pci_info and struct

b4959da

add grabbed parent path on Linux and Windows

b607a9b

windows: remove now unused host controller parsing

6685115

Replace .map() + .flatten() with .and_then()

c9822be

Avoid nested use of Iterator::chain().

b3917aa

Update src/platform/linux_usbfs/enumeration.rs

dfa821b

Co-authored-by: Martin Ling <[email protected]>

windows: just check starts with root_hub

a4d4892

remove bus_id example filter

f86952c

prevent unsafe function by passing UsbControllerType

d8b1f10

tuna-f1sh force-pushed the root_hub branch from be15463 to d8b1f10 Compare September 23, 2024 11:51

untested android support

c7d28f8

tuna-f1sh force-pushed the root_hub branch from d3c7c8f to c7d28f8 Compare September 23, 2024 12:05

kevinmehall mentioned this pull request Sep 24, 2024

Android support for non-rooted phones #80

Merged

tuna-f1sh added 3 commits September 24, 2024 10:59

macos: return usb_service_iter to main

1ffba0c

macos: tidy list_buses now passed hci_type

f4f3719

add getters for android; note that list_buses requires root

060f284

Remove dead code

4774c75

kevinmehall merged commit 8092280 into kevinmehall:main Sep 28, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List root hubs function #77

List root hubs function #77

tuna-f1sh commented Aug 23, 2024 •

edited

Loading

martinling commented Aug 23, 2024

kevinmehall commented Aug 26, 2024

tuna-f1sh commented Aug 27, 2024 •

edited

Loading

tuna-f1sh commented Aug 27, 2024 •

edited

Loading

martinling commented Aug 27, 2024

tuna-f1sh commented Sep 2, 2024

tuna-f1sh commented Sep 9, 2024

martinling left a comment

martinling Sep 9, 2024

kevinmehall Sep 9, 2024 •

edited

Loading

martinling Sep 9, 2024

tuna-f1sh Sep 10, 2024

martinling Sep 9, 2024

tuna-f1sh Sep 9, 2024

kevinmehall Sep 9, 2024

tuna-f1sh Sep 12, 2024

kevinmehall left a comment

kevinmehall Sep 9, 2024

tuna-f1sh Sep 9, 2024

kevinmehall Sep 9, 2024

kevinmehall Sep 9, 2024 •

edited

Loading

martinling left a comment

tuna-f1sh commented Sep 10, 2024 •

edited

Loading

tuna-f1sh commented Sep 12, 2024

martinling commented Sep 12, 2024

kevinmehall commented Sep 17, 2024

tuna-f1sh commented Sep 20, 2024

tuna-f1sh commented Sep 23, 2024

kevinmehall commented Sep 24, 2024

tuna-f1sh commented Sep 27, 2024

kevinmehall commented Sep 28, 2024

List root hubs function #77

List root hubs function #77

Conversation

tuna-f1sh commented Aug 23, 2024 • edited Loading

martinling commented Aug 23, 2024

kevinmehall commented Aug 26, 2024

tuna-f1sh commented Aug 27, 2024 • edited Loading

tuna-f1sh commented Aug 27, 2024 • edited Loading

martinling commented Aug 27, 2024

tuna-f1sh commented Sep 2, 2024

tuna-f1sh commented Sep 9, 2024

martinling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinmehall Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinmehall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinmehall Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

martinling left a comment

Choose a reason for hiding this comment

tuna-f1sh commented Sep 10, 2024 • edited Loading

tuna-f1sh commented Sep 12, 2024

martinling commented Sep 12, 2024

kevinmehall commented Sep 17, 2024

tuna-f1sh commented Sep 20, 2024

tuna-f1sh commented Sep 23, 2024

kevinmehall commented Sep 24, 2024

tuna-f1sh commented Sep 27, 2024

kevinmehall commented Sep 28, 2024

tuna-f1sh commented Aug 23, 2024 •

edited

Loading

tuna-f1sh commented Aug 27, 2024 •

edited

Loading

tuna-f1sh commented Aug 27, 2024 •

edited

Loading

kevinmehall Sep 9, 2024 •

edited

Loading

kevinmehall Sep 9, 2024 •

edited

Loading

tuna-f1sh commented Sep 10, 2024 •

edited

Loading