[Toybox] [PATCH v2] lsusb: add -t option for tree format display

[Patel, Jignesh1]

Hi Rob,

>This is the one you've been poking me to review?
As mentioned in my previous reply, the v2 patch should be applied on top of 06329d54bbae. I can successfully apply the v2 patch on top of 06329d54bbae without any issues.

toybox$ git reset --hard 06329d54bbae
HEAD is now at 06329d54 lsusb: add -t option for tree format display

toybox$ patch -p1 --dry-run -i  0001-PATCH-v2-lsusb-add-t-option-for-tree-format-display.patch
checking file toys/other/lsusb.c

toybox$ patch -p1 < 0001-PATCH-v2-lsusb-add-t-option-for-tree-format-display.patch
patching file toys/other/lsusb.c

:toybox$ ./toybox lsusb -t -i  /usr/local/usb.ids
/:  Bus 001.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/12p, 480M
    |__ Port 002: Dev 002, If 0, Class=Hub, Driver=hub/4p, 480M
    |__ Port 010: Dev 003, If 0, Class=Wireless, Driver=btusb, 12M
    |__ Port 010: Dev 003, If 1, Class=Wireless, Driver=btusb, 12M
/:  Bus 002.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/6p, 10000M
    |__ Port 002: Dev 002, If 0, Class=Hub, Driver=hub/4p, 5000M
        |__ Port 004: Dev 003, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
/:  Bus 003.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/2p, 480M
/:  Bus 004.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/2p, 10000M

I submitted my v2 patch review request on July 26th, prior to your push of commit 15eea69acd83 (Cleanup pass on lsusb -t) later that day. Therefore, attempting to apply it on top of 15eea69acd83 or 1bf38ae6487c will naturally result in these Hunks, since my v2 request came before your commit 15eea69acd83 according to the timeline.

> Was what you're trying to do impacted by 1bf38ae6487c?
The v2 patch addresses all the review comments you provided for 06329d54bbae. I've attempted to resolve all your concerns in v2. Below are my inline responses:

> Roughly doubles the size of the command (diffstat says it adds 200 lines to a command that previously had ~220 lines after the header), and does not seem to share any infrastructure outside the main() function except get_names()...
- In v2, I've reused existing parse_dev_ids() for usb.ids parsing with class support,
- Leveraged dev_ids structure throughout instead of custom structs
- Eliminated custom read functions in favor of standard sysfs operations

> Not just the order changing but what it's outputting... Pity there isn't a standard format for this information...
- Fixed in v2. The output now matches with usbutils exactly, including proper bus numbering, port hierarchy, and device information. 
- The driver field now correctly shows actual controller names and port counts.

e.g.
toybox: $ ./lsusb -t -i ~/usb.ids
/:  Bus 001.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/12p, 480M
    |__ Port 002: Dev 002, If 0, Class=Hub, Driver=hub/4p, 480M
    |__ Port 010: Dev 003, If 0, Class=Wireless, Driver=btusb, 12M
    |__ Port 010: Dev 003, If 1, Class=Wireless, Driver=btusb, 12M
/:  Bus 002.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/6p, 10000M
    |__ Port 002: Dev 002, If 0, Class=Hub, Driver=hub/4p, 5000M
        |__ Port 004: Dev 003, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
/:  Bus 003.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/2p, 480M
/:  Bus 004.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/2p, 10000M

> Why did you change id1 and id2 to unsigned in get_names()? (The 
> protocol fields are 16 bit, int is 32 bit, how can it be negative?)
You're correct - reverted to original int types in v2. No functional change was needed for 16-bit protocol fields in 32-bit int.

> Since unsigned defaults to int (in C99, probably C89 too) you can just say "unsigned" without having to say "unsigned int".
Noted for future - using shortest representation for consistency.

> I generally try to have it compile without warnings, even silly ones.
In v2, I have adopted your dynamic allocation pattern.

> toybox lsusb -t says: +/:  Bus 01.Port 1: Dev 1, Class=root_hub, 
> Driver=hub/0p, 480M I dunno what the "driver" field should be, but it's not the same info.
Fixed in v2, reads actual driver information from sysfs: Extracts the driver name from the full symlink path.

> Globals go in the GLOBALS() block, it's a memory efficiency thing.
All globals moved to GLOBALS() block in v2.

> You don't need to initialize global or static variables, ELF insures 
> they default to null/zero
Removed unnecessary initializations.

> Mine had a dynamically sized string field at the end so malloc() can measure the string it's about to copy and allocate enough space. Yours is doing a ...
In v2, reused the existing dev_ids struct with a flexible char name[] field, and used dynamic memory allocation wherever necessary.

> Those are functionally the same struct, tree of int with string, you could presumably have reused dev_ids.
v2 reuses struct dev_ids throughout, eliminating custom struct duplication.


I need to rewrite the v2 patch since two commits have been added after my v2 review request was submitted. 
Please confirm if I should proceed with rewriting the v2 patch to apply on top of the latest commit.

Thanks,
Jignesh

-----Original Message-----
From: Rob Landley <rob at landley.net> 
Sent: Friday, October 31, 2025 12:44 AM
To: Patel, Jignesh1 <jignesh1.patel at intel.com>
Cc: toybox at lists.landley.net
Subject: Re: [Toybox] [PATCH v2] lsusb: add -t option for tree format display

On 10/23/25 23:51, Patel, Jignesh1 wrote:
> Hi Rob,
> 
>> Did v2 go on top of 06329d54bbae and 15eea69acd83?
 >
> The v2 patch is intended to be applied on top of commit 06329d54bbae.

This is the one you've been poking me to review?

$ patch -p1 --dry-run -i 
0001-PATCH-v2-lsusb-add-t-option-for-tree-format-display.patch
checking file toys/other/lsusb.c
Hunk #1 FAILED at 30.
Hunk #2 FAILED at 43.
Hunk #3 FAILED at 108.
Hunk #4 succeeded at 157 (offset 2 lines).
Hunk #5 FAILED at 196.
4 out of 5 hunks FAILED

There isn't a newer version I missed?

Was what you're trying to do impacted by 1bf38ae6487c?

Rob