Re: [PATCH v4] bus/pci: don't open uio device in secondary process

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 13 Jan 2026 13:35:43 +0100
Konrad Sztyber <konrad.sztyber@gmail.com> wrote:

> From: Konrad Sztyber <konrad.sztyber@intel.com>
> 
> The uio_pci_generic driver clears the bus master bit when the device
> file is closed. So, when the secondary process terminates after probing
> a device, that device becomes unusable in the primary process.
> 
> To avoid that, the device file is now opened only in the primary process
> and the secondary gets it over UNIX domain socket via SCM_RIGHTS.
> 
> Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> ---

AI code review spotted one minor issue

## DPDK Patch Review: bus/pci: don't open uio device in secondary process

### Summary
This patch fixes a bug where the `uio_pci_generic` driver clears the bus master bit when the secondary process closes its UIO device file, rendering the device unusable in the primary process. The fix requests the fd from the primary process via SCM_RIGHTS instead of opening it directly in the secondary.

---

### Commit Message Review

| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ Pass | 50 characters |
| Lowercase after colon | ✅ Pass | |
| Correct prefix | ✅ Pass | `bus/pci:` is correct for `drivers/bus/pci/` |
| Imperative mood | ✅ Pass | "don't open" |
| No trailing period | ✅ Pass | |
| No forbidden punctuation | ✅ Pass | |
| Body ≤75 chars | ✅ Pass | All lines within limit |
| Body doesn't start with "It" | ✅ Pass | Starts with "The" |
| `Fixes:` tag present | ✅ Pass | `Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")` |
| `Fixes:` uses 12-char SHA | ✅ Pass | |
| `Cc: stable@dpdk.org` | ✅ Pass | Present for backport |
| `Signed-off-by:` present | ✅ Pass | Real name and valid email |
| Tag order correct | ✅ Pass | Fixes → Cc → (blank) → Signed-off-by |

---

### Code Style Review

| Check | Status | Notes |
|-------|--------|-------|
| Lines ≤100 chars | ✅ Pass | |
| Function return type on own line | ✅ Pass | `pci_uio_send_fd`, `pci_uio_request_fd` |
| Explicit NULL comparisons | ✅ Pass | `dev == NULL`, `dev != NULL` |
| Explicit integer comparisons | ✅ Pass | `fd < 0`, `rc != 0`, `pci_uio_dev_count == 0` |
| Naming conventions | ✅ Pass | lowercase with underscores throughout |
| Comment style | ✅ Pass | Multi-line block comment properly formatted |
| Include order | ✅ Pass | DPDK includes grouped appropriately |
| No trailing whitespace | ✅ Pass | |

---

### Warnings (should fix)

**1. Use of `assert()` instead of RTE_ASSERT or proper error handling**

```c
if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
    assert(pci_uio_dev_count > 0);
```

Standard C `assert()` is disabled when `NDEBUG` is defined and isn't consistent with DPDK style. Consider:
- Using `RTE_ASSERT(pci_uio_dev_count > 0);` for debug builds, or
- Adding explicit error handling if this is a condition that could occur in production

---

### Info (consider)

**1. Silent error handling in `pci_uio_send_fd`**

When the device isn't found or the fd is invalid, the function logs an error but sends an empty reply (num_fds=0). While the secondary process detects this, consider whether the reply should include an explicit error code in the param field for better diagnostics.

**2. Missing header include**

The code uses `assert()` but doesn't show `<assert.h>` being added. If `assert()` is retained, ensure the header is included. If switching to `RTE_ASSERT()`, `<rte_debug.h>` may be needed.

---

### Technical Review

The implementation approach is sound:

1. **Primary process**: Opens the UIO device and registers an MP action handler to send the fd to secondary processes
2. **Secondary process**: Requests the fd from primary via `rte_mp_request_sync` instead of opening it directly
3. **Cleanup**: Properly unregisters the MP action when the last UIO device is freed

The use of `rte_mp_*` infrastructure and SCM_RIGHTS for fd passing is the correct pattern for DPDK multi-process communication.

---

### Verdict

**Acceptable with minor changes** — The patch is well-structured and addresses a real bug correctly. The only substantive issue is the use of `assert()` which should be changed to DPDK-style error handling or `RTE_ASSERT()`.