ncm: retry link-state notification, fix carrier lost on collision#3761
Open
morse-cedricvandenbergh wants to merge 2 commits into
Open
ncm: retry link-state notification, fix carrier lost on collision#3761morse-cedricvandenbergh wants to merge 2 commits into
morse-cedricvandenbergh wants to merge 2 commits into
Conversation
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| frdm_k32l2a4s/net_lwip_webserver | 47,252 → 47,428 (+176) | — | — | — | 47,456 → 47,632 (+176) | +0.4% |
| lpcxpresso1769/net_lwip_webserver | 41,604 → 41,760 (+156) | — | — | — | 42,176 → 42,332 (+156) | +0.4% |
| mm32f327x_mb39/net_lwip_webserver | 37,436 → 37,592 (+156) | — | — | — | 42,716 → 42,872 (+156) | +0.4% |
| da14695_dk_usb/net_lwip_webserver | 43,484 → 43,640 (+156) | — | — | — | 43,688 → 43,844 (+156) | +0.4% |
| ek_tm4c123gxl/net_lwip_webserver | 43,604 → 43,760 (+156) | — | — | — | 43,800 → 43,956 (+156) | +0.4% |
| stm32c071nucleo/net_lwip_webserver | 44,308 → 44,484 (+176) | — | — | — | 49,492 → 49,668 (+176) | +0.4% |
| lpcxpresso18s37/net_lwip_webserver | 43,308 → 43,464 (+156) | — | — | — | 43,946 → 44,102 (+156) | +0.4% |
| frdm_k64f/net_lwip_webserver | 44,496 → 44,652 (+156) | — | — | — | 44,708 → 44,864 (+156) | +0.3% |
| fomu/net_lwip_webserver | 70,436 → 70,680 (+244) | — | — | — | 70,657 → 70,901 (+244) | +0.3% |
| metro_m4_express/net_lwip_webserver | 41,520 → 41,664 (+144) | — | — | — | 41,724 → 41,868 (+144) | +0.3% |
tud_network_link_state() delivered the NETWORK_CONNECTION notification edge-triggered and fire-once: if a previous notification was still in flight, notification_xmit() returned early and the notification for the new link state was never queued. Because link_is_up is committed before the send, the dedup at the top then suppressed any resend, so the host kept the stale carrier state - e.g. a permanent NO-CARRIER after a link up. The notification state (notification_xmit_state / _is_running / link_is_up) was also mutated from both the caller and the notify xfer-completion callback with no serialisation, so on RTOS ports where tud_network_link_state() runs in a task other than tud_task() the two race. Make link-state delivery level-triggered and race-safe: - add link_notify_pending, set when the link state changes and cleared only when a NETWORK_CONNECTION carrying the current state is actually submitted; - re-fire from the notify xfer-completion callback, so a send that collides with an in-flight notification is retried on the next completion instead of being silently dropped; - defer the send onto the usbd task via usbd_defer_func(), so a caller on another task cannot race the completion callback. A link toggle does not change the link speed, so only NETWORK_CONNECTION is re-sent on a change (CONNECTION_SPEED_CHANGE is still sent at SET_INTERFACE as before). Closes hathach#3760
The self-contained net_ncm fuzz harness #includes ncm_device.c and stubs the usbd symbols it references rather than linking the device stack. tud_network_link_state() now calls usbd_defer_func(), so add a matching no-op stub to keep the harness linking.
morse-cedricvandenbergh
force-pushed
the
fix/ncm-link-state-notify-retry
branch
from
362c719 to
8abd13b
Compare
Hardware-in-the-loop (HIL) Test Reporthfp.json✅ 52 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb.json✅ 314 passed · ❌ 25 failed · ⚪ 11 skipped · blank not run
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ncm: retry link-state notification, fix carrier lost on collision
Closes #3760
Problem
tud_network_link_state()delivers the CDC-NCMNETWORK_CONNECTIONnotification in an edge-triggered, fire-once way. If a previous notification is still in flight when the link state changes, the new notification is silently dropped and never retried, so the host keeps the previously reported carrier state. In practice a link up can be lost, leaving the host interface permanently atNO-CARRIER(kernel drops all TX → DHCP fails) even though the device thinks the link is up.Current code:
Three issues combine:
notification_xmit_is_running),notification_xmit(false)returns and theNETWORK_CONNECTIONfor the new state is never queued.link_is_upis committed before the dropped send, so a later call with the same value short-circuits at (A). Nothing re-attempts it → the host's lastNETWORK_CONNECTIONkeeps the oldwValue(e.g.0from a prior link-down) → permanentNO-CARRIER.notification_xmit_state/_is_running/link_is_upare mutated from bothtud_network_link_state()(caller context) andnetd_xfer_cb()(usbd context) with no serialisation. On RTOS ports wheretud_network_link_state()runs in a task other thantud_task(), the two race — which is what non-deterministically produces the in-flight collision above.An in-flight notification is present at a transition when, e.g., a preceding link-down notification is still completing, or the
SET_INTERFACE(alt=1)speed→connection chain (after a bus reset / re-enumeration) is still in flight when a link-state change arrives. Rapid link toggling and/or a bus reset around the transition make this likely.It is largely latent for single-threaded/super-loop apps that don't toggle the link rapidly, but reproducible for multi-threaded RTOS apps that drive link state from a dedicated task.
Fix
Make link-state delivery level-triggered and race-safe:
link_notify_pending: set whenlink_is_upchanges, cleared only when aNETWORK_CONNECTIONcarrying the current state is actually submitted.netd_xfer_cb(): if pending and the channel is free, (re)send — so a collided send is retried on the next completion instead of dropped.usbd_defer_func(), so a caller on another task cannot race the completion callback.Net: the
NETWORK_CONNECTIONfor the current link state is guaranteed to reach the host — immediately if the channel is free, otherwise on the next completion — regardless of caller context or in-flight collisions.Behavioural note for reviewers
On a link change this now re-sends only
NETWORK_CONNECTION, whereas the previous code reset toNOTIFICATION_SPEEDand re-sentCONNECTION_SPEED_CHANGEas well. A link toggle doesn't change the link speed, andCONNECTION_SPEED_CHANGEis still sent atSET_INTERFACE, so this is intentional (and is the variant that was validated). If strict behaviour-preservation is preferred,ncm_flush_link_notification()can reset toNOTIFICATION_SPEEDinstead — a one-line change.Testing
tud_network_link_state()from a separate task with frequent link down/up transitions plus USB re-enumeration around them: intermittent permanentNO-CARRIERafter a link-up, DHCP failing on the host.NETWORK_CONNECTION(wValue=1)and the host acquired DHCP. Instrumentation counters confirmed the previously-dropped case is now retried and delivered.Notes
src/class/net/ncm_device.c.6d3d33d…); this is not a regression fix for that change.