Re: [PATCH v17 01/13] net/sxe: add base driver directory and doc

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  9 Feb 2026 09:24:46 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> Adding a minimum maintainable directory structure for the
> network driver and request maintenance of the sxe driver.
> 
> Signed-off-by: Jie Liu <liujie5@linkdatatechnology.com>
> ---

Next time please add a cover letter to large patch sets.

The AI review identified a long list of this that need changing.

# Review: [PATCH v17 01-13/13] net/sxe: Linkdata SXE NIC driver

## Errors

### 1. Patch 02 — `sxe_dev_configure()`: uninitialized return value

`sxe_ethdev.c` declares `s32 ret;` and immediately falls through to `l_end: return ret;` without ever assigning `ret`. This returns an indeterminate value to the caller.

```c
static s32 sxe_dev_configure(struct rte_eth_dev *dev)
{
    s32 ret;
    /* ... */
    PMD_INIT_FUNC_TRACE();

l_end:
    return ret;  /* ret never assigned */
}
```

**Fix:** Initialize `ret = 0` or assign it from a meaningful operation.

---

### 2. Patch 02 — `sxe_hdc_cmd_process()`: all error codes silently overwritten

At label `l_ret:`, the line `ret = pthread_sigmask(SIG_SETMASK, &old_mask, NULL);` unconditionally overwrites `ret`. Every error path in this function (sem_wait failure, retry loop exhaustion, packet trans errors) reaches `l_ret` and has its error code replaced with the return value of `pthread_sigmask` (typically 0). The subsequent check `if (ret == -SXE_HDC_RETRY_ERR)` is then dead code.

This means the HDC command channel silently swallows all errors and reports success to callers.

```c
l_up:
    sem_post(sxe_hdc_sema_get());
l_ret:
    ret = pthread_sigmask(SIG_SETMASK, &old_mask, NULL);  /* overwrites error */
    if (ret)
        LOG_ERROR_BDF("...");

    if (ret == -SXE_HDC_RETRY_ERR)  /* dead code */
        ret = -EFAULT;

    return ret;
```

**Fix:** Save the original `ret` before restoring the signal mask and return it:
```c
l_up:
    sem_post(sxe_hdc_sema_get());
l_ret:
    if (pthread_sigmask(SIG_SETMASK, &old_mask, NULL) != 0)
        LOG_ERROR_BDF("...");

    if (ret == -SXE_HDC_RETRY_ERR)
        ret = -EFAULT;

    return ret;
```

---

### 3. Patch 13 — `sxevf_dev_stop()`: stop flag set to wrong value

The function sets `adapter->stop = false` when the device is being stopped. This should be `true`. The guard at the top `if (adapter->stop)` will therefore never trigger, and the device is never marked as stopped.

```c
static s32 sxevf_dev_stop(struct rte_eth_dev *dev)
{
    ...
    if (adapter->stop) {
        LOG_INFO_BDF("eth dev has been stopped.");
        goto l_out;
    }

    adapter->stop = false;  /* BUG: should be true */
```

**Fix:** `adapter->stop = true;`

---

### 4. Patch 04 — `sxe_dev_start()`: unsupported loopback mode leaks resources

After `sxe_rx_configure()`, `sxe_irq_configure()`, and `sxe_txrx_start()` have all run, an unsupported `lpbk_mode` value causes a `goto l_end` that skips the `l_error` cleanup path. This leaks IRQ vectors and queue resources.

```c
    sxe_txrx_start(dev);
    ...
    } else {
        ret = -ENOTSUP;
        PMD_LOG_ERR(INIT, "unsupported loopback mode:%u.",
                dev->data->dev_conf.lpbk_mode);
        goto l_end;  /* should be goto l_error */
    }
```

**Fix:** Change `goto l_end` to `goto l_error`.

---

### 5. Patch 02 — `hdc_packet_send()` / `hdc_resp_data_rcv()`: unaligned 32-bit memory access

Both functions cast arbitrary byte-offset pointers to `u32 *` and dereference them:

```c
pkg_data = *(u32 *)(data + offset);     /* hdc_packet_send */
*(u32 *)(out_data + offset) = pkt_data; /* hdc_resp_data_rcv */
```

If `data`/`out_data` is not 4-byte aligned, this causes undefined behavior and may fault on ARM (ARMv8 is listed as a supported architecture in `sxe.ini`).

**Fix:** Use `memcpy` for these accesses, or use `rte_le_to_cpu_32`/`rte_cpu_to_le_32` with `memcpy`.

---

### 6. Patch 02 — `sxe_fw_time_sync()`: error silently dropped

The return value from `sxe_fw_time_sync_process()` is stored in `ret_v` but the function returns `ret` which remains 0 on this path, so the caller never sees the failure.

```c
    ret_v = sxe_fw_time_sync_process(hw);
    if (ret_v) {
        LOG_WARN_BDF("fw time sync failed, ret_v=%d", ret_v);
        goto l_ret;
    }

l_ret:
    return ret;  /* ret is still 0 */
```

**Fix:** Use `ret` instead of `ret_v`, or set `ret = ret_v` before the goto.

---

## Warnings

### 1. Patch 04 — Subject line contains commas

"net/sxe: add link, flow ctrl, mac ops, mtu ops function" — commas are forbidden punctuation in DPDK subject lines per `check-git-log.sh`. Consider splitting this patch or rephrasing (e.g., "net/sxe: add link and mac layer operations").

---

### 2. Patch 02 — Commit message body contains non-ASCII characters

"Add basic modules: logs、 hardware communication、common components" — the `、` characters are Chinese punctuation (U+3001). Use standard ASCII commas.

---

### 3. Patch 01 — Release note says "Updated" for a new driver

The release note reads `**Updated Linkdata sxe ethernet driver.**` but this is a new driver, not an update. It should use the `**Added**` or `**New**` convention (e.g., `**Added Linkdata sxe ethernet driver.**`), matching the `New Features` section it's placed in.

---

### 4. Patch 02 — `rte_zmalloc()` used for control-path temporary buffers

In `sxe_driver_cmd_trans()`, `rte_zmalloc()` allocates temporary HDC request/response buffers. These don't need hugepage memory (no DMA, no shared-memory requirement). Standard `calloc()` would be faster and not consume limited hugepage resources.

---

### 5. Patch 02 — Global HDC semaphore shared across all PCI devices

`g_hdc_sem` is a single global semaphore. All SXE PFs serialize HDC firmware commands through it. If multiple SXE NICs are present, they will unnecessarily block each other. Consider a per-device semaphore.

---

### 6. Patch 02 — `sxe_irq_vec_free()` directly accesses `handle->intr_vec`

The function directly accesses `handle->intr_vec` instead of using the DPDK interrupt API (`rte_intr_vec_list_free()`). Direct field access may break with future DPDK versions where the interrupt handle is opaque.

---

## Info

- The series is at v17, indicating extensive prior review. The overall structure follows DPDK PMD conventions with proper SPDX headers, meson integration, feature documentation, and release notes.
- The base `sxe_hw.c` at ~6200 lines is large for a single file. Consider splitting register-group definitions from operational logic if practical.
- Several functions across the series have unused variables (e.g., `pci_dev` in `sxe_dev_infos_get` in patch 02, `link`/`num` declared early in `sxe_dev_stop` in patch 02). These will generate compiler warnings with `-Wunused-variable` depending on `#ifdef` configurations.