On Wed, 25 Feb 2026 11:52:25 +0000 Anatoly Burakov <[email protected]> wrote:
> Currently, device hotplug is not documented except in API headers. Add > documentation for device hotplug, both for user facing side of it, and a > high level overview of its internal workings. > > Signed-off-by: Anatoly Burakov <[email protected]> > --- > > Notes: > v2: > - Removed "summary" section > > doc/guides/prog_guide/dev_args.rst | 3 + > doc/guides/prog_guide/device_hotplug.rst | 286 +++++++++++++++++++++++ > doc/guides/prog_guide/index.rst | 1 + > 3 files changed, 290 insertions(+) > create mode 100644 doc/guides/prog_guide/device_hotplug.rst This is great to see. It would be the flow from kernel to udev/systemd was explained. If I remember right, mlx does this through another IB mechanism. Also mention this is Linux only. AI review is good at finding grammar and other things. --- **Overall assessment:** This is a solid and welcome addition. Device hotplug has long needed proper documentation beyond the API headers. The structure is logical, the code examples are helpful, and the multi-process synchronization section is particularly well done. A few issues worth addressing: **Technical issues:** 1. **`device_event_callback` return type mismatch (line 289):** The callback is declared as returning `int`, but `rte_dev_event_cb_fn` is typedef'd as returning `void`. This will cause a compiler warning/error if anyone copies this example verbatim. Should be `void` with no return statement. 2. **Missing error check on `rte_dev_probe()` (line 205):** The "Device Lifecycle Example" calls `rte_dev_probe(devargs)` without checking the return value, while the "Basic Usage" example above it correctly shows error handling. For a reference example demonstrating "proper device lifecycle management," this is inconsistent. Should check the return. 3. **`port_id` type (line 203):** `port_id` is declared as `unsigned` but `RTE_ETH_FOREACH_MATCHING_DEV` expects `uint16_t`. Should be `uint16_t port_id;`. 4. **`rte_eal_hotplug_add()`/`rte_eal_hotplug_remove()` deprecation status (line 179):** Are these functions deprecated or planned for deprecation in favor of `rte_dev_probe()`/`rte_dev_remove()`? If so, worth mentioning. If not, it might still be good to indicate which API is preferred for new code. **Documentation nits:** 5. **Line 152:** "after the initial EAL initialization has completed" — consider noting that `rte_eal_init()` must have returned successfully, since that's the specific precondition. 6. **Line 224:** "to find newly attached port" → "to find the newly attached port" 7. **Line 232:** "In addition to providing generic hotplug infrastructure to be used in the above described manner" — a bit wordy. Consider: "In addition to the hotplug APIs described above, EAL also offers a device event infrastructure." 8. **Line 269:** "When ``RTE_DEV_EVENT_REMOVE`` event is delivered, the EAL has already released all internal resources associated with the device." — Is this accurate? My understanding is that the uevent notification tells the application the kernel removed the device, but it's still up to the application to call `rte_dev_remove()` to clean up EAL resources. If the EAL had already cleaned up, there'd be nothing for the application to do. Please double-check this claim. 9. **Line 412, nl_groups:** The doc says `nl_groups = 0xffffffff`. This is an implementation detail that could change and may be more confusing than helpful for users reading the prog guide. Consider dropping it or noting it's an implementation note. **Structural:** 10. **Placement in index.rst:** The new page is added right after `dev_args` under "Device Libraries," which makes sense given the cross-references. Good. 11. **The "Implementation Details" section** (line 308 onward) switches from user-facing guidance to internal architecture. This is useful for contributors but might be better as a separate subsection with a note that it describes internals. Currently the document transitions from "here's how to use it" to "here's how it works internally" without much signaling. **Minor:** - Copyright says 2025 (line 142) — should be 2026 given the submission date.
