From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 16660242D6C for ; Fri, 30 Jan 2026 11:38:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.176.79.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769773084; cv=none; b=Ml0YIdJtkjzJnyMS9vOyKuT1QVMt1Wl6l4glWzzWiZeCGaUdVuXpLAtb/RWBoxM02s1uVCo91zPwH90i/lzJifjxhC8WmXC3rfyGIC6xWryx9dS6bPnoFyiK0Y71OsazGogvtfOTKd0R++2lC9s8w2F7wV6OBRdsy65d+2PpChI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769773084; c=relaxed/simple; bh=x6p4cUEMDcKQJfhlgTCqwQFDSqPdLYHm/rNjf0ERiuc=; h=Date:From:To:CC:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=W7Gx8oyixfczGosr0DOrC1TF5n7h6AFkOspCG1wqEg/VFt/M94x4SUw7p/NBiJlgw8nxFc2EJjT+JjD8b7hg+alSI0MFwGXXt/FPnKpu96wdmDGqD4q/jtEKKN3QO/EGGmb2Rkp6RsSPEuFvKY22UtczhtyDRA7u44+cZXwSVIg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=185.176.79.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.224.150]) by frasgout.his.huawei.com (SkyGuard) with ESMTPS id 4f2Yrs4Hf7zJ46j5; Fri, 30 Jan 2026 19:37:17 +0800 (CST) Received: from dubpeml500005.china.huawei.com (unknown [7.214.145.207]) by mail.maildlp.com (Postfix) with ESMTPS id 5A29C40539; Fri, 30 Jan 2026 19:37:58 +0800 (CST) Received: from localhost (10.203.177.15) by dubpeml500005.china.huawei.com (7.214.145.207) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Fri, 30 Jan 2026 11:37:57 +0000 Date: Fri, 30 Jan 2026 11:37:56 +0000 From: Jonathan Cameron To: Pavan Chebbi CC: , , , , , , , , Subject: Re: [PATCH v3 fwctl 2/5] fwctl/bnxt_en: Refactor aux bus functions to be more generic Message-ID: <20260130113756.00002279@huawei.com> In-Reply-To: <20260129155453.3626544-3-pavan.chebbi@broadcom.com> References: <20260129155453.3626544-1-pavan.chebbi@broadcom.com> <20260129155453.3626544-3-pavan.chebbi@broadcom.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: lhrpeml100011.china.huawei.com (7.191.174.247) To dubpeml500005.china.huawei.com (7.214.145.207) On Thu, 29 Jan 2026 07:54:50 -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. 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 > Signed-off-by: Pavan Chebbi 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);