USB4 initial work on suspend routine
Needs ReviewPublic
Actions

Authored by obiwac on Mar 22 2025, 1:50 AM.

Details

Reviewers

mohammad_fbsdphab_thelightbird.com
imp
jhb
emaste
ngie

Summary

Initial work on a suspend routine for USB4 on v1.0 host interfaces. At the moment I'm not quite concerned about resume, as it would already be quite nice to be able to suspend USB4 routers as this is a prerequisite for S0i3 on AMD.

This will also work on v2.0, but we might want to handle this differently in the future for these hosts.

Test Plan

Tested on AMD Pink Sardine controller (1022:1668/9), which is host interface version 1.0. The SLPR (sleep ready) bit is being set by the hardware.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 63790
Build 60674: arc lint + arc unit

Event Timeline

obiwac created this revision.Mar 22 2025, 1:50 AM

Herald added a subscriber: imp. · View Herald TranscriptMar 22 2025, 1:50 AM

obiwac requested review of this revision.Mar 22 2025, 1:50 AM

Harbormaster completed remote builds in B63048: Diff 152527.Mar 22 2025, 1:50 AM

obiwac added a parent revision: D49452: USB4 host reset (& writing to router config space).Mar 22 2025, 1:50 AM

obiwac edited the summary of this revision. (Show Details)

obiwac added reviewers: imp, jhb, emaste.Mar 22 2025, 3:24 AM

imp added inline comments.Mar 25 2025, 3:30 AM

sys/dev/thunderbolt/router.c
381	Why don't we want these events?
396	Do we want to be verbose about being in this state to see if we need to implement this or if we can punt? Or is it a 'gotta do eventually'?
398	50ms is a long time to wait here...
410	And we wait 50ms multiple times. 100 if I'm reading the code right 5 seconds sleeping in a suspend path seems potentially unwise if we have to do this for multiple bridges. Is there a way to avoid that?
421	This will prevent the whole system from shutting down.... how likely is this timeout?

obiwac added inline comments.Mar 25 2025, 3:17 PM

sys/dev/thunderbolt/router.c
381	Sorry, I should make this clearer. The initial work on USB4 is meant to get suspend working well enough for the firmware on AMD systems to enter S0i3, since these are handed over to the OS in a powered-on state. Currently, I want to make sure we don't wake up for any reasons as I'm not handling resume and am not intending to handle resume in the near future.
396	I think leaving as is fine; the spec just mentions that, while we can wait for ROP_CMPLT on v2, polling as on v1 routers is also fine. So more of a "gotta do eventually".
398	This comes from tSetSR and is the time between setting SLP and SLPR being set by the router.
410	Actually, I think tSetSR is the maximum time the router has to set SLPR after SLP. I will double check this tomorrow, because this means we can immediately poll once every maybe 5 ms a maximum of 10 times.
421	I'm guessing this is unlikeley but if this is the case then I'll downgrade this to a simple warning I guess.

Don't touch router defines; this should be done in D49452.

Harbormaster completed remote builds in B63790: Diff 154523.Apr 29 2025, 9:55 PM

vishwin added a subscriber: vishwin.Jun 14 2025, 8:09 PM

obiwac mentioned this in D52861: thunderbolt: Support generic USB4 NHIs.Mar 3 2026, 9:35 PM

emaste added a subscriber: seuros.Aug 12 2026, 5:16 PM

Rebase

Harbormaster completed remote builds in B75549: Diff 183966.Aug 13 2026, 6:20 AM

obiwac added a reviewer: ngie.Aug 13 2026, 6:22 AM

The changes seem reasonable. My general concerns with the change are just related to documentation, follow up and potential future collaboration (especially if I start helping out in this space in the next few months).

sys/dev/thunderbolt/nhi.c
429	I assume that the only reason why we don't have resume code after this change is that we don't actually properly initialize NHI today. Is that correct? If so, this warrants another TODO and follow up issue.
sys/dev/thunderbolt/router.c
381	Sorry, I should make this clearer. The initial work on USB4 is meant to get suspend working well enough for the firmware on AMD systems to enter S0i3, since these are handed over to the OS in a powered-on state. Currently, I want to make sure we don't wake up for any reasons as I'm not handling resume and am not intending to handle resume in the near future. I seems like it deserves documentation in the manpage and comment, as well as a tracking issue: this helps avoid the scenario where a random user will file a bug saying the feature doesn't work ("it's already documented clearly here") and helps encourage those who have the knowledge/capability to collaborate with enhancements/fixes.
406	Please use `NHI_SLPR_WAIT_US` instead of `50 ms` here (using another constant to convert from microseconds to milliseconds is reasonable because the constant is expressed in microseconds).
419–420	Please keep the string constant/format strings intact so it's greppable by humans.
427–429	Please file a follow up issue here.

emaste added inline comments.Aug 13 2026, 6:20 PM

sys/dev/thunderbolt/nhi.c
429	Fair point, either way there should be a comment here explaining why nhi_resume does nothing.
sys/dev/thunderbolt/router.c
419–420	As an aside, we should note this in style(9) if we don't already

rebase + respond to comments

Harbormaster completed remote builds in B76888: Diff 186697.Mon, Sep 14, 4:24 PM

obiwac added inline comments.Mon, Sep 14, 4:24 PM

sys/dev/thunderbolt/router.c
406	opted to just do `/ 1000` inline because no existing constant and this is obvious enough
419–420	what exactly should style(9) say about this? i unwrapped this line because it was <= 80 chars anyway, but if the string makes the line significantly longer we should wrap anyway. what's the threshold?
421	actually I think it is best for this to error hard. I don't want people thinking they can enter S0i3 when actually they aren't (because this driver failed to suspend). I am in the process of adding some proactive diagnostics if we know we didn't enter S0i3 on resume, but I'd still like to keep it like this. We can replace with `return (0);` if this causes problems for people using S3 once we load this kernel module by default
427–429	should i really file bugzilla PRs for each TODO I have in my code? this is all very unfinished anyway...