Re: [PATCH v2 fwctl 2/5] fwctl/bnxt_en: Refactor aux bus functions to be more generic

[Leon Romanovsky <leon@kernel.org>]

On Sun, Jan 25, 2026 at 09:37:07PM -0800, Pavan Chebbi wrote:
> Up until now there was only one auxiliary device that bnxt
> created and that was for RoCE driver. bnxt fwctl is also
> going to use an aux bus device that bnxt should create.
> This requires some nomenclature changes and refactoring of
> the existing bnxt aux dev functions.
> 
> Convert 'aux_priv' and 'edev' members of struct bnxt into
> arrays where each element contains supported auxbus device's
> data. Move struct bnxt_aux_priv from bnxt.h to ulp.h because
> that is where it belongs. Make aux bus init/uninit/add/del
> functions more generic which will loop through all the aux
> device types. Make bnxt_ulp_start/stop functions (the only
> other common functions applicable to any aux device) loop
> through the aux devices to update their config and states.
> Make callers of bnxt_ulp_start() call it only when there
> are no errors.
> 
> Also, as an improvement in code, bnxt_register_dev() can skip
> unnecessary dereferencing of edev from bp, instead use the
> edev pointer from the function parameter.
> 
> Future patches will reuse these functions to add an aux bus
> device for fwctl.
> 
> Reviewed-by: Andy Gospodarek <gospo@broadcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  47 ++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  19 +-
>  .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |   8 +-
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |   2 +-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 334 +++++++++++-------
>  include/linux/bnxt/ulp.h                      |  25 +-
>  6 files changed, 271 insertions(+), 164 deletions(-)

<...>

> +static void bnxt_auxdev_set_state(struct bnxt *bp, int idx, int state)
> +{
> +	mutex_lock(&bp->auxdev_lock);
> +	bp->auxdev_state[idx] = state;
> +	mutex_unlock(&bp->auxdev_lock);
> +}
> +
> +static bool bnxt_auxdev_is_init(struct bnxt *bp, int idx)
> +{
> +	bool ret;
> +
> +	mutex_lock(&bp->auxdev_lock);
> +	ret = (bp->auxdev_state[idx] == BNXT_ADEV_STATE_INIT);
> +	mutex_unlock(&bp->auxdev_lock);
> +	return ret;
> +}
> +
> +static bool bnxt_auxdev_is_active(struct bnxt *bp, int idx)
> +{
> +	bool ret;
> +
> +	mutex_lock(&bp->auxdev_lock);
> +	ret = (bp->auxdev_state[idx] == BNXT_ADEV_STATE_ADD);
> +	mutex_unlock(&bp->auxdev_lock);
> +	return ret;
> +}

<...>

>  void bnxt_ulp_stop(struct bnxt *bp)
>  {
> -	struct bnxt_aux_priv *aux_priv = bp->aux_priv;
> -	struct bnxt_en_dev *edev = bp->edev;
> -
> -	if (!edev)
> -		return;
> +	int i;
>  
> -	mutex_lock(&edev->en_dev_lock);
> -	if (!bnxt_ulp_registered(edev) ||
> -	    (edev->flags & BNXT_EN_FLAG_ULP_STOPPED))
> -		goto ulp_stop_exit;
> -
> -	edev->flags |= BNXT_EN_FLAG_ULP_STOPPED;
> -	if (aux_priv) {
> +	for (i = 0; i < __BNXT_AUXDEV_MAX; i++) {
> +		struct bnxt_aux_priv *aux_priv;
>  		struct auxiliary_device *adev;
> +		struct bnxt_en_dev *edev;
> +
> +		if (!bnxt_auxdev_is_active(bp, i))

It’s good that you stopped using atomic_t for state variables, but the
locking is still incorrect. It’s not enough to hold the lock only while
reading the state; you need to keep the lock for the entire period during
which the auxdev is expected to remain active.

Thanks