On Sat, Jan 07, 2023 at 07:02:00PM -0800, Ajit Khaparde wrote:
>
> Ajit Khaparde <[email protected]>
> Dec 7, 2022, 9:53 AM
> to me, andrew.gospodarek, davem, edumazet, jgg, kuba, leon, linux-kernel, linux-rdma, michael.chan, netdev, pabeni, selvin.xavier
>
> Add auxiliary device driver for Broadcom devices.
> The bnxt_en driver will register and initialize an aux device
> if RDMA is enabled in the underlying device.
> The bnxt_re driver will then probe and initialize the
> RoCE interfaces with the infiniband stack.
>
> We got rid of the bnxt_en_ops which the bnxt_re driver used to
> communicate with bnxt_en.
> Similarly We have tried to clean up most of the bnxt_ulp_ops.
> In most of the cases we used the functions and entry points provided
> by the auxiliary bus driver framework.
> And now these are the minimal functions needed to support the functionality.
>
> We will try to work on getting rid of the remaining if we find any
> other viable option in future.
>
> v1->v2:
> - Incorporated review comments including usage of ulp_id &
> complex function indirections.
> - Used function calls provided by the auxiliary bus interface
> instead of proprietary calls.
> - Refactor code to remove ROCE driver's access to bnxt structure.
>
> v2->v3:
> - Addressed review comments including cleanup of some unnecessary wrappers
> - Fixed warnings seen during cross compilation
>
> v3->v4:
> - Cleaned up bnxt_ulp.c and bnxt_ulp.h further
> - Removed some more dead code
> - Sending the patchset as a standalone series
>
> v4->v5:
> - Removed the SRIOV config callback which bnxt_en driver was calling into
> bnxt_re driver.
> - Removed excessive checks for rdev and other pointers.
>
> v5->v6:
> - Removed excessive checks for dev and other pointers
> - Remove runtime interrupt vector allocation. bnxt_en preallocates
> interrupt vectors for bnxt_re to use.
>
> Please apply. Thanks.
>
> Ajit Khaparde (7):
> bnxt_en: Add auxiliary driver support
> RDMA/bnxt_re: Use auxiliary driver interface
> bnxt_en: Remove usage of ulp_id
> bnxt_en: Use direct API instead of indirection
> bnxt_en: Use auxiliary bus calls over proprietary calls
> RDMA/bnxt_re: Remove the sriov config callback
> bnxt_en: Remove runtime interrupt vector allocation
>
> Hongguang Gao (1):
> bnxt_en: Remove struct bnxt access from RoCE driver
>
Thanks a lot for pursuing it,
Reviewed-by: Leon Romanovsky <[email protected]>
On Sat, 7 Jan 2023 19:02:01 -0800 Ajit Khaparde wrote:
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index 2e54bf4fc7a7..6c697172f042 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> @@ -25,32 +25,37 @@
> #include "bnxt_hwrm.h"
> #include "bnxt_ulp.h"
>
> +static DEFINE_IDA(bnxt_aux_dev_ids);
> +
> static int bnxt_register_dev(struct bnxt_en_dev *edev, unsigned int ulp_id,
> struct bnxt_ulp_ops *ulp_ops, void *handle)
> {
> struct net_device *dev = edev->net;
> struct bnxt *bp = netdev_priv(dev);
> struct bnxt_ulp *ulp;
> + int rc = 0;
>
> - ASSERT_RTNL();
> if (ulp_id >= BNXT_MAX_ULP)
> return -EINVAL;
>
> ulp = &edev->ulp_tbl[ulp_id];
> if (rcu_access_pointer(ulp->ulp_ops)) {
> netdev_err(bp->dev, "ulp id %d already registered\n", ulp_id);
> - return -EBUSY;
> + rc = -EBUSY;
> + goto exit;
The change to jump to the return statement rater than return directly
seems unrelated to the rest of the patch, and wrong.
> }
> if (ulp_id == BNXT_ROCE_ULP) {
> unsigned int max_stat_ctxs;
>
> max_stat_ctxs = bnxt_get_max_func_stat_ctxs(bp);
> if (max_stat_ctxs <= BNXT_MIN_ROCE_STAT_CTXS ||
> - bp->cp_nr_rings == max_stat_ctxs)
> - return -ENOMEM;
> + bp->cp_nr_rings == max_stat_ctxs) {
> + rc = -ENOMEM;
> + goto exit;
> + }
> }
>
> - atomic_set(&ulp->ref_count, 0);
> + atomic_set(&ulp->ref_count, 1);
> ulp->handle = handle;
> rcu_assign_pointer(ulp->ulp_ops, ulp_ops);
>
> @@ -59,7 +64,8 @@ static int bnxt_register_dev(struct bnxt_en_dev *edev, unsigned int ulp_id,
> bnxt_hwrm_vnic_cfg(bp, 0);
> }
>
> - return 0;
> +exit:
> + return rc;
> }
>
> static int bnxt_unregister_dev(struct bnxt_en_dev *edev, unsigned int ulp_id)
> @@ -69,10 +75,11 @@ static int bnxt_unregister_dev(struct bnxt_en_dev *edev, unsigned int ulp_id)
> struct bnxt_ulp *ulp;
> int i = 0;
>
> - ASSERT_RTNL();
> if (ulp_id >= BNXT_MAX_ULP)
> return -EINVAL;
>
> + edev->flags |= BNXT_EN_FLAG_ULP_STOPPED;
> +
> ulp = &edev->ulp_tbl[ulp_id];
> if (!rcu_access_pointer(ulp->ulp_ops)) {
> netdev_err(bp->dev, "ulp id %d not registered\n", ulp_id);
> @@ -126,7 +133,6 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id,
> int total_vecs;
> int rc = 0;
>
> - ASSERT_RTNL();
> if (ulp_id != BNXT_ROCE_ULP)
> return -EINVAL;
>
> @@ -149,6 +155,7 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id,
> max_idx = min_t(int, bp->total_irqs, max_cp_rings);
> idx = max_idx - avail_msix;
> }
> +
> edev->ulp_tbl[ulp_id].msix_base = idx;
> edev->ulp_tbl[ulp_id].msix_requested = avail_msix;
> hw_resc = &bp->hw_resc;
> @@ -156,8 +163,10 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, unsigned int ulp_id,
> if (bp->total_irqs < total_vecs ||
> (BNXT_NEW_RM(bp) && hw_resc->resv_irqs < total_vecs)) {
> if (netif_running(dev)) {
> + rtnl_lock();
What prevents the device from going down after you check running
but before you take the lock?
> bnxt_close_nic(bp, true, false);
> rc = bnxt_open_nic(bp, true, false);
> + rtnl_unlock();
> } else {
> rc = bnxt_reserve_rings(bp, true);
> }
> @@ -475,6 +467,143 @@ static const struct bnxt_en_ops bnxt_en_ops_tbl = {
> .bnxt_register_fw_async_events = bnxt_register_async_events,
> };
>
> +void bnxt_aux_dev_free(struct bnxt *bp)
> +{
> + kfree(bp->aux_dev);
> + bp->aux_dev = NULL;
> +}
> +
> +static struct bnxt_aux_dev *bnxt_aux_dev_alloc(struct bnxt *bp)
> +{
> + struct bnxt_aux_dev *bnxt_adev;
> +
> + bnxt_adev = kzalloc(sizeof(*bnxt_adev), GFP_KERNEL);
double space
> + if (!bnxt_adev)
> + return NULL;
> +
> + return bnxt_adev;
This entire function is rather pointless.
If you really want it - it can be simply written as:
static struct bnxt_aux_dev *bnxt_aux_dev_alloc(struct bnxt *bp)
{
return kzalloc(sizeof(struct bnxt_aux_dev), GFP_KERNEL);
}
> +}
> +
> +void bnxt_rdma_aux_device_uninit(struct bnxt *bp)
> +{
> + struct bnxt_aux_dev *bnxt_adev;
> + struct auxiliary_device *adev;
> +
> + /* Skip if no auxiliary device init was done. */
> + if (!(bp->flags & BNXT_FLAG_ROCE_CAP))
> + return;
> +
> + bnxt_adev = bp->aux_dev;
> + adev = &bnxt_adev->aux_dev;
> + auxiliary_device_delete(adev);
auxiliary_device_delete() waits for all the references to disappear?
The lifetime rules between adev and "edev" seem a little odd to me,
maybe I'm not familiar enough with auxdev.
> + auxiliary_device_uninit(adev);
> + if (bnxt_adev->id >= 0)
> + ida_free(&bnxt_aux_dev_ids, bnxt_adev->id);
> +}
> +
> +void bnxt_rdma_aux_device_init(struct bnxt *bp)
> +{
> + int rc;
> +
> + if (bp->flags & BNXT_FLAG_ROCE_CAP) {
flip the condition and return early, don't indent an entire function.
> + bp->aux_dev = bnxt_aux_dev_alloc(bp);
> + if (!bp->aux_dev)
> + goto skip_ida_init;
> +
> + bp->aux_dev->id = ida_alloc(&bnxt_aux_dev_ids, GFP_KERNEL);
> + if (bp->aux_dev->id < 0) {
> + netdev_warn(bp->dev,
> + "ida alloc failed for ROCE auxiliary device\n");
> + goto skip_aux_init;
> + }
> +
> + /* If aux bus init fails, continue with netdev init. */
> + rc = bnxt_rdma_aux_device_add(bp);
> + if (rc) {
> + netdev_warn(bp->dev,
> + "Failed to add auxiliary device for ROCE\n");
> + goto aux_add_failed;
> + }
> + }
> + return;
> +
> +aux_add_failed:
> + ida_free(&bnxt_aux_dev_ids, bp->aux_dev->id);
> + bp->aux_dev->id = -1;
> +skip_aux_init:
> + bnxt_aux_dev_free(bp);
> +skip_ida_init:
> + bp->flags &= ~BNXT_FLAG_ROCE_CAP;
> +}
> +static inline void bnxt_set_edev_info(struct bnxt_en_dev *edev, struct bnxt *bp)
Please don't use inline for no good reason.
> +{
> + edev->en_ops = &bnxt_en_ops_tbl;
> + edev->net = bp->dev;
> + edev->pdev = bp->pdev;
> + edev->l2_db_size = bp->db_size;
> + edev->l2_db_size_nc = bp->db_size;
> +
> + if (bp->flags & BNXT_FLAG_ROCEV1_CAP)
> + edev->flags |= BNXT_EN_FLAG_ROCEV1_CAP;
> + if (bp->flags & BNXT_FLAG_ROCEV2_CAP)
> + edev->flags |= BNXT_EN_FLAG_ROCEV2_CAP;
> +}
> +
> +int bnxt_rdma_aux_device_add(struct bnxt *bp)
> +{
> + struct bnxt_aux_dev *bnxt_adev = bp->aux_dev;
> + struct bnxt_en_dev *edev = bnxt_adev->edev;
> + struct auxiliary_device *aux_dev;
> + int ret;
> +
> + edev = kzalloc(sizeof(*edev), GFP_KERNEL);
> + if (!edev) {
> + ret = -ENOMEM;
> + goto cleanup_edev_failure;
> + }
> +
> + aux_dev = &bnxt_adev->aux_dev;
> + aux_dev->id = bnxt_adev->id;
> + aux_dev->name = "rdma";
> + aux_dev->dev.parent = &bp->pdev->dev;
> + aux_dev->dev.release = bnxt_aux_dev_release;
> +
> + bnxt_adev->edev = edev;
> + bp->edev = edev;
> + bnxt_set_edev_info(edev, bp);
> +
> + ret = auxiliary_device_init(aux_dev);
> + if (ret)
> + goto cleanup_init_failure;
> +
> + ret = auxiliary_device_add(aux_dev);
> + if (ret)
> + goto cleanup_add_failure;
> +
> + return 0;
> +
> +cleanup_add_failure:
Name your labels after what you clean up, not what failed.
> + auxiliary_device_uninit(aux_dev);
> +cleanup_init_failure:
> + kfree(edev);
> + bp->edev = NULL;
> +cleanup_edev_failure:
Don't jump to the return statement, just return.
> + return ret;
> +}
> +
> struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
> {
> struct bnxt *bp = netdev_priv(dev);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> index 42b50abc3e91..647147a68554 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> @@ -17,6 +17,7 @@
> #define BNXT_MIN_ROCE_STAT_CTXS 1
>
> struct hwrm_async_event_cmpl;
> +struct bnxt_aux_dev;
This forward declaration is not needed, at least in this patch.
> struct bnxt;
>
> struct bnxt_msix_entry {
> @@ -102,10 +103,14 @@ int bnxt_get_ulp_stat_ctxs(struct bnxt *bp);
> void bnxt_ulp_stop(struct bnxt *bp);
> void bnxt_ulp_start(struct bnxt *bp, int err);
> void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs);
> -void bnxt_ulp_shutdown(struct bnxt *bp);
> void bnxt_ulp_irq_stop(struct bnxt *bp);
> void bnxt_ulp_irq_restart(struct bnxt *bp, int err);
> void bnxt_ulp_async_events(struct bnxt *bp, struct hwrm_async_event_cmpl *cmpl);
> +void bnxt_aux_dev_release(struct device *dev);
> +int bnxt_rdma_aux_device_add(struct bnxt *bp);
This is only used in bnxt_ulp.c, please remove the declaration and make
it static. Please check other functions for the same problem.
> +void bnxt_rdma_aux_device_uninit(struct bnxt *bp);
> +void bnxt_rdma_aux_device_init(struct bnxt *bp);
> +void bnxt_aux_dev_free(struct bnxt *bp);
On Tue, Jan 10, 2023 at 2:51 PM Jakub Kicinski <[email protected]> wrote:
>
> On Sat, 7 Jan 2023 19:02:01 -0800 Ajit Khaparde wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
:::: snip ::::
>
> This forward declaration is not needed, at least in this patch.
>
> > struct bnxt;
> >
> > struct bnxt_msix_entry {
> > @@ -102,10 +103,14 @@ int bnxt_get_ulp_stat_ctxs(struct bnxt *bp);
> > void bnxt_ulp_stop(struct bnxt *bp);
> > void bnxt_ulp_start(struct bnxt *bp, int err);
> > void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs);
> > -void bnxt_ulp_shutdown(struct bnxt *bp);
> > void bnxt_ulp_irq_stop(struct bnxt *bp);
> > void bnxt_ulp_irq_restart(struct bnxt *bp, int err);
> > void bnxt_ulp_async_events(struct bnxt *bp, struct hwrm_async_event_cmpl *cmpl);
> > +void bnxt_aux_dev_release(struct device *dev);
> > +int bnxt_rdma_aux_device_add(struct bnxt *bp);
>
> This is only used in bnxt_ulp.c, please remove the declaration and make
> it static. Please check other functions for the same problem.
>
> > +void bnxt_rdma_aux_device_uninit(struct bnxt *bp);
> > +void bnxt_rdma_aux_device_init(struct bnxt *bp);
> > +void bnxt_aux_dev_free(struct bnxt *bp);
>
Thanks Kuba. I will go over the comments and send the next version.
Hi Ajit,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc3 next-20230112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ajit-Khaparde/bnxt_en-Add-auxiliary-driver-support/20230108-110519
patch link: https://lore.kernel.org/r/20230108030208.26390-2-ajit.khaparde%40broadcom.com
patch subject: [PATCH 1/8] bnxt_en: Add auxiliary driver support
config: i386-randconfig-a001-20220124
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/d04297da18739e2bc3c9262f658ad880d58a2051
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ajit-Khaparde/bnxt_en-Add-auxiliary-driver-support/20230108-110519
git checkout d04297da18739e2bc3c9262f658ad880d58a2051
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "auxiliary_device_init" [drivers/net/ethernet/broadcom/bnxt/bnxt_en.ko] undefined!
>> ERROR: modpost: "__auxiliary_device_add" [drivers/net/ethernet/broadcom/bnxt/bnxt_en.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests