udbc: Add usb debug host mode driver

[thj]

Summary

xhci offers a debugging interface which uses a special usb 3 cable wit the D+,
D- and VBUS pairs disconnected. This interface allows a target device to
configure its xhci controller as a debugging channel which can then be used to
provide a serial link between the target and a debug host.

This change extracts the udbc host mode driver from hrs's xhci implementation.

xhci debug target support will come in the future, I am currently aiming for
early in the 16 branch point. I can test the host mode today and it seems to me
to be generally useful for debugging linux or windows targets and for FreeBSD
15 to be able to debug 16+

This diff:

• extracts the driver and makefile changes
• removes if 0'd code
• formats the driver with clang format
• adds a man page

I'll have intermittent availability for the next three weeks and am happy for
this review to mature a bit. My goal is to commit near the start of August.

Test Plan

I have tested this driver against the wip freebsd target implementation and the
linux xhci debug implementation

I wrote a simple serial performance tool to investigate the throughput udbc can
obtain and to see if the driver remained stable over time. udbc+linux is slow
(manages 1.2MB/s, compared to null modem tests running at 3MB/s). Part of this
is the test tool is unoptimized, but I suspect there is more to extract.

Instructions for setting up xhci debug on linux are available here:
https://www.kernel.org/doc/html/v4.19/driver-api/usb/usb3-debug-port.html
I have a patched debian 12 kernel I can provide to anyone that wants to try the
target portion in a vm.

My perf tools is here:
https://www.freebsd.org/~thj/examples/sperf.py

[bz]

sys/dev/usb/serial/udbc.c

119

I believe if you move the block from here until MODULE_VERSION to the end of the file all the prototypes can go away as the functions will just be ordered correctly and frankly having the MODULE lines in the middle of the file feels weird (at least from the [PCI] drivers I have cached in my mind.

The put the bus probe/attach/detach functions before that. and then on top the auxiliary ones, usually the read/write wrappers ones comes first.

Maybe I al too wpaul-ified with the voices in my head ;-) but it's a structure I am used to and I assume this does not diff to NetBSD anymore in any reasonable way, does it? If it does, then disregard.

335

label at beginning of line.

368

dead code below?

sys/modules/udbc/Makefile

6

Normally I would expect SRCS= udbc.c SRCS+= all the (bus etc) header files or with a \ but the implementation first.

sys/modules/usb/Makefile

51

I find this highly confusing to have += and \ mixed in a file; also the re-wrapping of long lines feels annoying; Can we not just expand them into one-a-line (maybe upfront and then add the single new line).

I assume the current format is for grouping by type but that could easily be a comment for a block then if we don't want to sort them alphanum; kind-of make it more like the level up modules/Makefile?

[pauamma_gundo.com]

Also needs something added to share/man/man4/Makefile, IIRC.

share/man/man4/udbc.4

39

56–59

Is there a difference between "target" (uncapitalized) and "Target" (capitalized)?

94–96

[ziaee]

share/man/man4/udbc.4

1

This hyphen was for a long abandoned parser.

17–31

I'm trying to increase the consistency and predictability of the manual. Predictability is one of the greatest strengths of manuals, but it requires consistency, like we have in every other section's synopses. I gave a 5 min lightning talk about section 4 synopses at BSDCan:

https://people.freebsd.org/~ziaee/tmp/bsdcan25lightning-ziaee.webm

61

This should go earlyish in DESCRIPTION, HARDWARE should say something like:

The
.Nm
driver supports the debugging channel provided by XHCI compliant controllers.

If I'm understanding this patch correctly.

79

84

I think we should move these to STANDARDS

[thj]

sys/dev/usb/serial/udbc.c

119

I'm a bit torn, I expect the MODULE lines at the end of the file, but it seems every other usb serial device does this. I'm put good odds on it being because they all copy from the same file.

I'll ask some other people what they think

368

I've removed all the ioctl code, it is unused and not common to other files

[thj]

• Don't split subdir += lines
• rearrange headers to build (I am sure this built in this tree before)
• Move DRIVER_MODULE to the bottom of the file
• Address review comments on makefile

[ziaee]

share/man/man4/udbc.4

61

Sorry I didn't explain it well enough. This entire section will appear in the hardware release notes. Its supposed to describe what is supported by the driver, see https://man-dev.freebsd.org/style.mdoc#HARDWARE_Section

94–96

The 15.0 is an argument to .Fx so it does need to be on the same line.

[hrs]

share/man/man4/udbc.4

15

I have a patch to explain the DbC function based on the words in the specification. Could you take a look at it?

https://people.allbsd.org/~hrs/FreeBSD/D51299-udbc.4
https://people.allbsd.org/~hrs/FreeBSD/D51299-udbc.4.diff

These terminologies may be rather difficult to understand, but I feel a bit odd about the original patch since the udbc driver is not xHC-specific. The suggested patch tries to keep more generic description. Plus, I added a reference of the A-to-A cable specification.

[thj]

share/man/man4/udbc.4

15

These links don't work for me (0930UTC 14th August)

sys/dev/usb/serial/udbc.c

119

I have moved the MODULE lines to the bottom of the file, but there are a lot of calls to these functions and untangling isn't enjoyable. I think this is a balance between being similar to other usb serial files and being different.

sys/modules/usb/Makefile

51

I've made them multiple SUBDIR += calls.

[thj]

share/man/man4/udbc.4

15

Thanks these work today (they didn't yesterday).

I'll probably take your text whole in the review

[thj]

• take hrs's text for the man page

[bcr]

OK for the updates to the man page.

[bz]

sys/dev/usb/serial/udbc.c

119

ACK

325

Label still not at the beginning of the line.

358

This entire function currently reads:

return (ENOIOCTL);

[thj]

• move tr_setup label to start of line
• remove body of ioctl function

[thj]

I am pretty sure I've addressed all outstanding comments, but I could easily have missed some. Could interested people check again?

[bz]

From me it looks okay. If no one from USB approves timely I'd put it in.

[lwhsu]

share/man/man4/udbc.4

131

sys/dev/usb/serial/udbc.c

6

158

168

Does the specification specify bConfigIndex needs to be 0?

170

should we use interface macro?

[thj]

sys/dev/usb/serial/udbc.c

168

not that I can see (the string configindex doesn't appear in either relevant specification).

This pattern is in most usb drivers so I would guess it was copied over.

[thj]

• fix a typo in man page
• provide full year in copyright
• move devmethod_end onto its own line
• use interface define rather than device define

[thj]

Unless someone shouts at me I will land this on Monday so it can make ALPHA3 on Thursday