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

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

On Thu, 29 Jan 2026 07:54:50 -0800
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.

Minor thing but why wrap to 60 ish chars?  Convention for much of the kernel
is 75 (to give a bit of room for git tools deciding to indent it
a little).

> 
> 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>
Hi Pavan,

Various comments inline.  Please also make sure to run a few
static analysis tools over the code. There seems to be a use
after free that is pretty obvious in here, so I'd expect that
to be picked up. That stuff is easier for tooling that humans!

Jonathan

> ---
>  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 | 339 +++++++++++-------
>  include/linux/bnxt/ulp.h                      |  25 +-
>  6 files changed, 273 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 4481d80cdfc2..a9bfbfabf121 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

>  static int bnxt_init_board(struct pci_dev *pdev, struct net_device *dev)
> @@ -16211,12 +16214,13 @@ static void bnxt_remove_one(struct pci_dev *pdev)
>  	if (BNXT_PF(bp))
>  		__bnxt_sriov_disable(bp);
>  
> -	bnxt_rdma_aux_device_del(bp);
> +	bnxt_aux_devices_del(bp);
>  
>  	unregister_netdev(dev);
>  	bnxt_ptp_clear(bp);
>  
> -	bnxt_rdma_aux_device_uninit(bp);
> +	bnxt_aux_devices_uninit(bp);
> +	bnxt_auxdev_id_free(bp, bp->auxdev_id);

I'm not following why there is a mix of devices and single
device ID freeing going on here. 
If we have a set of devices, how come there is only one of those ids?

Maybe it's just a naming thing and auxdev_id is only meant to be used
for this one auxdev that is already in the driver?

>  
>  	bnxt_free_l2_filters(bp, true);
>  	bnxt_free_ntp_fltrs(bp, true);
> @@ -16802,7 +16806,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	bnxt_set_tpa_flags(bp);
>  	bnxt_init_ring_params(bp);
>  	bnxt_set_ring_params(bp);
> -	bnxt_rdma_aux_device_init(bp);
> +	mutex_init(&bp->auxdev_lock);
> +	if (!bnxt_auxdev_id_alloc(bp))
> +		bnxt_aux_devices_init(bp);
>  	rc = bnxt_set_dflt_rings(bp, true);
>  	if (rc) {
>  		if (BNXT_VF(bp) && rc == -ENODEV) {
> @@ -16866,7 +16872,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	bnxt_dl_fw_reporters_create(bp);
>  
> -	bnxt_rdma_aux_device_add(bp);
> +	bnxt_aux_devices_add(bp);
>  
>  	bnxt_print_device_info(bp);
>  
> @@ -16874,7 +16880,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	return 0;
>  init_err_cleanup:
> -	bnxt_rdma_aux_device_uninit(bp);
> +	bbnxt_aux_devices_uninitnxt_aux_devices_uninit(bp);
> +	bnxt_auxdev_id_free(bp, bp->auxdev_id);
>  	bnxt_dl_unregister(bp);
>  init_err_dl:
>  	bnxt_shutdown_tc(bp);
> @@ -17008,9 +17015,10 @@ static int bnxt_resume(struct device *device)
>  
>  resume_exit:
>  	netdev_unlock(bp->dev);
> -	bnxt_ulp_start(bp, rc);
> -	if (!rc)
> +	if (!rc) {
> +		bnxt_ulp_start(bp);
>  		bnxt_reenable_sriov(bp);
> +	}
>  	return rc;
>  }
>  
> @@ -17190,9 +17198,10 @@ static void bnxt_io_resume(struct pci_dev *pdev)
>  		netif_device_attach(netdev);
>  
>  	netdev_unlock(netdev);
> -	bnxt_ulp_start(bp, err);
> -	if (!err)
> +	if (!err) {
> +		bnxt_ulp_start(bp);
>  		bnxt_reenable_sriov(bp);
> +	}
>  }
>  
>  static const struct pci_error_handlers bnxt_err_handler = {

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index fa513892db50..3097fc5755e6 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c

> +
> +static struct bnxt_aux_device bnxt_aux_devices[__BNXT_AUXDEV_MAX] = {{

const?  If not I'd be concerned that this will break badly if there
are multiple devices.


> +	.name		= "rdma",
> +}};

> @@ -227,20 +264,27 @@ EXPORT_SYMBOL(bnxt_send_msg);
>  
>  void bnxt_ulp_stop(struct bnxt *bp)
>  {
> -	struct bnxt_aux_priv *aux_priv = bp->aux_priv;
> -	struct bnxt_en_dev *edev = bp->edev;
> +	int i;
Similar to later comment, could move that into the loop init.
(assuming not some local netdev thing about not doing that?)
>  
> -	if (!edev)
> -		return;
> -
> -	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) {
> +	mutex_lock(&bp->auxdev_lock);
> +	for (i = 0; i < __BNXT_AUXDEV_MAX; i++) {

...

>  static void bnxt_aux_dev_release(struct device *dev)
> @@ -408,20 +462,25 @@ static void bnxt_aux_dev_release(struct device *dev)
>  		container_of(dev, struct bnxt_aux_priv, aux_dev.dev);
>  	struct bnxt *bp = netdev_priv(aux_priv->edev->net);
>  
> -	ida_free(&bnxt_aux_dev_ids, aux_priv->id);
>  	kfree(aux_priv->edev->ulp_tbl);
> -	bp->edev = NULL;
> +	bp->edev[aux_priv->id] = NULL;
>  	kfree(aux_priv->edev);
>  	kfree(aux_priv);
> -	bp->aux_priv = NULL;
> +	bp->aux_priv[aux_priv->id] = NULL;

UAF of aux_priv.
You'll need a local copy of aux_priv->id.

>  }
>  
> -void bnxt_rdma_aux_device_del(struct bnxt *bp)
> +void bnxt_aux_devices_del(struct bnxt *bp)
>  {
> -	if (!bp->edev)
> -		return;
> +	int idx;
>  
> -	auxiliary_device_delete(&bp->aux_priv->aux_dev);
> +	mutex_lock(&bp->auxdev_lock);
> +	for (idx = 0; idx < __BNXT_AUXDEV_MAX; idx++) {
> +		if (bnxt_auxdev_is_active(bp, idx)) {
> +			auxiliary_device_delete(&bp->aux_priv[idx]->aux_dev);
> +			bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_INIT);
> +		}
> +	}
> +	mutex_unlock(&bp->auxdev_lock);
>  }
>  
>  static void bnxt_set_edev_info(struct bnxt_en_dev *edev, struct bnxt *bp)
> @@ -451,83 +510,105 @@ static void bnxt_set_edev_info(struct bnxt_en_dev *edev, struct bnxt *bp)
>  	edev->bar0 = bp->bar0;
>  }
>  
> -void bnxt_rdma_aux_device_add(struct bnxt *bp)
> +void bnxt_aux_devices_add(struct bnxt *bp)
>  {
>  	struct auxiliary_device *aux_dev;
> -	int rc;
> -
> -	if (!bp->edev)
> -		return;
> -
> -	aux_dev = &bp->aux_priv->aux_dev;
> -	rc = auxiliary_device_add(aux_dev);
> -	if (rc) {
> -		netdev_warn(bp->dev, "Failed to add auxiliary device for ROCE\n");
> -		auxiliary_device_uninit(aux_dev);
> -		bp->flags &= ~BNXT_FLAG_ROCE_CAP;
> +	int rc, idx;
> +
> +	mutex_lock(&bp->auxdev_lock);
> +	for (idx = 0; idx < __BNXT_AUXDEV_MAX; idx++) {
> +		if (bnxt_auxdev_is_init(bp, idx)) {
I haven't checked later patches, but if you can I'd do
		if (!bnxt_auxdev_is_init(bp, idx))
			continue;

		aux_dev = ..

Just to reduce indent without it making much different otherwise to readability.

> +			aux_dev = &bp->aux_priv[idx]->aux_dev;
> +			rc = auxiliary_device_add(aux_dev);
> +			if (rc) {
> +				netdev_warn(bp->dev, "Failed to add auxiliary device for ROCE\n");
> +				auxiliary_device_uninit(aux_dev);
> +				if (idx == BNXT_AUXDEV_RDMA)
> +					bp->flags &= ~BNXT_FLAG_ROCE_CAP;

Similar to below. I'd like this type specific stuff wrapped up in callbacks or
data.

> +				continue;
> +			}
> +			bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_ADD);
> +		}
>  	}
> +	mutex_unlock(&bp->auxdev_lock);
>  }
>  
> -void bnxt_rdma_aux_device_init(struct bnxt *bp)
> +void bnxt_aux_devices_init(struct bnxt *bp)
>  {
>  	struct auxiliary_device *aux_dev;
>  	struct bnxt_aux_priv *aux_priv;
>  	struct bnxt_en_dev *edev;
>  	struct bnxt_ulp *ulp;
> -	int rc;
> +	int rc, idx;
I'm not certain for net, but for most of the kernel it's now
acceptable to do
	for (int idx = 0; ...

to limit the scope of idx.

> +
> +	mutex_lock(&bp->auxdev_lock);
> +	for (idx = 0; idx < __BNXT_AUXDEV_MAX; idx++) {
It might be worth considering a precursor patch to factor out
a bnxt_aux_devices_init_one()
that would be a no operation changes patch, then do this
on top.  Would make things a lot easier to follow than this diff.

> +		bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_NONE);
> +
> +		if (idx == BNXT_AUXDEV_RDMA &&
> +		    !(bp->flags & BNXT_FLAG_ROCE_CAP))
> +			continue;
I'd find the code easier to read if the few bits of device
specific stuff are via callbacks or even better data in struct bnxt_aux_device.
Having a mix of stuff that is in there and checks against specific IDs
in the code just tends to end up messy if more devices get added over time.

> +
> +		aux_priv = kzalloc(sizeof(*aux_priv), GFP_KERNEL);
> +		if (!aux_priv)
> +			goto next_auxdev;
> +
> +		aux_dev = &aux_priv->aux_dev;
> +		aux_dev->id = bp->auxdev_id;
> +		aux_dev->name = bnxt_aux_devices[idx].name;
> +		aux_dev->dev.parent = &bp->pdev->dev;
> +		aux_dev->dev.release = bnxt_aux_dev_release;

		Could skip the zeroing at allocation in favor of the follow.
		For me this is easier to read but that's just personal preference
		so up to you.

		
		aux_priv = (struct bnxt_aux_priv) {
			.aux_dev = {
				.id = bp->auxdev_id,
				.name = bnxt_aux_devices[idx].name,
				.dev = {
					.parent = &bp->pdev->dev,
					.release = bnxt_aux_dev_release,
				},
			},
		};
> +
> +		rc = auxiliary_device_init(aux_dev);
> +		if (rc) {
> +			kfree(aux_priv);
> +			goto next_auxdev;
> +		}
> +		bp->aux_priv[idx] = aux_priv;
>  
> -	if (!(bp->flags & BNXT_FLAG_ROCE_CAP))
> -		return;
> +		/* From this point, all cleanup will happen via the .release
> +		 * callback & any error unwinding will need to include a call
> +		 * to auxiliary_device_uninit.
> +		 */
> +		edev = kzalloc(sizeof(*edev), GFP_KERNEL);
> +		if (!edev)
> +			goto aux_dev_uninit;
>  
> -	aux_priv = kzalloc(sizeof(*bp->aux_priv), GFP_KERNEL);
> -	if (!aux_priv)
> -		goto exit;
> +		aux_priv->edev = edev;
> +		bnxt_set_edev_info(edev, bp);
>  
> -	aux_priv->id = ida_alloc(&bnxt_aux_dev_ids, GFP_KERNEL);
> -	if (aux_priv->id < 0) {
> -		netdev_warn(bp->dev,
> -			    "ida alloc failed for ROCE auxiliary device\n");
> -		kfree(aux_priv);
> -		goto exit;
> -	}
> +		ulp = kzalloc(sizeof(*ulp), GFP_KERNEL);
> +		if (!ulp)
> +			goto aux_dev_uninit;
>  
> -	aux_dev = &aux_priv->aux_dev;
> -	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;
> +		edev->ulp_tbl = ulp;
> +		bp->edev[idx] = edev;
> +		if (idx == BNXT_AUXDEV_RDMA)

Another thing I'd like to see as a callback registered in the per
auxdev type data alongside the name etc.

> +			bp->ulp_num_msix_want = bnxt_set_dflt_ulp_msix(bp);
> +		aux_priv->id = idx;
> +		bnxt_auxdev_set_state(bp, idx, BNXT_ADEV_STATE_INIT);
>  
> -	rc = auxiliary_device_init(aux_dev);
> -	if (rc) {
> -		ida_free(&bnxt_aux_dev_ids, aux_priv->id);
> -		kfree(aux_priv);
> -		goto exit;
> +		continue;
> +aux_dev_uninit:
> +		auxiliary_device_uninit(aux_dev);
> +next_auxdev:
> +		if (idx == BNXT_AUXDEV_RDMA)
> +			bp->flags &= ~BNXT_FLAG_ROCE_CAP;

Similar to above, I'd prefer this to be either data or a callback that
is specific to the idx.

>  	}
> -	bp->aux_priv = aux_priv;
> -
> -	/* From this point, all cleanup will happen via the .release callback &
> -	 * any error unwinding will need to include a call to
> -	 * auxiliary_device_uninit.
> -	 */
> -	edev = kzalloc(sizeof(*edev), GFP_KERNEL);
> -	if (!edev)
> -		goto aux_dev_uninit;
> -
> -	aux_priv->edev = edev;
> -
> -	ulp = kzalloc(sizeof(*ulp), GFP_KERNEL);
> -	if (!ulp)
> -		goto aux_dev_uninit;
> +	mutex_unlock(&bp->auxdev_lock);
> +}
>  
> -	edev->ulp_tbl = ulp;
> -	bp->edev = edev;
> -	bnxt_set_edev_info(edev, bp);
> -	bp->ulp_num_msix_want = bnxt_set_dflt_ulp_msix(bp);