Re: [PATCH net-next v2 2/6] bnxt_en: Refactor aux bus functions to be generic

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Tue, 23 Sep 2025 02:58:21 -0700
Pavan Chebbi <pavan.chebbi@broadcom.com> 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.
> 
> Make aux bus init/uninit/add/del functions generic which will
> accept aux device type as a parameter. Change aux_dev_ids to
> aux_dev_rdma_ids to mean it is for RoCE driver.
> 
> Also rename the 'aux_priv' and 'edev' members of struct bp to
> 'aux_priv_rdma' and 'edev_rdma' respectively, to mean they belong
> to rdma.
> Rename bnxt_aux_device_release() as bnxt_rdma_aux_device_release()
> 
> Future patches will reuse these functions to add an aux bus device
> for fwctl.
> 
Hi Pavan,

It might just be a question of patch break up, but the code here
doesn't really match with what you suggest when talking about making these
functions generic.  They still have a lot of what looks to be unconditional
RDMA specific code in them after this patch.

I think if this 'generic' approach makes sense this patch needs to
be much clearer on what is rdma specific than it currently is. I'm not
yet convinced that this approach is preferable to a few helper functions
(for the generic bits) that rdma and fwctl specific registration functions call.

Thanks,

Jonathan



> Reviewed-by: Andy Gospodarek <gospo@broadcom.com>
> Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index 992eec874345..665850753f90 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c

I stopped reading that this point as the same issue on how generic things are
continued and would have lead to many similar comments.

> @@ -465,7 +466,8 @@ void bnxt_rdma_aux_device_add(struct bnxt *bp)
>  	}
>  }
>  
> -void bnxt_rdma_aux_device_init(struct bnxt *bp)
> +void bnxt_aux_device_init(struct bnxt *bp,

This confuses me a bit.  The patch description says it will make them
generic and this has a bunch of code that really doesn't look generic.

> +			  enum bnxt_ulp_auxdev_type auxdev_type)
>  {
>  	struct auxiliary_device *aux_dev;
>  	struct bnxt_aux_priv *aux_priv;
> @@ -473,14 +475,15 @@ void bnxt_rdma_aux_device_init(struct bnxt *bp)
>  	struct bnxt_ulp *ulp;
>  	int rc;
>  
> -	if (!(bp->flags & BNXT_FLAG_ROCE_CAP))
> +	if (auxdev_type == BNXT_AUXDEV_RDMA &&
> +	    !(bp->flags & BNXT_FLAG_ROCE_CAP))
>  		return;
>  
> -	aux_priv = kzalloc(sizeof(*bp->aux_priv), GFP_KERNEL);
> +	aux_priv = kzalloc(sizeof(*bp->aux_priv_rdma), GFP_KERNEL);
>  	if (!aux_priv)
>  		goto exit;
>  
> -	aux_priv->id = ida_alloc(&bnxt_aux_dev_ids, GFP_KERNEL);
> +	aux_priv->id = ida_alloc(&bnxt_rdma_aux_dev_ids, GFP_KERNEL);
>  	if (aux_priv->id < 0) {
>  		netdev_warn(bp->dev,
>  			    "ida alloc failed for ROCE auxiliary device\n");
> @@ -492,15 +495,15 @@ void bnxt_rdma_aux_device_init(struct bnxt *bp)
>  	aux_dev->id = aux_priv->id;
>  	aux_dev->name = "rdma";
>  	aux_dev->dev.parent = &bp->pdev->dev;
> -	aux_dev->dev.release = bnxt_aux_dev_release;
> +	aux_dev->dev.release = bnxt_rdma_aux_dev_release;

Another call that looks very rmda specific.
I would put these all under conditional checks even if that means
that if any other value is passed in for type the function doesn't
yet do anything useful.

>  
>  	rc = auxiliary_device_init(aux_dev);
>  	if (rc) {
> -		ida_free(&bnxt_aux_dev_ids, aux_priv->id);
> +		ida_free(&bnxt_rdma_aux_dev_ids, aux_priv->id);
>  		kfree(aux_priv);
>  		goto exit;
>  	}
> -	bp->aux_priv = aux_priv;
> +	bp->aux_priv_rdma = aux_priv;

As below. This feels like an odd thing to not make conditional on the type.

>  
>  	/* From this point, all cleanup will happen via the .release callback &
>  	 * any error unwinding will need to include a call to
> @@ -517,9 +520,10 @@ void bnxt_rdma_aux_device_init(struct bnxt *bp)
>  		goto aux_dev_uninit;
>  
>  	edev->ulp_tbl = ulp;
> -	bp->edev = edev;
> +	bp->edev_rdma = edev;

This seems to have a slightly odd mix of conditional assignment like
the ulp_num_msix_want below and unconditional assignment of clearly
RDMA specific things like evdev_rdma.

>  	bnxt_set_edev_info(edev, bp);
> -	bp->ulp_num_msix_want = bnxt_set_dflt_ulp_msix(bp);
> +	if (auxdev_type == BNXT_AUXDEV_RDMA)
> +		bp->ulp_num_msix_want = bnxt_set_dflt_ulp_msix(bp);
>  
>  	return;
>  
> diff --git a/include/linux/bnxt/ulp.h b/include/linux/bnxt/ulp.h
> index 7b9dd8ebe4bc..baac0dd44078 100644
> --- a/include/linux/bnxt/ulp.h
> +++ b/include/linux/bnxt/ulp.h
> @@ -20,6 +20,11 @@
>  struct hwrm_async_event_cmpl;
>  struct bnxt;
>  
> +enum bnxt_ulp_auxdev_type {
> +	BNXT_AUXDEV_RDMA = 0,
> +	__BNXT_AUXDEV_MAX,

Trivial but no point in a trailing comma after a entry that will always
terminate this list (like this one).  Having the comma just makes an
accidental addition of stuff after this harder to spot!


> +};