|SUCCESS| [v2,6/6] net/hinic: fix mutexes for multi-process

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163036

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-15

git sha: series.patch (multiple commits)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: Multiple patches fixing process-shared mutex initialization

This series addresses improper mutex initialization across multiple DPDK
drivers and libraries. The patches add PTHREAD_PROCESS_SHARED attribute
to mutexes protecting data structures in shared memory accessed by both
primary and secondary processes.

> +/*
> + * Initialize mutex for process-shared access.
> + * The rte_eth_dev_data structure is in shared memory, so any mutex
> + * protecting it must use PTHREAD_PROCESS_SHARED.
> + */
> +static void
> +eth_dev_flow_ops_mutex_init(pthread_mutex_t *mutex)
> +{
> +#ifndef RTE_EXEC_ENV_WINDOWS
> +	pthread_mutexattr_t attr;
> +
> +	pthread_mutexattr_init(&attr);
> +	pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_mutex_init(mutex, &attr);
> +	pthread_mutexattr_destroy(&attr);
> +#else
> +	pthread_mutex_init(mutex, NULL);
> +#endif
> +}

Does pthread_mutexattr_init() ever fail? The return value is ignored
here and in multiple other patches, but POSIX allows for failure. Same
concern applies to pthread_mutexattr_setpshared().

> +static void
> +axgbe_init_mutex(pthread_mutex_t *mutex)
> +{
> +	pthread_mutexattr_t attr;
> +
> +	pthread_mutexattr_init(&attr);
> +	pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_mutex_init(mutex, &attr);
> +	pthread_mutexattr_destroy(&attr);
> +}

Does pthread_mutex_init() propagate failure here? The return value is
ignored but several patches (net/bnxt patch 5, net/failsafe patch 2)
had previously checked the return value from pthread_mutex_init().

[ ... ]

> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index 3e590d38f7..aa3fe53b22 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -144,11 +144,20 @@ fs_mutex_init(struct fs_priv *priv)
>  	/* Allow mutex relocks for the thread holding the mutex. */
>  	ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
>  	if (ret) {
> -		ERROR("Cannot set mutex type - %s", strerror(ret));
> -		return ret;
> +		ERROR("Cannot set mutex recursive - %s", strerror(ret));
> +		goto out;
> +	}
> +	/* Allow mutex to be shared between processes. */
> +	ret = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	if (ret) {
> +		ERROR("Cannot set mutex pshared - %s", strerror(ret));
> +		goto out;
>  	}
> +	pthread_mutex_init(&priv->hotplug_mutex, &attr);
>  
> -	return pthread_mutex_init(&priv->hotplug_mutex, &attr);
> +out:
> +	pthread_mutexattr_destroy(&attr);
> +	return ret;
>  }

Does this code check pthread_mutex_init() return value? The original
code returned it directly, but after the patch the return value from
pthread_mutex_init() is lost.

[ ... ]

> diff --git a/drivers/net/bnxt/bnxt_txq.c b/drivers/net/bnxt/bnxt_txq.c
> index 7752f06eb7..8c834acb1d 100644
> --- a/drivers/net/bnxt/bnxt_txq.c
> +++ b/drivers/net/bnxt/bnxt_txq.c
> @@ -204,7 +204,8 @@ int bnxt_tx_queue_setup_op(struct rte_eth_dev *eth_dev,
>  		goto err;
>  	}
>  
> -	return pthread_mutex_init(&txq->txq_lock, NULL);
> +	bnxt_init_mutex(&txq->txq_lock);
> +	return 0;
>  err:
>  	bnxt_tx_queue_release_op(eth_dev, queue_idx);
>  	return rc;

Does this code ignore pthread_mutex_init() failures now? Previously
bnxt_tx_queue_setup_op() would return the error code from
pthread_mutex_init(), but after the patch it unconditionally returns 0.

[ ... ]

> +static void
> +atl_init_mutex(pthread_mutex_t *mutex)
> +{
> +	pthread_mutexattr_t attr;
> +
> +	pthread_mutexattr_init(&attr);
> +	pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_mutex_init(mutex, &attr);
> +	pthread_mutexattr_destroy(&attr);
> +}

Does this wrapper match the calling pattern? All pthread API calls can
fail, but the wrapper discards error information that callers might
need.

> diff --git a/drivers/net/hinic/base/hinic_compat.h b/drivers/net/hinic/base/hinic_compat.h
> index d3994c50e9..18148a119e 100644
> --- a/drivers/net/hinic/base/hinic_compat.h
> +++ b/drivers/net/hinic/base/hinic_compat.h
> @@ -197,10 +197,21 @@ static inline u16 ilog2(u32 n)
>  	return res;
>  }
>  
> +/*
> + * Initialize mutex for process-shared access.
> + * Structures may be in shared memory accessible by multiple processes,
> + * so mutexes must use PTHREAD_PROCESS_SHARED.
> + */
>  static inline int hinic_mutex_init(pthread_mutex_t *pthreadmutex,
>  					const pthread_mutexattr_t *mattr)
>  {
> -	return pthread_mutex_init(pthreadmutex, mattr);
> +	pthread_mutexattr_t attr;
> +
> +	pthread_mutexattr_init(&attr);
> +	pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_mutex_init(pthreadmutex, mattr ? mattr : &attr);
> +	pthread_mutexattr_destroy(&attr);
> +	return 0;
>  }

Does this code leak pthread_mutexattr_t when mattr is non-NULL? When
the caller-provided mattr is used, the locally initialized attr is
destroyed anyway without being used in pthread_mutex_init().