[joshuaswarren]

This is a follow-up to #1601. In that thread, RyanTheTide suggested replacing the hrtimer_active() check in the send path with !hrtimer_is_queued(). I didn't take that swap. The tick forwards its own timer, and hrtimer_forward() requires the timer not be enqueued by anyone else at that moment. A kick that saw the timer as not queued while the tick was running could start it under the tick.

This change keeps one flag, rx_fast_armed. A kick starts the timer only when it moves the flag from 0 to 1. The tick keeps the flag while it restarts. When the tick finds the deadline passed, it clears the flag and reads the deadline again. A kick that landed in between saw the flag set and started nothing, so the tick takes the flag back and restarts. Before, that kick was lost until the next jiffy poll or send.

The stop and quiesce paths now use one helper that cancels the timer and clears the flag.

Tests: two KUnit cases in mailbox-g17-test.h, run on arm64 under QEMU with apple_firmware-style config (KUNIT, ARCH_APPLE, PM, APPLE_MAILBOX). One checks that only the 0 to 1 kick starts the timer. The other models the race and fails when the recheck is removed. I removed it to confirm: 3 pass, 1 fail. With the recheck in place the suite reports 4 pass, 1 skip. The skip is the existing uclamp test, which needs CONFIG_UCLAMP_TASK. checkpatch --strict is clean.

Not booted yet. I will boot it on a 14-inch M5 Max MacBook Pro and measure GPU mailbox receive latency with and without this patch, then post the numbers here.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-12T23:23:44.970725Z	b16d4c9	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b16d4c9362

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Serialize cancellation and flag reset against kicks

During a stop/warm restart with an in-flight RTKit reply, a kick can win the 0→1 exchange but not yet reach hrtimer_start(). hrtimer_cancel() then finds no active timer, this reset clears the sender's ownership, and the sender queues a timer with rx_fast_armed == 0. This is reachable because apple_rtkit_detach_rx() stops the mailbox before apple_rtkit_warm_restart() drains rtk->wq, so an already-running receive worker can still send. If a quick restart lets that timer run during its grace period, another send can enqueue it while its callback calls hrtimer_forward_now(), triggering the queued-timer warning or racing the expiry update. Exclude concurrent kicks while cancelling and resetting ownership.

Useful? React with 👍 / 👎.

[joshuaswarren]

[Joshua's agent] This is the same window an earlier review found, and 391eb78 closes it. The comment is on the old commit b16d4c9, where the cancel helper reset the flag unconditionally.

The helper now clears the flag only when hrtimer_cancel() removed a queued timer. A kick that has taken the flag but has not reached hrtimer_start() keeps its ownership, and its timer starts afterwards. A tick that is running when the cancel arrives clears the flag itself on every NORESTART exit, so no idle timer is left holding a stale flag.

The new KUnit case apple_g17_fast_rx_cancel_keeps_inflight_kick_test covers it: the flag survives a cancel with no queued timer, and a cancel of a queued timer clears it. With the old unconditional reset restored, that case fails at its first assertion. Under arm64 QEMU the suite gives 5 pass and 1 skip, the skip being the uclamp case that needs CONFIG_UCLAMP_TASK.

The stop and warm restart interleaving itself is not run on hardware yet. The latency run will exercise it.

[joshuaswarren]

GREEN at b16d4c9
Cross-model review (GLM)
Model: zai/glm-5.3

Joshua's agent review (lane w7X, a non-Claude reviewer working for Joshua Warren). Scope Main gave me: the race correctness of the kick/tick handoff and the KUnit race case. I did not boot hardware.

CI caveat, up front: at the time of posting the PR's own checks kunit, DWC3/ATC x3 and rust-build are still queued or running (only source and host-tests finished, both success). This GREEN covers the patch's logic and the KUnit run I did myself below; it is not a statement that those jobs passed.

The handoff is correct. The state is one flag F (rx_fast_armed), the deadline U (rx_fast_until) and the timer T. I went through every interleaving of a kick against the tick's stop decision:

1. The kick's xchg(F,1) lands before the tick's xchg(F,0): the kick sees 1 and starts nothing. Its write to U happened before its xchg, the tick's xchg is fully ordered after it, so the tick's re-read of U sees the new deadline, ktime_before is true, the tick's xchg(F,1) returns 0 and it restarts. This is the lost-kick case the patch fixes.
2. The kick's xchg lands after the tick's xchg(F,0): the kick sees 0 and starts T. The tick's own xchg(F,1) then returns 1, keep_armed is false and the tick returns NORESTART. Exactly one of them owns T; hrtimer_start() on a timer whose callback is still running is allowed, and __run_hrtimer() only re-enqueues for RESTART, so the kick's enqueue is kept.
3. U is written and the flag exchanged on one side, flag exchanged and U read on the other, both with value-returning xchg (fully ordered), which is the store-buffering pattern closed from both sides, so at least one side sees the other's write. A kick cannot be missed.
4. While the tick forwards (traffic, or now < U), F stays 1, so no kick starts T; hrtimer_forward_now() never runs against an enqueued timer.
   The kick runs under tx_lock with interrupts off, so it cannot be preempted between its xchg and hrtimer_start(). priv comes from devm_kzalloc, so F starts at 0. checkpatch --strict -g HEAD is clean (0 errors, 0 warnings, 0 checks).

KUnit, which I ran myself: worktree at this head, kunit.py run --arch=arm64 --cross_compile=aarch64-linux-gnu- under QEMU with CONFIG_KUNIT, COMPILE_TEST, OF, PM, APPLE_MAILBOX (no ARCH_APPLE), suite apple_mbox_g17_stall: 5 tests, 4 passed, 1 skipped (the uclamp case), as the description says. The test calls the real apple_mbox_fast_rx_kick, apple_mbox_fast_rx_keep_armed and apple_mbox_fast_rx_cancel, not copies.

My mutations of mailbox.c (each applied, rebuilt, the suite re-run, restored):

mutation	result
K1 keep_armed always false after the drop (recheck removed)	3 pass, 1 fail (caught; matches the description)
K2 recheck ignores the deadline (only re-takes the flag)	caught
K3 tick never drops the flag first	caught
K5 fast_rx_cancel does not clear the flag	caught
K6 kick always starts the timer	caught (2 fail)
K4 kick is read-then-set (atomic_read then atomic_set) instead of atomic_xchg	survives (4 pass)

Findings, none blocking:

1. The "race case" is not a concurrent test, and K4 shows what that costs. apple_g17_fast_rx_keeps_racing_kick_test sets up the state after the interleaving (flag 1, deadline 0, a kick that starts nothing, then a live deadline) and checks that keep_armed takes the flag back. That is the right assertion for the recheck, and it fails when the recheck is removed. But it runs on one CPU in order, so it cannot catch a kick that is not an atomic test-and-set, or a missing barrier; the correctness of those parts rests on the code reading above, not on a test. If you want one, a two-thread stress of kick against a tick that stops and re-arms (counting hrtimer_start calls that overlap a callback) would cover it. Say so in the description so nobody reads "models the race" as "stress-tested".
2. apple_mbox_fast_rx_cancel() clears the flag after hrtimer_cancel(), so a kick can slip between them. If a sender calls apple_mbox_fast_rx_kick() in that window (the stop path sets active false but only the quiesce path sets removing, and apple_mbox_send() fences on removing), the kick sees F=0, sets 1 and starts T; then cancel stores 0. The result is T queued with F=0. The next tick sees !active and exits (and writes 0), so it is harmless today; it only matters if the mailbox is restarted within one 25 us poll period, which I do not think can happen. The comment on apple_mbox_fast_rx_cancel ("After this the timer is neither queued nor running") is not strictly true against a concurrent kick; I would soften it.
3. Not booted. The description says so and promises receive-latency numbers on a 14-inch M5 Max; I have no hardware and did not verify any latency or boot behaviour.

On RyanTheTide's !hrtimer_is_queued() suggestion from #1601: the description's reason for not taking it is right. A kick that sees "not queued" while the tick is inside its callback can start T under the tick's hrtimer_forward_now(); the flag removes that.

[joshuaswarren]

[Joshua's agent] I pushed 391eb78 after reviewing the first revision. It closes two lost-wakeup windows in the arming flag.

The cancel helper was clearing the flag even when no timer was queued. A kick that had already taken the flag could have its timer started while the flag read 0. It now clears the flag only when it cancels a queued timer.

The tick's early exit for an inactive or removing mailbox was dropping the flag without the deadline recheck. A kick that raced a resume could lose its poll. It now uses the same recheck as the normal exit.

The KUnit cases changed as well. The race case no longer sleeps past the kick deadline. It checks that the kick moved the deadline and that the recheck keeps the flag. A third case covers the cancel with an in-flight kick and with a queued timer. Under arm64 QEMU the suite gives 5 pass and 1 skip (the uclamp case needs CONFIG_UCLAMP_TASK). With the cancel fix reverted, the in-flight case fails.

The review didn't exercise the real tick function, so the early-exit change has no unit test. It will get hardware numbers in the latency run.

[RyanTheTide]

@joshuaswarren the flag handling and the cancel helper look right to me, and KUnit covers the race. Before merge, please post the boot on the M5 Max you mentioned: GPU mailbox receive latency with and without the patch, and at least one warm restart or s2idle cycle of a T6050 coprocessor with the patch, with no hrtimer warnings in dmesg.

[joshuaswarren]

[Joshua's agent] Boot results for 391eb78 on a 14-inch MacBook Pro with M5 Max (T6050), 2026-11-13.

What ran: the patch is the only mailbox change in both patched kernels. The first patched kernel (comb1) also carries a Wi-Fi trace commit. The second (comb2) is comb1 plus an RTKit crash-log fix. The unpatched baseline is the same tree without those commits.

1. Mailbox timing, with and without the patch. I have no receive-latency probe on this machine. The number is a frame loop through the GPU mailbox: vkcube --c 6000 --present_mode 0 --wsi wayland, idle machine, 10 runs, milliseconds per run.

Kernel	Uptime	Runs	Mean
Without patch, boot f2882f0f	3 min	802, 877, 893, 1014, 1033, 956, 1011, 1174, 1057, 1034	985
With patch (comb1), boot b771aaab	3.7 min	622, 882, 900, 970, 976, 978, 997, 1015, 1075, 1165	958
With patch (comb1), same boot	40 min	600, 724, 828, 713, 690, 989, 941, 932, 999, 999	842

The patched kernel is not slower. Runs rise across a series on both kernels. I read that as heat or clock steps. The data shows no speedup, and it does not measure receive latency.

2. Suspend and resume. On comb2 (boot 30469a69-e7ce-401e-a22e-3ba165250985) I ran one s2idle cycle with an RTC wake. Suspend entry was at 338.1 s and exit at 399.9 s. suspend_stats read success 1, fail 0. After resume the Neural Engine firmware logged "resume ok". The GPU ran 6000 vkcube frames with exit code 0 in 578 ms. Wi-Fi, Bluetooth and the SD host were still present. dmesg had no mailbox, SError or fault line.

3. hrtimer warnings. Zero hrtimer lines in dmesg on comb1 (checked at 3, 10 and 40 min uptime) and on comb2 (checked at 3 min and after the suspend).

Limit: frame-loop timing only, no receive-latency probe, one suspend cycle, one machine.

[DjDeveloperr]

@joshuaswarren a heads-up: #1844 now removes the T6050 GPU mailbox fast-receive poller entirely. Following a direction from the project owner, those mailboxes receive by interrupt like the other ASC mailboxes, and mailbox.c goes back to its state before #1601. With the fast-poll hrtimer gone, the kick/tick race this PR fixes with rx_fast_armed can no longer happen, so this PR won't apply after #1844 lands. Closing it then seems right, unless you see a case I'm missing.

[joshuaswarren]

[Joshua's agent] Thanks, @DjDeveloperr. Agreed: #1844 is now merged (adb5a7e), so the T6050 fast-poll timer and the kick/tick race addressed here are gone. I see no remaining T6050 case requiring #1739 after that change; this PR is superseded there. I am not extending that conclusion to the separate T8152/ASCWrap v8 poller.

Our earlier numbers remain historical polling-versus-polling results, not evidence against interrupts: the unpatched baseline already had the #1601 poller, while comb1/comb2 added the arming fix. The 6000-frame vkcube means (985 ms baseline; 958 ms patched at similar uptime, 842 ms later) were whole frame-loop wall times, not mailbox receive latency, and drifted within each series. They establish neither a receive-latency improvement nor an interrupt-versus-polling throughput comparison. The successful s2idle and zero hrtimer-warning observations apply only to those recorded polling builds.

The M5 is back on comb3c today; that recovery/desktop check is not a #1844 A/B run. We have no new interrupt-path timing or power result on this 14-inch K114c, and cannot transfer the J716c inference gains in #1844 to this machine without a matched rerun. No timing claim from our earlier comment should be used to retain the removed poller.

[DjDeveloperr]

Thanks @joshuaswarren, and thanks for the careful race analysis here. As you noted, #1844 (merged as adb5a7e) superseded this PR. The maintainer's direction is that mailboxes are always driven by interrupts, so the T6050 fast-receive poller and the kick/tick race it needed are gone.

Could you close this one? If you find something that still applies on the interrupt path, a fresh PR against current integration/aurora would be welcome. The separate T8152/ASCWrap v8 poller you mentioned would also be a fresh PR.

[joshuaswarren]

Closing as superseded by #1844 (adb5a7e). Thanks for the review. If anything still applies on the interrupt path, it'll come as a fresh PR against integration/aurora.