Re: [PATCH v18 00/11] net/sxe2: fix logic errors and address feedback

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 19 May 2026 22:47:59 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> This patch set addresses the feedback received on the v10 submission
> for the sxe2 PMD. The primary focus is on fixing vector path selection,
> ensuring memory safety during mbuf initialization, and cleaning up
> redundant logic in the configuration functions.

Great a couple more things to address before merge.

Review of [PATCH v18 00/11] sxe2 PMD
====================================

The v17 addendum issues are addressed in v18:

  - sxe2.ini features matrix now matches what the code advertises.
    VLAN offload, QinQ offload, Timestamp offload, Inner L3
    checksum, Inner L4 checksum, and FreeBSD rows are removed.
    All remaining "Y" / "P" rows correspond to entries in
    {rx,tx}_offload_capa or registered dev_ops callbacks.

  - sxe2_{rx,tx}_desciptor_status renamed to
    sxe2_{rx,tx}_descriptor_status throughout.

One new style issue and one general cleanup request:


Patch 09/11: drivers: add data path for Rx and Tx
-------------------------------------------------

Info: blank line added between function signature and opening
brace.  In the same hunk that renames the function:

  static int32_t sxe2_tx_descriptor_status(void *tx_queue, uint16_t offset)
  +
   {

There should be no blank line between the prototype line and
the body brace.  Looks like an editing artifact in this respin.


All patches: unnecessary (void) casts on void-returning calls
-------------------------------------------------------------

Warning: (void) casts on pthread_mutex_lock / pthread_mutex_unlock.

  (void)pthread_mutex_lock(&cdev->config.lock);
  ...
  (void)pthread_mutex_unlock(&cdev->config.lock);

These calls do return int, but in practice never fail when the
mutex is initialised with pthread_mutex_init(&m, NULL) — the
documented errors (EINVAL, EAGAIN, EDEADLK) all require either
an uninitialised mutex (programmer bug) or non-default
attributes (recursive / error-checking, neither of which is in
use here).  The cast is redundant noise.  Just write:

  pthread_mutex_lock(&cdev->config.lock);
  ...
  pthread_mutex_unlock(&cdev->config.lock);

This is inconsistently applied even within the same file:
sxe2_common_dev_create_by_pci() uses a bare pthread_mutex_unlock()
two lines after a (void)-cast pthread_mutex_lock() on the same
mutex.  Pick one style — bare calls — and apply it everywhere.

DPDK convention for void-returning callers is to call without
the (void) cast (see rte_spinlock_lock / rte_pktmbuf_free
usage across the tree).  The cast adds nothing and visually
clutters the lock/unlock pairs that are already busy enough.
Please drop all of them.

Series hygiene
--------------

The descriptor-status rename should be folded back into the
patch that introduces the function (patch 08), so the function
is correctly named at the commit it first appears.  Currently
patch 08 introduces sxe2_{rx,tx}_descriptor_status (good, fixed
from v17) but patch 09 still touches the function and its
hunks would have been cleaner if no rename was ever needed.  In
this case the fix landed in patch 08 already so it's fine —
just calling out the pattern.