[PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Changelog:
v2:
 * kbuild spotted that I didn't delete all code in patch #5, so deleted
   even more ulp_ops derefences.
v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
 * Go much deeper and removed useless ULP indirection
v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
-----------------------------------------------------------------------

The following series fixes issue spotted in [1], where bnxt_re driver
messed with module reference counting in order to implement symbol
dependency of bnxt_re and bnxt modules. All of this is done, when in
upstream we have only one ULP user of that bnxt module. The simple
declaration of exported symbol would do the trick.

This series removes that custom module_get/_put, which is not supposed
to be in the driver from the beginning and get rid of nasty indirection
logic that isn't relevant for the upstream code.

Such small changes allow us to simplify the bnxt code and my hope that
Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.

Thanks

[1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org

Leon Romanovsky (5):
  RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
  RDMA/bnxt_re: Create direct symbolic link between bnxt modules
  RDMA/bnxt_re: Get rid of custom module reference counting
  net/bnxt: Remove useless check of non-existent ULP id
  net/bnxt: Use direct API instead of useless indirection

 drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
 drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
 6 files changed, 119 insertions(+), 260 deletions(-)

-- 
2.30.2

[PATCH rdma-next v2 1/5] RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

The "select" kconfig keyword provides reverse dependency, however it
doesn't check that selected symbol meets its own dependencies. Usually
"select" is used for non-visible symbols, so instead of trying to keep
dependencies in sync with BNXT ethernet driver, simply "depends on" it,
like Kconfig documentation suggest.

* CONFIG_PCI is already required by BNXT
* CONFIG_NETDEVICES and CONFIG_ETHERNET are needed to chose BNXT

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/Kconfig | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/Kconfig b/drivers/infiniband/hw/bnxt_re/Kconfig
index 0feac5132ce1..6a17f5cdb020 100644
--- a/drivers/infiniband/hw/bnxt_re/Kconfig
+++ b/drivers/infiniband/hw/bnxt_re/Kconfig
@@ -2,9 +2,7 @@
 config INFINIBAND_BNXT_RE
 	tristate "Broadcom Netxtreme HCA support"
 	depends on 64BIT
-	depends on ETHERNET && NETDEVICES && PCI && INET && DCB
-	select NET_VENDOR_BROADCOM
-	select BNXT
+	depends on INET && DCB && BNXT
 	help
 	  This driver supports Broadcom NetXtreme-E 10/25/40/50 gigabit
 	  RoCE HCAs.  To compile this driver as a module, choose M here:
-- 
2.30.2

Re: [PATCH rdma-next v2 1/5] RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> The "select" kconfig keyword provides reverse dependency, however it
> doesn't check that selected symbol meets its own dependencies. Usually
> "select" is used for non-visible symbols, so instead of trying to keep
> dependencies in sync with BNXT ethernet driver, simply "depends on" it,
> like Kconfig documentation suggest.
>
> * CONFIG_PCI is already required by BNXT
> * CONFIG_NETDEVICES and CONFIG_ETHERNET are needed to chose BNXT
>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/bnxt_re/Kconfig | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/Kconfig b/drivers/infiniband/hw/bnxt_re/Kconfig
> index 0feac5132ce1..6a17f5cdb020 100644
> --- a/drivers/infiniband/hw/bnxt_re/Kconfig
> +++ b/drivers/infiniband/hw/bnxt_re/Kconfig
> @@ -2,9 +2,7 @@
>  config INFINIBAND_BNXT_RE
>         tristate "Broadcom Netxtreme HCA support"
>         depends on 64BIT
> -       depends on ETHERNET && NETDEVICES && PCI && INET && DCB
> -       select NET_VENDOR_BROADCOM
> -       select BNXT
> +       depends on INET && DCB && BNXT
>         help
>           This driver supports Broadcom NetXtreme-E 10/25/40/50 gigabit
>           RoCE HCAs.  To compile this driver as a module, choose M here:
> --
> 2.30.2
>
Acked-By: Devesh Sharma <devesh.sharma@broadcom.com>

-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[PATCH rdma-next v2 2/5] RDMA/bnxt_re: Create direct symbolic link between bnxt modules

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Convert indirect probe call to its direct equivalent to create
symbols link between RDMA and netdev modules. This will give
us an ability to remove custom module reference counting that
doesn't belong to the driver.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/main.c          | 7 +------
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 2 --
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     | 1 -
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 1 +
 4 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index b30d37f0bad2..140c54ee5916 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -610,15 +610,10 @@ static void bnxt_re_dev_unprobe(struct net_device *netdev,
 
 static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
 {
-	struct bnxt *bp = netdev_priv(netdev);
 	struct bnxt_en_dev *en_dev;
 	struct pci_dev *pdev;
 
-	/* Call bnxt_en's RoCE probe via indirect API */
-	if (!bp->ulp_probe)
-		return ERR_PTR(-EINVAL);
-
-	en_dev = bp->ulp_probe(netdev);
+	en_dev = bnxt_ulp_probe(netdev);
 	if (IS_ERR(en_dev))
 		return en_dev;
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index a680fd9c68ea..3f0e4bde5dc9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -12859,8 +12859,6 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!BNXT_CHIP_P4_PLUS(bp))
 		bp->flags |= BNXT_FLAG_DOUBLE_DB;
 
-	bp->ulp_probe = bnxt_ulp_probe;
-
 	rc = bnxt_init_mac_addr(bp);
 	if (rc) {
 		dev_err(&pdev->dev, "Unable to initialize mac address.\n");
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 1259e68cba2a..eb0314d7a9b1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1745,7 +1745,6 @@ struct bnxt {
 	(BNXT_CHIP_P4(bp) || BNXT_CHIP_P5(bp))
 
 	struct bnxt_en_dev	*edev;
-	struct bnxt_en_dev *	(*ulp_probe)(struct net_device *);
 
 	struct bnxt_napi	**bnapi;
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
index 64dbbb04b043..a918e374f3c5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
@@ -491,3 +491,4 @@ struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
 	}
 	return bp->edev;
 }
+EXPORT_SYMBOL(bnxt_ulp_probe);
-- 
2.30.2

Re: [PATCH rdma-next v2 2/5] RDMA/bnxt_re: Create direct symbolic link between bnxt modules

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> Convert indirect probe call to its direct equivalent to create
> symbols link between RDMA and netdev modules. This will give
> us an ability to remove custom module reference counting that
> doesn't belong to the driver.
>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/bnxt_re/main.c          | 7 +------
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 2 --
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     | 1 -
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 1 +
>  4 files changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index b30d37f0bad2..140c54ee5916 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -610,15 +610,10 @@ static void bnxt_re_dev_unprobe(struct net_device *netdev,
>
>  static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
>  {
> -       struct bnxt *bp = netdev_priv(netdev);
>         struct bnxt_en_dev *en_dev;
>         struct pci_dev *pdev;
>
> -       /* Call bnxt_en's RoCE probe via indirect API */
> -       if (!bp->ulp_probe)
> -               return ERR_PTR(-EINVAL);
> -
> -       en_dev = bp->ulp_probe(netdev);
> +       en_dev = bnxt_ulp_probe(netdev);
>         if (IS_ERR(en_dev))
>                 return en_dev;
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index a680fd9c68ea..3f0e4bde5dc9 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -12859,8 +12859,6 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         if (!BNXT_CHIP_P4_PLUS(bp))
>                 bp->flags |= BNXT_FLAG_DOUBLE_DB;
>
> -       bp->ulp_probe = bnxt_ulp_probe;
> -
>         rc = bnxt_init_mac_addr(bp);
>         if (rc) {
>                 dev_err(&pdev->dev, "Unable to initialize mac address.\n");
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 1259e68cba2a..eb0314d7a9b1 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -1745,7 +1745,6 @@ struct bnxt {
>         (BNXT_CHIP_P4(bp) || BNXT_CHIP_P5(bp))
>
>         struct bnxt_en_dev      *edev;
> -       struct bnxt_en_dev *    (*ulp_probe)(struct net_device *);
>
>         struct bnxt_napi        **bnapi;
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index 64dbbb04b043..a918e374f3c5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> @@ -491,3 +491,4 @@ struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
>         }
>         return bp->edev;
>  }
> +EXPORT_SYMBOL(bnxt_ulp_probe);

Acked-By: Devesh Sharma <devesh.sharma@broadcom.com>
> --
> 2.30.2
>


-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[PATCH rdma-next v2 3/5] RDMA/bnxt_re: Get rid of custom module reference counting

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Instead of manually messing with parent driver module reference
counting rely on export symbol mechanism to ensure that proper
probe/remove chain is performed.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 140c54ee5916..8bfbf0231a9e 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -601,13 +601,6 @@ static struct bnxt_re_dev *bnxt_re_from_netdev(struct net_device *netdev)
 	return container_of(ibdev, struct bnxt_re_dev, ibdev);
 }
 
-static void bnxt_re_dev_unprobe(struct net_device *netdev,
-				struct bnxt_en_dev *en_dev)
-{
-	dev_put(netdev);
-	module_put(en_dev->pdev->driver->driver.owner);
-}
-
 static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
 {
 	struct bnxt_en_dev *en_dev;
@@ -628,10 +621,6 @@ static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
 		return ERR_PTR(-ENODEV);
 	}
 
-	/* Bump net device reference count */
-	if (!try_module_get(pdev->driver->driver.owner))
-		return ERR_PTR(-ENODEV);
-
 	dev_hold(netdev);
 
 	return en_dev;
@@ -1558,13 +1547,12 @@ static int bnxt_re_dev_init(struct bnxt_re_dev *rdev, u8 wqe_mode)
 
 static void bnxt_re_dev_unreg(struct bnxt_re_dev *rdev)
 {
-	struct bnxt_en_dev *en_dev = rdev->en_dev;
 	struct net_device *netdev = rdev->netdev;
 
 	bnxt_re_dev_remove(rdev);
 
 	if (netdev)
-		bnxt_re_dev_unprobe(netdev, en_dev);
+		dev_put(netdev);
 }
 
 static int bnxt_re_dev_reg(struct bnxt_re_dev **rdev, struct net_device *netdev)
@@ -1586,7 +1574,7 @@ static int bnxt_re_dev_reg(struct bnxt_re_dev **rdev, struct net_device *netdev)
 	*rdev = bnxt_re_dev_add(netdev, en_dev);
 	if (!*rdev) {
 		rc = -ENOMEM;
-		bnxt_re_dev_unprobe(netdev, en_dev);
+		dev_put(netdev);
 		goto exit;
 	}
 exit:
-- 
2.30.2

Re: [PATCH rdma-next v2 3/5] RDMA/bnxt_re: Get rid of custom module reference counting

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> Instead of manually messing with parent driver module reference
> counting rely on export symbol mechanism to ensure that proper
> probe/remove chain is performed.
>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/bnxt_re/main.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index 140c54ee5916..8bfbf0231a9e 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -601,13 +601,6 @@ static struct bnxt_re_dev *bnxt_re_from_netdev(struct net_device *netdev)
>         return container_of(ibdev, struct bnxt_re_dev, ibdev);
>  }
>
> -static void bnxt_re_dev_unprobe(struct net_device *netdev,
> -                               struct bnxt_en_dev *en_dev)
> -{
> -       dev_put(netdev);
> -       module_put(en_dev->pdev->driver->driver.owner);
> -}
> -
>  static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
>  {
>         struct bnxt_en_dev *en_dev;
> @@ -628,10 +621,6 @@ static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
>                 return ERR_PTR(-ENODEV);
>         }
>
> -       /* Bump net device reference count */
> -       if (!try_module_get(pdev->driver->driver.owner))
> -               return ERR_PTR(-ENODEV);
> -
>         dev_hold(netdev);
>
>         return en_dev;
> @@ -1558,13 +1547,12 @@ static int bnxt_re_dev_init(struct bnxt_re_dev *rdev, u8 wqe_mode)
>
>  static void bnxt_re_dev_unreg(struct bnxt_re_dev *rdev)
>  {
> -       struct bnxt_en_dev *en_dev = rdev->en_dev;
>         struct net_device *netdev = rdev->netdev;
>
>         bnxt_re_dev_remove(rdev);
>
>         if (netdev)
> -               bnxt_re_dev_unprobe(netdev, en_dev);
> +               dev_put(netdev);
>  }
>
>  static int bnxt_re_dev_reg(struct bnxt_re_dev **rdev, struct net_device *netdev)
> @@ -1586,7 +1574,7 @@ static int bnxt_re_dev_reg(struct bnxt_re_dev **rdev, struct net_device *netdev)
>         *rdev = bnxt_re_dev_add(netdev, en_dev);
>         if (!*rdev) {
>                 rc = -ENOMEM;
> -               bnxt_re_dev_unprobe(netdev, en_dev);
> +               dev_put(netdev);
>                 goto exit;
>         }
>  exit:

Acked-By: Devesh Sharma <devesh.sharma@broadcom.com>
> --
> 2.30.2
>


-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[PATCH rdma-next v2 4/5] net/bnxt: Remove useless check of non-existent ULP id

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There is no other bnxt ULP driver in the upstream and all checks
for the ULP id are useless, so remove them and convert double array
table to proper pointer structure.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 189 +++++++-----------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |   6 +-
 2 files changed, 73 insertions(+), 122 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
index a918e374f3c5..f7af900afaed 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
@@ -29,34 +29,26 @@ static int bnxt_register_dev(struct bnxt_en_dev *edev, int ulp_id,
 {
 	struct net_device *dev = edev->net;
 	struct bnxt *bp = netdev_priv(dev);
+	unsigned int max_stat_ctxs;
 	struct bnxt_ulp *ulp;
 
 	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;
-	}
-	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;
 
-		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;
-	}
+	ulp = kzalloc(sizeof(*ulp), GFP_KERNEL);
+	if (!ulp)
+		return -ENOMEM;
 
-	atomic_set(&ulp->ref_count, 0);
+	edev->ulp_tbl = ulp;
 	ulp->handle = handle;
 	rcu_assign_pointer(ulp->ulp_ops, ulp_ops);
 
-	if (ulp_id == BNXT_ROCE_ULP) {
-		if (test_bit(BNXT_STATE_OPEN, &bp->state))
-			bnxt_hwrm_vnic_cfg(bp, 0);
-	}
+	if (test_bit(BNXT_STATE_OPEN, &bp->state))
+		bnxt_hwrm_vnic_cfg(bp, 0);
 
 	return 0;
 }
@@ -69,15 +61,9 @@ static int bnxt_unregister_dev(struct bnxt_en_dev *edev, int ulp_id)
 	int i = 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 not registered\n", ulp_id);
-		return -EINVAL;
-	}
-	if (ulp_id == BNXT_ROCE_ULP && ulp->msix_requested)
+	ulp = edev->ulp_tbl;
+	if (ulp->msix_requested)
 		edev->en_ops->bnxt_free_msix(edev, ulp_id);
 
 	if (ulp->max_async_event_id)
@@ -91,6 +77,8 @@ static int bnxt_unregister_dev(struct bnxt_en_dev *edev, int ulp_id)
 		msleep(100);
 		i++;
 	}
+	kfree(ulp);
+	edev->ulp_tbl = NULL;
 	return 0;
 }
 
@@ -99,8 +87,8 @@ static void bnxt_fill_msix_vecs(struct bnxt *bp, struct bnxt_msix_entry *ent)
 	struct bnxt_en_dev *edev = bp->edev;
 	int num_msix, idx, i;
 
-	num_msix = edev->ulp_tbl[BNXT_ROCE_ULP].msix_requested;
-	idx = edev->ulp_tbl[BNXT_ROCE_ULP].msix_base;
+	num_msix = edev->ulp_tbl->msix_requested;
+	idx = edev->ulp_tbl->msix_base;
 	for (i = 0; i < num_msix; i++) {
 		ent[i].vector = bp->irq_tbl[idx + i].vector;
 		ent[i].ring_idx = idx + i;
@@ -126,13 +114,11 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, int ulp_id,
 	int rc = 0;
 
 	ASSERT_RTNL();
-	if (ulp_id != BNXT_ROCE_ULP)
-		return -EINVAL;
 
 	if (!(bp->flags & BNXT_FLAG_USING_MSIX))
 		return -ENODEV;
 
-	if (edev->ulp_tbl[ulp_id].msix_requested)
+	if (edev->ulp_tbl->msix_requested)
 		return -EAGAIN;
 
 	max_cp_rings = bnxt_get_max_func_cp_rings(bp);
@@ -148,8 +134,8 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, 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;
+	edev->ulp_tbl->msix_base = idx;
+	edev->ulp_tbl->msix_requested = avail_msix;
 	hw_resc = &bp->hw_resc;
 	total_vecs = idx + avail_msix;
 	if (bp->total_irqs < total_vecs ||
@@ -162,7 +148,7 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, int ulp_id,
 		}
 	}
 	if (rc) {
-		edev->ulp_tbl[ulp_id].msix_requested = 0;
+		edev->ulp_tbl->msix_requested = 0;
 		return -EAGAIN;
 	}
 
@@ -171,7 +157,7 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, int ulp_id,
 
 		resv_msix = hw_resc->resv_irqs - bp->cp_nr_rings;
 		avail_msix = min_t(int, resv_msix, avail_msix);
-		edev->ulp_tbl[ulp_id].msix_requested = avail_msix;
+		edev->ulp_tbl->msix_requested = avail_msix;
 	}
 	bnxt_fill_msix_vecs(bp, ent);
 	edev->flags |= BNXT_EN_FLAG_MSIX_REQUESTED;
@@ -184,13 +170,11 @@ static int bnxt_free_msix_vecs(struct bnxt_en_dev *edev, int ulp_id)
 	struct bnxt *bp = netdev_priv(dev);
 
 	ASSERT_RTNL();
-	if (ulp_id != BNXT_ROCE_ULP)
-		return -EINVAL;
 
 	if (!(edev->flags & BNXT_EN_FLAG_MSIX_REQUESTED))
 		return 0;
 
-	edev->ulp_tbl[ulp_id].msix_requested = 0;
+	edev->ulp_tbl->msix_requested = 0;
 	edev->flags &= ~BNXT_EN_FLAG_MSIX_REQUESTED;
 	if (netif_running(dev) && !(edev->flags & BNXT_EN_FLAG_ULP_STOPPED)) {
 		bnxt_close_nic(bp, true, false);
@@ -204,7 +188,7 @@ int bnxt_get_ulp_msix_num(struct bnxt *bp)
 	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
 		struct bnxt_en_dev *edev = bp->edev;
 
-		return edev->ulp_tbl[BNXT_ROCE_ULP].msix_requested;
+		return edev->ulp_tbl->msix_requested;
 	}
 	return 0;
 }
@@ -214,8 +198,8 @@ int bnxt_get_ulp_msix_base(struct bnxt *bp)
 	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
 		struct bnxt_en_dev *edev = bp->edev;
 
-		if (edev->ulp_tbl[BNXT_ROCE_ULP].msix_requested)
-			return edev->ulp_tbl[BNXT_ROCE_ULP].msix_base;
+		if (edev->ulp_tbl->msix_requested)
+			return edev->ulp_tbl->msix_base;
 	}
 	return 0;
 }
@@ -225,7 +209,7 @@ int bnxt_get_ulp_stat_ctxs(struct bnxt *bp)
 	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
 		struct bnxt_en_dev *edev = bp->edev;
 
-		if (edev->ulp_tbl[BNXT_ROCE_ULP].msix_requested)
+		if (edev->ulp_tbl->msix_requested)
 			return BNXT_MIN_ROCE_STAT_CTXS;
 	}
 
@@ -240,9 +224,6 @@ static int bnxt_send_msg(struct bnxt_en_dev *edev, int ulp_id,
 	struct input *req;
 	int rc;
 
-	if (ulp_id != BNXT_ROCE_ULP && bp->fw_reset_state)
-		return -EBUSY;
-
 	mutex_lock(&bp->hwrm_cmd_lock);
 	req = fw_msg->msg;
 	req->resp_addr = cpu_to_le64(bp->hwrm_cmd_resp_dma_addr);
@@ -275,27 +256,25 @@ void bnxt_ulp_stop(struct bnxt *bp)
 {
 	struct bnxt_en_dev *edev = bp->edev;
 	struct bnxt_ulp_ops *ops;
-	int i;
+	struct bnxt_ulp *ulp;
 
 	if (!edev)
 		return;
 
 	edev->flags |= BNXT_EN_FLAG_ULP_STOPPED;
-	for (i = 0; i < BNXT_MAX_ULP; i++) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[i];
+	ulp = edev->ulp_tbl;
 
-		ops = rtnl_dereference(ulp->ulp_ops);
-		if (!ops || !ops->ulp_stop)
-			continue;
-		ops->ulp_stop(ulp->handle);
-	}
+	ops = rtnl_dereference(ulp->ulp_ops);
+	if (!ops || !ops->ulp_stop)
+		return;
+	ops->ulp_stop(ulp->handle);
 }
 
 void bnxt_ulp_start(struct bnxt *bp, int err)
 {
 	struct bnxt_en_dev *edev = bp->edev;
 	struct bnxt_ulp_ops *ops;
-	int i;
+	struct bnxt_ulp *ulp;
 
 	if (!edev)
 		return;
@@ -305,58 +284,53 @@ void bnxt_ulp_start(struct bnxt *bp, int err)
 	if (err)
 		return;
 
-	for (i = 0; i < BNXT_MAX_ULP; i++) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[i];
+	ulp = edev->ulp_tbl;
 
-		ops = rtnl_dereference(ulp->ulp_ops);
-		if (!ops || !ops->ulp_start)
-			continue;
-		ops->ulp_start(ulp->handle);
-	}
+	ops = rtnl_dereference(ulp->ulp_ops);
+	if (!ops || !ops->ulp_start)
+		return;
+	ops->ulp_start(ulp->handle);
 }
 
 void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs)
 {
 	struct bnxt_en_dev *edev = bp->edev;
 	struct bnxt_ulp_ops *ops;
-	int i;
+	struct bnxt_ulp *ulp;
 
 	if (!edev)
 		return;
 
-	for (i = 0; i < BNXT_MAX_ULP; i++) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[i];
+	ulp = edev->ulp_tbl;
 
-		rcu_read_lock();
-		ops = rcu_dereference(ulp->ulp_ops);
-		if (!ops || !ops->ulp_sriov_config) {
-			rcu_read_unlock();
-			continue;
-		}
-		bnxt_ulp_get(ulp);
+	rcu_read_lock();
+	ops = rcu_dereference(ulp->ulp_ops);
+	if (!ops || !ops->ulp_sriov_config) {
 		rcu_read_unlock();
-		ops->ulp_sriov_config(ulp->handle, num_vfs);
-		bnxt_ulp_put(ulp);
+		return;
 	}
+	bnxt_ulp_get(ulp);
+	rcu_read_unlock();
+	ops->ulp_sriov_config(ulp->handle, num_vfs);
+	bnxt_ulp_put(ulp);
 }
 
 void bnxt_ulp_shutdown(struct bnxt *bp)
 {
 	struct bnxt_en_dev *edev = bp->edev;
 	struct bnxt_ulp_ops *ops;
-	int i;
+	struct bnxt_ulp *ulp;
 
 	if (!edev)
 		return;
 
-	for (i = 0; i < BNXT_MAX_ULP; i++) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[i];
+	ulp = edev->ulp_tbl;
 
-		ops = rtnl_dereference(ulp->ulp_ops);
-		if (!ops || !ops->ulp_shutdown)
-			continue;
-		ops->ulp_shutdown(ulp->handle);
-	}
+	ops = rtnl_dereference(ulp->ulp_ops);
+	if (!ops || !ops->ulp_shutdown)
+		return;
+
+	ops->ulp_shutdown(ulp->handle);
 }
 
 void bnxt_ulp_irq_stop(struct bnxt *bp)
@@ -368,7 +342,7 @@ void bnxt_ulp_irq_stop(struct bnxt *bp)
 		return;
 
 	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[BNXT_ROCE_ULP];
+		struct bnxt_ulp *ulp = edev->ulp_tbl;
 
 		if (!ulp->msix_requested)
 			return;
@@ -389,7 +363,7 @@ void bnxt_ulp_irq_restart(struct bnxt *bp, int err)
 		return;
 
 	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[BNXT_ROCE_ULP];
+		struct bnxt_ulp *ulp = edev->ulp_tbl;
 		struct bnxt_msix_entry *ent = NULL;
 
 		if (!ulp->msix_requested)
@@ -416,47 +390,27 @@ void bnxt_ulp_async_events(struct bnxt *bp, struct hwrm_async_event_cmpl *cmpl)
 	u16 event_id = le16_to_cpu(cmpl->event_id);
 	struct bnxt_en_dev *edev = bp->edev;
 	struct bnxt_ulp_ops *ops;
-	int i;
+	struct bnxt_ulp *ulp;
 
 	if (!edev)
 		return;
 
 	rcu_read_lock();
-	for (i = 0; i < BNXT_MAX_ULP; i++) {
-		struct bnxt_ulp *ulp = &edev->ulp_tbl[i];
-
-		ops = rcu_dereference(ulp->ulp_ops);
-		if (!ops || !ops->ulp_async_notifier)
-			continue;
-		if (!ulp->async_events_bmap ||
-		    event_id > ulp->max_async_event_id)
-			continue;
-
-		/* Read max_async_event_id first before testing the bitmap. */
-		smp_rmb();
-		if (test_bit(event_id, ulp->async_events_bmap))
-			ops->ulp_async_notifier(ulp->handle, cmpl);
-	}
-	rcu_read_unlock();
-}
+	ulp = edev->ulp_tbl;
 
-static int bnxt_register_async_events(struct bnxt_en_dev *edev, int ulp_id,
-				      unsigned long *events_bmap, u16 max_id)
-{
-	struct net_device *dev = edev->net;
-	struct bnxt *bp = netdev_priv(dev);
-	struct bnxt_ulp *ulp;
+	ops = rcu_dereference(ulp->ulp_ops);
+	if (!ops || !ops->ulp_async_notifier)
+		goto out;
+	if (!ulp->async_events_bmap || event_id > ulp->max_async_event_id)
+		goto out;
 
-	if (ulp_id >= BNXT_MAX_ULP)
-		return -EINVAL;
+	/* Read max_async_event_id first before testing the bitmap. */
+	smp_rmb();
+	if (test_bit(event_id, ulp->async_events_bmap))
+		ops->ulp_async_notifier(ulp->handle, cmpl);
 
-	ulp = &edev->ulp_tbl[ulp_id];
-	ulp->async_events_bmap = events_bmap;
-	/* Make sure bnxt_ulp_async_events() sees this order */
-	smp_wmb();
-	ulp->max_async_event_id = max_id;
-	bnxt_hwrm_func_drv_rgtr(bp, events_bmap, max_id + 1, true);
-	return 0;
+out:
+	rcu_read_unlock();
 }
 
 static const struct bnxt_en_ops bnxt_en_ops_tbl = {
@@ -465,7 +419,6 @@ static const struct bnxt_en_ops bnxt_en_ops_tbl = {
 	.bnxt_request_msix	= bnxt_req_msix_vecs,
 	.bnxt_free_msix		= bnxt_free_msix_vecs,
 	.bnxt_send_fw_msg	= bnxt_send_msg,
-	.bnxt_register_fw_async_events	= bnxt_register_async_events,
 };
 
 struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
index 6b4d2556a6df..3caee7c2f8c9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
@@ -66,7 +66,7 @@ struct bnxt_en_dev {
 	#define BNXT_EN_FLAG_MSIX_REQUESTED	0x4
 	#define BNXT_EN_FLAG_ULP_STOPPED	0x8
 	const struct bnxt_en_ops	*en_ops;
-	struct bnxt_ulp			ulp_tbl[BNXT_MAX_ULP];
+	struct bnxt_ulp			*ulp_tbl;
 	int				l2_db_size;	/* Doorbell BAR size in
 							 * bytes mapped by L2
 							 * driver.
@@ -86,13 +86,11 @@ struct bnxt_en_ops {
 	int (*bnxt_free_msix)(struct bnxt_en_dev *, int);
 	int (*bnxt_send_fw_msg)(struct bnxt_en_dev *, int,
 				struct bnxt_fw_msg *);
-	int (*bnxt_register_fw_async_events)(struct bnxt_en_dev *, int,
-					     unsigned long *, u16);
 };
 
 static inline bool bnxt_ulp_registered(struct bnxt_en_dev *edev, int ulp_id)
 {
-	if (edev && rcu_access_pointer(edev->ulp_tbl[ulp_id].ulp_ops))
+	if (edev && edev->ulp_tbl)
 		return true;
 	return false;
 }
-- 
2.30.2

[PATCH rdma-next v2 5/5] net/bnxt: Use direct API instead of useless indirection

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There is no need in any indirection complexity for one ULP user,
remove all this complexity in favour of direct calls to the exported
symbols. This allows us to greatly simplify the code.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/main.c          | 70 +++-------------
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 83 +++++++------------
 drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 26 ++----
 4 files changed, 55 insertions(+), 126 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index 8bfbf0231a9e..b3f8fe8314a8 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -351,24 +351,6 @@ static struct bnxt_ulp_ops bnxt_re_ulp_ops = {
 
 /* RoCE -> Net driver */
 
-/* Driver registration routines used to let the networking driver (bnxt_en)
- * to know that the RoCE driver is now installed
- */
-static int bnxt_re_unregister_netdev(struct bnxt_re_dev *rdev)
-{
-	struct bnxt_en_dev *en_dev;
-	int rc;
-
-	if (!rdev)
-		return -EINVAL;
-
-	en_dev = rdev->en_dev;
-
-	rc = en_dev->en_ops->bnxt_unregister_device(rdev->en_dev,
-						    BNXT_ROCE_ULP);
-	return rc;
-}
-
 static int bnxt_re_register_netdev(struct bnxt_re_dev *rdev)
 {
 	struct bnxt_en_dev *en_dev;
@@ -379,28 +361,11 @@ static int bnxt_re_register_netdev(struct bnxt_re_dev *rdev)
 
 	en_dev = rdev->en_dev;
 
-	rc = en_dev->en_ops->bnxt_register_device(en_dev, BNXT_ROCE_ULP,
-						  &bnxt_re_ulp_ops, rdev);
+	rc = bnxt_register_dev(en_dev, &bnxt_re_ulp_ops, rdev);
 	rdev->qplib_res.pdev = rdev->en_dev->pdev;
 	return rc;
 }
 
-static int bnxt_re_free_msix(struct bnxt_re_dev *rdev)
-{
-	struct bnxt_en_dev *en_dev;
-	int rc;
-
-	if (!rdev)
-		return -EINVAL;
-
-	en_dev = rdev->en_dev;
-
-
-	rc = en_dev->en_ops->bnxt_free_msix(rdev->en_dev, BNXT_ROCE_ULP);
-
-	return rc;
-}
-
 static int bnxt_re_request_msix(struct bnxt_re_dev *rdev)
 {
 	int rc = 0, num_msix_want = BNXT_RE_MAX_MSIX, num_msix_got;
@@ -413,9 +378,8 @@ static int bnxt_re_request_msix(struct bnxt_re_dev *rdev)
 
 	num_msix_want = min_t(u32, BNXT_RE_MAX_MSIX, num_online_cpus());
 
-	num_msix_got = en_dev->en_ops->bnxt_request_msix(en_dev, BNXT_ROCE_ULP,
-							 rdev->msix_entries,
-							 num_msix_want);
+	num_msix_got =
+		bnxt_req_msix_vecs(en_dev, rdev->msix_entries, num_msix_want);
 	if (num_msix_got < BNXT_RE_MIN_MSIX) {
 		rc = -EINVAL;
 		goto done;
@@ -471,7 +435,7 @@ static int bnxt_re_net_ring_free(struct bnxt_re_dev *rdev,
 	req.ring_id = cpu_to_le16(fw_ring_id);
 	bnxt_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&resp,
 			    sizeof(resp), DFLT_HWRM_CMD_TIMEOUT);
-	rc = en_dev->en_ops->bnxt_send_fw_msg(en_dev, BNXT_ROCE_ULP, &fw_msg);
+	rc = bnxt_send_msg(en_dev, &fw_msg);
 	if (rc)
 		ibdev_err(&rdev->ibdev, "Failed to free HW ring:%d :%#x",
 			  req.ring_id, rc);
@@ -508,7 +472,7 @@ static int bnxt_re_net_ring_alloc(struct bnxt_re_dev *rdev,
 	req.int_mode = ring_attr->mode;
 	bnxt_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&resp,
 			    sizeof(resp), DFLT_HWRM_CMD_TIMEOUT);
-	rc = en_dev->en_ops->bnxt_send_fw_msg(en_dev, BNXT_ROCE_ULP, &fw_msg);
+	rc = bnxt_send_msg(en_dev, &fw_msg);
 	if (!rc)
 		*fw_ring_id = le16_to_cpu(resp.ring_id);
 
@@ -535,7 +499,7 @@ static int bnxt_re_net_stats_ctx_free(struct bnxt_re_dev *rdev,
 	req.stat_ctx_id = cpu_to_le32(fw_stats_ctx_id);
 	bnxt_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&req,
 			    sizeof(req), DFLT_HWRM_CMD_TIMEOUT);
-	rc = en_dev->en_ops->bnxt_send_fw_msg(en_dev, BNXT_ROCE_ULP, &fw_msg);
+	rc = bnxt_send_msg(en_dev, &fw_msg);
 	if (rc)
 		ibdev_err(&rdev->ibdev, "Failed to free HW stats context %#x",
 			  rc);
@@ -567,7 +531,7 @@ static int bnxt_re_net_stats_ctx_alloc(struct bnxt_re_dev *rdev,
 	req.stat_ctx_flags = STAT_CTX_ALLOC_REQ_STAT_CTX_FLAGS_ROCE;
 	bnxt_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&resp,
 			    sizeof(resp), DFLT_HWRM_CMD_TIMEOUT);
-	rc = en_dev->en_ops->bnxt_send_fw_msg(en_dev, BNXT_ROCE_ULP, &fw_msg);
+	rc = bnxt_send_msg(en_dev, &fw_msg);
 	if (!rc)
 		*fw_stats_ctx_id = le32_to_cpu(resp.stat_ctx_id);
 
@@ -1119,7 +1083,7 @@ static int bnxt_re_query_hwrm_pri2cos(struct bnxt_re_dev *rdev, u8 dir,
 
 	bnxt_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&resp,
 			    sizeof(resp), DFLT_HWRM_CMD_TIMEOUT);
-	rc = en_dev->en_ops->bnxt_send_fw_msg(en_dev, BNXT_ROCE_ULP, &fw_msg);
+	rc = bnxt_send_msg(en_dev, &fw_msg);
 	if (rc)
 		return rc;
 
@@ -1302,7 +1266,7 @@ static void bnxt_re_query_hwrm_intf_version(struct bnxt_re_dev *rdev)
 	req.hwrm_intf_upd = HWRM_VERSION_UPDATE;
 	bnxt_re_fill_fw_msg(&fw_msg, (void *)&req, sizeof(req), (void *)&resp,
 			    sizeof(resp), DFLT_HWRM_CMD_TIMEOUT);
-	rc = en_dev->en_ops->bnxt_send_fw_msg(en_dev, BNXT_ROCE_ULP, &fw_msg);
+	rc = bnxt_send_msg(en_dev, &fw_msg);
 	if (rc) {
 		ibdev_err(&rdev->ibdev, "Failed to query HW version, rc = 0x%x",
 			  rc);
@@ -1365,20 +1329,12 @@ static void bnxt_re_dev_uninit(struct bnxt_re_dev *rdev)
 		bnxt_re_net_ring_free(rdev, rdev->rcfw.creq.ring_id, type);
 		bnxt_qplib_free_rcfw_channel(&rdev->rcfw);
 	}
-	if (test_and_clear_bit(BNXT_RE_FLAG_GOT_MSIX, &rdev->flags)) {
-		rc = bnxt_re_free_msix(rdev);
-		if (rc)
-			ibdev_warn(&rdev->ibdev,
-				   "Failed to free MSI-X vectors: %#x", rc);
-	}
+	if (test_and_clear_bit(BNXT_RE_FLAG_GOT_MSIX, &rdev->flags))
+		bnxt_free_msix_vecs(rdev->en_dev);
 
 	bnxt_re_destroy_chip_ctx(rdev);
-	if (test_and_clear_bit(BNXT_RE_FLAG_NETDEV_REGISTERED, &rdev->flags)) {
-		rc = bnxt_re_unregister_netdev(rdev);
-		if (rc)
-			ibdev_warn(&rdev->ibdev,
-				   "Failed to unregister with netdev: %#x", rc);
-	}
+	if (test_and_clear_bit(BNXT_RE_FLAG_NETDEV_REGISTERED, &rdev->flags))
+		bnxt_unregister_dev(rdev->en_dev);
 }
 
 /* worker thread for polling periodic events. Now used for QoS programming*/
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 3f0e4bde5dc9..aafee562e782 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5346,7 +5346,7 @@ int bnxt_hwrm_vnic_cfg(struct bnxt *bp, u16 vnic_id)
 #endif
 	if ((bp->flags & BNXT_FLAG_STRIP_VLAN) || def_vlan)
 		req.flags |= cpu_to_le32(VNIC_CFG_REQ_FLAGS_VLAN_STRIP_MODE);
-	if (!vnic_id && bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP))
+	if (!vnic_id && bnxt_ulp_registered(bp->edev))
 		req.flags |= cpu_to_le32(bnxt_get_roce_vnic_mode(bp));
 
 	return hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
index f7af900afaed..6b4c6b0445b8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
@@ -24,8 +24,8 @@
 #include "bnxt.h"
 #include "bnxt_ulp.h"
 
-static int bnxt_register_dev(struct bnxt_en_dev *edev, int ulp_id,
-			     struct bnxt_ulp_ops *ulp_ops, void *handle)
+int bnxt_register_dev(struct bnxt_en_dev *edev, struct bnxt_ulp_ops *ulp_ops,
+		      void *handle)
 {
 	struct net_device *dev = edev->net;
 	struct bnxt *bp = netdev_priv(dev);
@@ -45,15 +45,16 @@ static int bnxt_register_dev(struct bnxt_en_dev *edev, int ulp_id,
 
 	edev->ulp_tbl = ulp;
 	ulp->handle = handle;
-	rcu_assign_pointer(ulp->ulp_ops, ulp_ops);
+	ulp->ulp_ops = ulp_ops;
 
 	if (test_bit(BNXT_STATE_OPEN, &bp->state))
 		bnxt_hwrm_vnic_cfg(bp, 0);
 
 	return 0;
 }
+EXPORT_SYMBOL(bnxt_register_dev);
 
-static int bnxt_unregister_dev(struct bnxt_en_dev *edev, int ulp_id)
+void bnxt_unregister_dev(struct bnxt_en_dev *edev)
 {
 	struct net_device *dev = edev->net;
 	struct bnxt *bp = netdev_priv(dev);
@@ -64,13 +65,11 @@ static int bnxt_unregister_dev(struct bnxt_en_dev *edev, int ulp_id)
 
 	ulp = edev->ulp_tbl;
 	if (ulp->msix_requested)
-		edev->en_ops->bnxt_free_msix(edev, ulp_id);
+		bnxt_free_msix_vecs(edev);
 
 	if (ulp->max_async_event_id)
 		bnxt_hwrm_func_drv_rgtr(bp, NULL, 0, true);
 
-	RCU_INIT_POINTER(ulp->ulp_ops, NULL);
-	synchronize_rcu();
 	ulp->max_async_event_id = 0;
 	ulp->async_events_bmap = NULL;
 	while (atomic_read(&ulp->ref_count) != 0 && i < 10) {
@@ -79,8 +78,8 @@ static int bnxt_unregister_dev(struct bnxt_en_dev *edev, int ulp_id)
 	}
 	kfree(ulp);
 	edev->ulp_tbl = NULL;
-	return 0;
 }
+EXPORT_SYMBOL(bnxt_unregister_dev);
 
 static void bnxt_fill_msix_vecs(struct bnxt *bp, struct bnxt_msix_entry *ent)
 {
@@ -102,8 +101,8 @@ static void bnxt_fill_msix_vecs(struct bnxt *bp, struct bnxt_msix_entry *ent)
 	}
 }
 
-static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, int ulp_id,
-			      struct bnxt_msix_entry *ent, int num_msix)
+int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, struct bnxt_msix_entry *ent,
+		       int num_msix)
 {
 	struct net_device *dev = edev->net;
 	struct bnxt *bp = netdev_priv(dev);
@@ -163,8 +162,9 @@ static int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, int ulp_id,
 	edev->flags |= BNXT_EN_FLAG_MSIX_REQUESTED;
 	return avail_msix;
 }
+EXPORT_SYMBOL(bnxt_req_msix_vecs);
 
-static int bnxt_free_msix_vecs(struct bnxt_en_dev *edev, int ulp_id)
+void bnxt_free_msix_vecs(struct bnxt_en_dev *edev)
 {
 	struct net_device *dev = edev->net;
 	struct bnxt *bp = netdev_priv(dev);
@@ -172,7 +172,7 @@ static int bnxt_free_msix_vecs(struct bnxt_en_dev *edev, int ulp_id)
 	ASSERT_RTNL();
 
 	if (!(edev->flags & BNXT_EN_FLAG_MSIX_REQUESTED))
-		return 0;
+		return;
 
 	edev->ulp_tbl->msix_requested = 0;
 	edev->flags &= ~BNXT_EN_FLAG_MSIX_REQUESTED;
@@ -180,12 +180,12 @@ static int bnxt_free_msix_vecs(struct bnxt_en_dev *edev, int ulp_id)
 		bnxt_close_nic(bp, true, false);
 		bnxt_open_nic(bp, true, false);
 	}
-	return 0;
 }
+EXPORT_SYMBOL(bnxt_free_msix_vecs);
 
 int bnxt_get_ulp_msix_num(struct bnxt *bp)
 {
-	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
+	if (bnxt_ulp_registered(bp->edev)) {
 		struct bnxt_en_dev *edev = bp->edev;
 
 		return edev->ulp_tbl->msix_requested;
@@ -195,7 +195,7 @@ int bnxt_get_ulp_msix_num(struct bnxt *bp)
 
 int bnxt_get_ulp_msix_base(struct bnxt *bp)
 {
-	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
+	if (bnxt_ulp_registered(bp->edev)) {
 		struct bnxt_en_dev *edev = bp->edev;
 
 		if (edev->ulp_tbl->msix_requested)
@@ -206,7 +206,7 @@ int bnxt_get_ulp_msix_base(struct bnxt *bp)
 
 int bnxt_get_ulp_stat_ctxs(struct bnxt *bp)
 {
-	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
+	if (bnxt_ulp_registered(bp->edev)) {
 		struct bnxt_en_dev *edev = bp->edev;
 
 		if (edev->ulp_tbl->msix_requested)
@@ -216,8 +216,7 @@ int bnxt_get_ulp_stat_ctxs(struct bnxt *bp)
 	return 0;
 }
 
-static int bnxt_send_msg(struct bnxt_en_dev *edev, int ulp_id,
-			 struct bnxt_fw_msg *fw_msg)
+int bnxt_send_msg(struct bnxt_en_dev *edev, struct bnxt_fw_msg *fw_msg)
 {
 	struct net_device *dev = edev->net;
 	struct bnxt *bp = netdev_priv(dev);
@@ -241,6 +240,7 @@ static int bnxt_send_msg(struct bnxt_en_dev *edev, int ulp_id,
 	mutex_unlock(&bp->hwrm_cmd_lock);
 	return rc;
 }
+EXPORT_SYMBOL(bnxt_send_msg);
 
 static void bnxt_ulp_get(struct bnxt_ulp *ulp)
 {
@@ -263,8 +263,7 @@ void bnxt_ulp_stop(struct bnxt *bp)
 
 	edev->flags |= BNXT_EN_FLAG_ULP_STOPPED;
 	ulp = edev->ulp_tbl;
-
-	ops = rtnl_dereference(ulp->ulp_ops);
+	ops = ulp->ulp_ops;
 	if (!ops || !ops->ulp_stop)
 		return;
 	ops->ulp_stop(ulp->handle);
@@ -285,8 +284,7 @@ void bnxt_ulp_start(struct bnxt *bp, int err)
 		return;
 
 	ulp = edev->ulp_tbl;
-
-	ops = rtnl_dereference(ulp->ulp_ops);
+	ops = ulp->ulp_ops;
 	if (!ops || !ops->ulp_start)
 		return;
 	ops->ulp_start(ulp->handle);
@@ -303,14 +301,11 @@ void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs)
 
 	ulp = edev->ulp_tbl;
 
-	rcu_read_lock();
-	ops = rcu_dereference(ulp->ulp_ops);
-	if (!ops || !ops->ulp_sriov_config) {
-		rcu_read_unlock();
+	ops = ulp->ulp_ops;
+	if (!ops || !ops->ulp_sriov_config)
 		return;
-	}
+
 	bnxt_ulp_get(ulp);
-	rcu_read_unlock();
 	ops->ulp_sriov_config(ulp->handle, num_vfs);
 	bnxt_ulp_put(ulp);
 }
@@ -325,8 +320,7 @@ void bnxt_ulp_shutdown(struct bnxt *bp)
 		return;
 
 	ulp = edev->ulp_tbl;
-
-	ops = rtnl_dereference(ulp->ulp_ops);
+	ops = ulp->ulp_ops;
 	if (!ops || !ops->ulp_shutdown)
 		return;
 
@@ -341,13 +335,13 @@ void bnxt_ulp_irq_stop(struct bnxt *bp)
 	if (!edev || !(edev->flags & BNXT_EN_FLAG_MSIX_REQUESTED))
 		return;
 
-	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
+	if (bnxt_ulp_registered(bp->edev)) {
 		struct bnxt_ulp *ulp = edev->ulp_tbl;
 
 		if (!ulp->msix_requested)
 			return;
 
-		ops = rtnl_dereference(ulp->ulp_ops);
+		ops = ulp->ulp_ops;
 		if (!ops || !ops->ulp_irq_stop)
 			return;
 		ops->ulp_irq_stop(ulp->handle);
@@ -362,14 +356,14 @@ void bnxt_ulp_irq_restart(struct bnxt *bp, int err)
 	if (!edev || !(edev->flags & BNXT_EN_FLAG_MSIX_REQUESTED))
 		return;
 
-	if (bnxt_ulp_registered(bp->edev, BNXT_ROCE_ULP)) {
+	if (bnxt_ulp_registered(bp->edev)) {
 		struct bnxt_ulp *ulp = edev->ulp_tbl;
 		struct bnxt_msix_entry *ent = NULL;
 
 		if (!ulp->msix_requested)
 			return;
 
-		ops = rtnl_dereference(ulp->ulp_ops);
+		ops = ulp->ulp_ops;
 		if (!ops || !ops->ulp_irq_restart)
 			return;
 
@@ -395,32 +389,20 @@ void bnxt_ulp_async_events(struct bnxt *bp, struct hwrm_async_event_cmpl *cmpl)
 	if (!edev)
 		return;
 
-	rcu_read_lock();
 	ulp = edev->ulp_tbl;
-
-	ops = rcu_dereference(ulp->ulp_ops);
+	ops = ulp->ulp_ops;
 	if (!ops || !ops->ulp_async_notifier)
-		goto out;
+		return;
+
 	if (!ulp->async_events_bmap || event_id > ulp->max_async_event_id)
-		goto out;
+		return;
 
 	/* Read max_async_event_id first before testing the bitmap. */
 	smp_rmb();
 	if (test_bit(event_id, ulp->async_events_bmap))
 		ops->ulp_async_notifier(ulp->handle, cmpl);
-
-out:
-	rcu_read_unlock();
 }
 
-static const struct bnxt_en_ops bnxt_en_ops_tbl = {
-	.bnxt_register_device	= bnxt_register_dev,
-	.bnxt_unregister_device	= bnxt_unregister_dev,
-	.bnxt_request_msix	= bnxt_req_msix_vecs,
-	.bnxt_free_msix		= bnxt_free_msix_vecs,
-	.bnxt_send_fw_msg	= bnxt_send_msg,
-};
-
 struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
 {
 	struct bnxt *bp = netdev_priv(dev);
@@ -431,7 +413,6 @@ struct bnxt_en_dev *bnxt_ulp_probe(struct net_device *dev)
 		edev = kzalloc(sizeof(*edev), GFP_KERNEL);
 		if (!edev)
 			return ERR_PTR(-ENOMEM);
-		edev->en_ops = &bnxt_en_ops_tbl;
 		if (bp->flags & BNXT_FLAG_ROCEV1_CAP)
 			edev->flags |= BNXT_EN_FLAG_ROCEV1_CAP;
 		if (bp->flags & BNXT_FLAG_ROCEV2_CAP)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
index 3caee7c2f8c9..367f70ab0d3a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
@@ -10,10 +10,6 @@
 #ifndef BNXT_ULP_H
 #define BNXT_ULP_H
 
-#define BNXT_ROCE_ULP	0
-#define BNXT_OTHER_ULP	1
-#define BNXT_MAX_ULP	2
-
 #define BNXT_MIN_ROCE_CP_RINGS	2
 #define BNXT_MIN_ROCE_STAT_CTXS	1
 
@@ -47,7 +43,7 @@ struct bnxt_fw_msg {
 
 struct bnxt_ulp {
 	void		*handle;
-	struct bnxt_ulp_ops __rcu *ulp_ops;
+	struct bnxt_ulp_ops *ulp_ops;
 	unsigned long	*async_events_bmap;
 	u16		max_async_event_id;
 	u16		msix_requested;
@@ -65,7 +61,6 @@ struct bnxt_en_dev {
 						 BNXT_EN_FLAG_ROCEV2_CAP)
 	#define BNXT_EN_FLAG_MSIX_REQUESTED	0x4
 	#define BNXT_EN_FLAG_ULP_STOPPED	0x8
-	const struct bnxt_en_ops	*en_ops;
 	struct bnxt_ulp			*ulp_tbl;
 	int				l2_db_size;	/* Doorbell BAR size in
 							 * bytes mapped by L2
@@ -77,18 +72,15 @@ struct bnxt_en_dev {
 							 */
 };
 
-struct bnxt_en_ops {
-	int (*bnxt_register_device)(struct bnxt_en_dev *, int,
-				    struct bnxt_ulp_ops *, void *);
-	int (*bnxt_unregister_device)(struct bnxt_en_dev *, int);
-	int (*bnxt_request_msix)(struct bnxt_en_dev *, int,
-				 struct bnxt_msix_entry *, int);
-	int (*bnxt_free_msix)(struct bnxt_en_dev *, int);
-	int (*bnxt_send_fw_msg)(struct bnxt_en_dev *, int,
-				struct bnxt_fw_msg *);
-};
+int bnxt_register_dev(struct bnxt_en_dev *edev, struct bnxt_ulp_ops *ulp_ops,
+		      void *handle);
+void bnxt_unregister_dev(struct bnxt_en_dev *edev);
+int bnxt_req_msix_vecs(struct bnxt_en_dev *edev, struct bnxt_msix_entry *ent,
+		       int num_msix);
+void bnxt_free_msix_vecs(struct bnxt_en_dev *edev);
+int bnxt_send_msg(struct bnxt_en_dev *edev, struct bnxt_fw_msg *fw_msg);
 
-static inline bool bnxt_ulp_registered(struct bnxt_en_dev *edev, int ulp_id)
+static inline bool bnxt_ulp_registered(struct bnxt_en_dev *edev)
 {
 	if (edev && edev->ulp_tbl)
 		return true;
-- 
2.30.2

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> Changelog:
> v2:
>  * kbuild spotted that I didn't delete all code in patch #5, so deleted
>    even more ulp_ops derefences.
> v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
>  * Go much deeper and removed useless ULP indirection
> v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> -----------------------------------------------------------------------
>
> The following series fixes issue spotted in [1], where bnxt_re driver
> messed with module reference counting in order to implement symbol
> dependency of bnxt_re and bnxt modules. All of this is done, when in
> upstream we have only one ULP user of that bnxt module. The simple
> declaration of exported symbol would do the trick.
>
> This series removes that custom module_get/_put, which is not supposed
> to be in the driver from the beginning and get rid of nasty indirection
> logic that isn't relevant for the upstream code.
>
> Such small changes allow us to simplify the bnxt code and my hope that
> Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
>
> Thanks
>
> [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
>
> Leon Romanovsky (5):
>   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
>   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
>   RDMA/bnxt_re: Get rid of custom module reference counting
>   net/bnxt: Remove useless check of non-existent ULP id
>   net/bnxt: Use direct API instead of useless indirection
>
>  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
>  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
>  6 files changed, 119 insertions(+), 260 deletions(-)

Hi Leon,

After a couple of internal discussions we reached a conclusion to
implement the Auxbus driver interface and fix the problem once and for
all.
>
> --
> 2.30.2
>


-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > Changelog:
> > v2:
> >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> >    even more ulp_ops derefences.
> > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> >  * Go much deeper and removed useless ULP indirection
> > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > -----------------------------------------------------------------------
> >
> > The following series fixes issue spotted in [1], where bnxt_re driver
> > messed with module reference counting in order to implement symbol
> > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > upstream we have only one ULP user of that bnxt module. The simple
> > declaration of exported symbol would do the trick.
> >
> > This series removes that custom module_get/_put, which is not supposed
> > to be in the driver from the beginning and get rid of nasty indirection
> > logic that isn't relevant for the upstream code.
> >
> > Such small changes allow us to simplify the bnxt code and my hope that
> > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> >
> > Thanks
> >
> > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> >
> > Leon Romanovsky (5):
> >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> >   RDMA/bnxt_re: Get rid of custom module reference counting
> >   net/bnxt: Remove useless check of non-existent ULP id
> >   net/bnxt: Use direct API instead of useless indirection
> >
> >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> >  6 files changed, 119 insertions(+), 260 deletions(-)
> 
> Hi Leon,
> 
> After a couple of internal discussions we reached a conclusion to
> implement the Auxbus driver interface and fix the problem once and for
> all.

Thanks Devesh,

Jason, it looks like we can proceed with this patchset, because in
auxbus mode this module refcount and ULP indirection logics will be
removed anyway.

Thanks

> >
> > --
> > 2.30.2
> >
> 
> 
> -- 
> -Regards
> Devesh

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 3028 bytes --]

On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > >
> > > Changelog:
> > > v2:
> > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > >    even more ulp_ops derefences.
> > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > >  * Go much deeper and removed useless ULP indirection
> > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > -----------------------------------------------------------------------
> > >
> > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > messed with module reference counting in order to implement symbol
> > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > upstream we have only one ULP user of that bnxt module. The simple
> > > declaration of exported symbol would do the trick.
> > >
> > > This series removes that custom module_get/_put, which is not supposed
> > > to be in the driver from the beginning and get rid of nasty indirection
> > > logic that isn't relevant for the upstream code.
> > >
> > > Such small changes allow us to simplify the bnxt code and my hope that
> > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > >
> > > Thanks
> > >
> > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > >
> > > Leon Romanovsky (5):
> > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > >   net/bnxt: Remove useless check of non-existent ULP id
> > >   net/bnxt: Use direct API instead of useless indirection
> > >
> > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > >  6 files changed, 119 insertions(+), 260 deletions(-)
> >
> > Hi Leon,
> >
> > After a couple of internal discussions we reached a conclusion to
> > implement the Auxbus driver interface and fix the problem once and for
> > all.
>
> Thanks Devesh,
>
> Jason, it looks like we can proceed with this patchset, because in
> auxbus mode this module refcount and ULP indirection logics will be
> removed anyway.
>
> Thanks
Hi Leon,

In my internal testing, I am seeing a crash using the 3rd patch. I am
spending a few cycles on debugging it. expect my input in a day or so.
>
> > >
> > > --
> > > 2.30.2
> > >
> >
> >
> > --
> > -Regards
> > Devesh
>
>


-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > >
> > > > Changelog:
> > > > v2:
> > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > >    even more ulp_ops derefences.
> > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > >  * Go much deeper and removed useless ULP indirection
> > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > -----------------------------------------------------------------------
> > > >
> > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > messed with module reference counting in order to implement symbol
> > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > declaration of exported symbol would do the trick.
> > > >
> > > > This series removes that custom module_get/_put, which is not supposed
> > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > logic that isn't relevant for the upstream code.
> > > >
> > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > >
> > > > Thanks
> > > >
> > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > >
> > > > Leon Romanovsky (5):
> > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > >   net/bnxt: Use direct API instead of useless indirection
> > > >
> > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > >
> > > Hi Leon,
> > >
> > > After a couple of internal discussions we reached a conclusion to
> > > implement the Auxbus driver interface and fix the problem once and for
> > > all.
> >
> > Thanks Devesh,
> >
> > Jason, it looks like we can proceed with this patchset, because in
> > auxbus mode this module refcount and ULP indirection logics will be
> > removed anyway.
> >
> > Thanks
> Hi Leon,
> 
> In my internal testing, I am seeing a crash using the 3rd patch. I am
> spending a few cycles on debugging it. expect my input in a day or so.

Can you please post the kernel crash report here?
I don't see how function rename in patch #3 can cause to the crash.

Thanks

> >
> > > >
> > > > --
> > > > 2.30.2
> > > >
> > >
> > >
> > > --
> > > -Regards
> > > Devesh
> >
> >
> 
> 
> -- 
> -Regards
> Devesh

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Jason Gunthorpe]

On Thu, Apr 08, 2021 at 02:44:45PM +0300, Leon Romanovsky wrote:

> > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > spending a few cycles on debugging it. expect my input in a day or so.
> 
> Can you please post the kernel crash report here?
> I don't see how function rename in patch #3 can cause to the crash.

I looked too, I'm also quite surprised that 1,2,3 alone have a
bug.. Is there some condition where ulp_probe can be null?

Ugh the is_bnxt_re_dev() is horribly gross too

Jason

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

On Thu, Apr 08, 2021 at 08:53:47AM -0300, Jason Gunthorpe wrote:
> On Thu, Apr 08, 2021 at 02:44:45PM +0300, Leon Romanovsky wrote:
> 
> > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > spending a few cycles on debugging it. expect my input in a day or so.
> > 
> > Can you please post the kernel crash report here?
> > I don't see how function rename in patch #3 can cause to the crash.
> 
> I looked too, I'm also quite surprised that 1,2,3 alone have a
> bug.. Is there some condition where ulp_probe can be null?

My speculative guess that they are testing not upstream kernel/module.

> 
> Ugh the is_bnxt_re_dev() is horribly gross too

The whole bnxt* code is very creative. The function bnxt_re_from_netdev()
below is junk too.

It is interesting to see how my review skills from 2017 improved over years :).

Thanks

> 
> Jason

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Thu, Apr 8, 2021 at 5:23 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Thu, Apr 08, 2021 at 02:44:45PM +0300, Leon Romanovsky wrote:
>
> > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > spending a few cycles on debugging it. expect my input in a day or so.
> >
> > Can you please post the kernel crash report here?
> > I don't see how function rename in patch #3 can cause to the crash.
>
> I looked too, I'm also quite surprised that 1,2,3 alone have a
> bug.. Is there some condition where ulp_probe can be null?
>
> Ugh the is_bnxt_re_dev() is horribly gross too
Yeah, it is indeed. I will take this feedback to the internal team.
>
> Jason



-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 3759 bytes --]

On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > >
> > > > > Changelog:
> > > > > v2:
> > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > >    even more ulp_ops derefences.
> > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > >  * Go much deeper and removed useless ULP indirection
> > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > -----------------------------------------------------------------------
> > > > >
> > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > messed with module reference counting in order to implement symbol
> > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > declaration of exported symbol would do the trick.
> > > > >
> > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > logic that isn't relevant for the upstream code.
> > > > >
> > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > >
> > > > > Thanks
> > > > >
> > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > >
> > > > > Leon Romanovsky (5):
> > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > >
> > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > >
> > > > Hi Leon,
> > > >
> > > > After a couple of internal discussions we reached a conclusion to
> > > > implement the Auxbus driver interface and fix the problem once and for
> > > > all.
> > >
> > > Thanks Devesh,
> > >
> > > Jason, it looks like we can proceed with this patchset, because in
> > > auxbus mode this module refcount and ULP indirection logics will be
> > > removed anyway.
> > >
> > > Thanks
> > Hi Leon,
> >
> > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > spending a few cycles on debugging it. expect my input in a day or so.
>
> Can you please post the kernel crash report here?
> I don't see how function rename in patch #3 can cause to the crash.
Hey, unfortunately my kdump service config is giving me tough time on
my host. I will share if I get it.
>
> Thanks
>
> > >
> > > > >
> > > > > --
> > > > > 2.30.2
> > > > >
> > > >
> > > >
> > > > --
> > > > -Regards
> > > > Devesh
> > >
> > >
> >
> >
> > --
> > -Regards
> > Devesh
>
>


-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

On Thu, Apr 08, 2021 at 08:42:57PM +0530, Devesh Sharma wrote:
> On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > >
> > > > > > Changelog:
> > > > > > v2:
> > > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > > >    even more ulp_ops derefences.
> > > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > > >  * Go much deeper and removed useless ULP indirection
> > > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > -----------------------------------------------------------------------
> > > > > >
> > > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > > messed with module reference counting in order to implement symbol
> > > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > > declaration of exported symbol would do the trick.
> > > > > >
> > > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > > logic that isn't relevant for the upstream code.
> > > > > >
> > > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > >
> > > > > > Leon Romanovsky (5):
> > > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > > >
> > > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > > >
> > > > > Hi Leon,
> > > > >
> > > > > After a couple of internal discussions we reached a conclusion to
> > > > > implement the Auxbus driver interface and fix the problem once and for
> > > > > all.
> > > >
> > > > Thanks Devesh,
> > > >
> > > > Jason, it looks like we can proceed with this patchset, because in
> > > > auxbus mode this module refcount and ULP indirection logics will be
> > > > removed anyway.
> > > >
> > > > Thanks
> > > Hi Leon,
> > >
> > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > spending a few cycles on debugging it. expect my input in a day or so.
> >
> > Can you please post the kernel crash report here?
> > I don't see how function rename in patch #3 can cause to the crash.
> Hey, unfortunately my kdump service config is giving me tough time on
> my host. I will share if I get it.

Any news here?

> >
> > Thanks
> >
> > > >
> > > > > >
> > > > > > --
> > > > > > 2.30.2
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > -Regards
> > > > > Devesh
> > > >
> > > >
> > >
> > >
> > > --
> > > -Regards
> > > Devesh
> >
> >
> 
> 
> -- 
> -Regards
> Devesh

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

On Mon, Apr 12, 2021 at 1:10 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Apr 08, 2021 at 08:42:57PM +0530, Devesh Sharma wrote:
> > On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > > > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > >
> > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > >
> > > > > > > Changelog:
> > > > > > > v2:
> > > > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > > > >    even more ulp_ops derefences.
> > > > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > > > >  * Go much deeper and removed useless ULP indirection
> > > > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > -----------------------------------------------------------------------
> > > > > > >
> > > > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > > > messed with module reference counting in order to implement symbol
> > > > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > > > declaration of exported symbol would do the trick.
> > > > > > >
> > > > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > > > logic that isn't relevant for the upstream code.
> > > > > > >
> > > > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > >
> > > > > > > Leon Romanovsky (5):
> > > > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > > > >
> > > > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > > > >
> > > > > > Hi Leon,
> > > > > >
> > > > > > After a couple of internal discussions we reached a conclusion to
> > > > > > implement the Auxbus driver interface and fix the problem once and for
> > > > > > all.
> > > > >
> > > > > Thanks Devesh,
> > > > >
> > > > > Jason, it looks like we can proceed with this patchset, because in
> > > > > auxbus mode this module refcount and ULP indirection logics will be
> > > > > removed anyway.
> > > > >
> > > > > Thanks
> > > > Hi Leon,
> > > >
> > > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > > spending a few cycles on debugging it. expect my input in a day or so.
> > >
> > > Can you please post the kernel crash report here?
> > > I don't see how function rename in patch #3 can cause to the crash.
> > Hey, unfortunately my kdump service config is giving me tough time on
> > my host. I will share if I get it.
>
> Any news here?
Expect something by this Friday. yesterday was a holiday in India.
>
> > >
> > > Thanks
> > >
> > > > >
> > > > > > >
> > > > > > > --
> > > > > > > 2.30.2
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > -Regards
> > > > > > Devesh
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > -Regards
> > > > Devesh
> > >
> > >
> >
> >
> > --
> > -Regards
> > Devesh
>
>


-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

On Wed, Apr 14, 2021 at 07:15:37PM +0530, Devesh Sharma wrote:
> On Mon, Apr 12, 2021 at 1:10 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Thu, Apr 08, 2021 at 08:42:57PM +0530, Devesh Sharma wrote:
> > > On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > > > > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > >
> > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > >
> > > > > > > > Changelog:
> > > > > > > > v2:
> > > > > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > > > > >    even more ulp_ops derefences.
> > > > > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > > > > >  * Go much deeper and removed useless ULP indirection
> > > > > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > -----------------------------------------------------------------------
> > > > > > > >
> > > > > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > > > > messed with module reference counting in order to implement symbol
> > > > > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > > > > declaration of exported symbol would do the trick.
> > > > > > > >
> > > > > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > > > > logic that isn't relevant for the upstream code.
> > > > > > > >
> > > > > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > >
> > > > > > > > Leon Romanovsky (5):
> > > > > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > > > > >
> > > > > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > > > > >
> > > > > > > Hi Leon,
> > > > > > >
> > > > > > > After a couple of internal discussions we reached a conclusion to
> > > > > > > implement the Auxbus driver interface and fix the problem once and for
> > > > > > > all.
> > > > > >
> > > > > > Thanks Devesh,
> > > > > >
> > > > > > Jason, it looks like we can proceed with this patchset, because in
> > > > > > auxbus mode this module refcount and ULP indirection logics will be
> > > > > > removed anyway.
> > > > > >
> > > > > > Thanks
> > > > > Hi Leon,
> > > > >
> > > > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > > > spending a few cycles on debugging it. expect my input in a day or so.
> > > >
> > > > Can you please post the kernel crash report here?
> > > > I don't see how function rename in patch #3 can cause to the crash.
> > > Hey, unfortunately my kdump service config is giving me tough time on
> > > my host. I will share if I get it.
> >
> > Any news here?
> Expect something by this Friday. yesterday was a holiday in India.

Any update? 
This series is close to three weeks already and I would like to progress with it.

Thanks

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 5224 bytes --]

On Sat, Apr 17, 2021 at 1:44 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Apr 14, 2021 at 07:15:37PM +0530, Devesh Sharma wrote:
> > On Mon, Apr 12, 2021 at 1:10 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Thu, Apr 08, 2021 at 08:42:57PM +0530, Devesh Sharma wrote:
> > > > On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > > > > > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > >
> > > > > > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > > > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > > >
> > > > > > > > > Changelog:
> > > > > > > > > v2:
> > > > > > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > > > > > >    even more ulp_ops derefences.
> > > > > > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > > > > > >  * Go much deeper and removed useless ULP indirection
> > > > > > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > > -----------------------------------------------------------------------
> > > > > > > > >
> > > > > > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > > > > > messed with module reference counting in order to implement symbol
> > > > > > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > > > > > declaration of exported symbol would do the trick.
> > > > > > > > >
> > > > > > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > > > > > logic that isn't relevant for the upstream code.
> > > > > > > > >
> > > > > > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > >
> > > > > > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > >
> > > > > > > > > Leon Romanovsky (5):
> > > > > > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > > > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > > > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > > > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > > > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > > > > > >
> > > > > > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > > > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > > > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > > > > > >
> > > > > > > > Hi Leon,
> > > > > > > >
> > > > > > > > After a couple of internal discussions we reached a conclusion to
> > > > > > > > implement the Auxbus driver interface and fix the problem once and for
> > > > > > > > all.
> > > > > > >
> > > > > > > Thanks Devesh,
> > > > > > >
> > > > > > > Jason, it looks like we can proceed with this patchset, because in
> > > > > > > auxbus mode this module refcount and ULP indirection logics will be
> > > > > > > removed anyway.
> > > > > > >
> > > > > > > Thanks
> > > > > > Hi Leon,
> > > > > >
> > > > > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > > > > spending a few cycles on debugging it. expect my input in a day or so.
> > > > >
> > > > > Can you please post the kernel crash report here?
> > > > > I don't see how function rename in patch #3 can cause to the crash.
> > > > Hey, unfortunately my kdump service config is giving me tough time on
> > > > my host. I will share if I get it.
> > >
> > > Any news here?
> > Expect something by this Friday. yesterday was a holiday in India.
>
> Any update?
> This series is close to three weeks already and I would like to progress with it.
Hi Leon,

The host crash I indicated earlier is actually caused by patch 4 and
not by patch 3 from this series. I spent time to root cause the
problem and realized that patch-4 is touching quite many areas which
would require much intrusive testing and validation.
As I indicated earlier, we are implementing the PCI Aux driver
interface at a faster pace. While PCI Aux changes are in progress we
are willing to retain the existing bnxt_re and bnxt_en interface
untouched.
The problem of module referencing would be rectified with PCI aux
change by inheritance.
>
> Thanks




--
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky]

On Sun, Apr 18, 2021 at 12:09:16AM +0530, Devesh Sharma wrote:
> On Sat, Apr 17, 2021 at 1:44 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Apr 14, 2021 at 07:15:37PM +0530, Devesh Sharma wrote:
> > > On Mon, Apr 12, 2021 at 1:10 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Thu, Apr 08, 2021 at 08:42:57PM +0530, Devesh Sharma wrote:
> > > > > On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > > > > > > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > > > > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > > > >
> > > > > > > > > > Changelog:
> > > > > > > > > > v2:
> > > > > > > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > > > > > > >    even more ulp_ops derefences.
> > > > > > > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > > > > > > >  * Go much deeper and removed useless ULP indirection
> > > > > > > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > > > -----------------------------------------------------------------------
> > > > > > > > > >
> > > > > > > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > > > > > > messed with module reference counting in order to implement symbol
> > > > > > > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > > > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > > > > > > declaration of exported symbol would do the trick.
> > > > > > > > > >
> > > > > > > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > > > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > > > > > > logic that isn't relevant for the upstream code.
> > > > > > > > > >
> > > > > > > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > > > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > > >
> > > > > > > > > > Leon Romanovsky (5):
> > > > > > > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > > > > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > > > > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > > > > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > > > > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > > > > > > >
> > > > > > > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > > > > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > > > > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > > > > > > >
> > > > > > > > > Hi Leon,
> > > > > > > > >
> > > > > > > > > After a couple of internal discussions we reached a conclusion to
> > > > > > > > > implement the Auxbus driver interface and fix the problem once and for
> > > > > > > > > all.
> > > > > > > >
> > > > > > > > Thanks Devesh,
> > > > > > > >
> > > > > > > > Jason, it looks like we can proceed with this patchset, because in
> > > > > > > > auxbus mode this module refcount and ULP indirection logics will be
> > > > > > > > removed anyway.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > Hi Leon,
> > > > > > >
> > > > > > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > > > > > spending a few cycles on debugging it. expect my input in a day or so.
> > > > > >
> > > > > > Can you please post the kernel crash report here?
> > > > > > I don't see how function rename in patch #3 can cause to the crash.
> > > > > Hey, unfortunately my kdump service config is giving me tough time on
> > > > > my host. I will share if I get it.
> > > >
> > > > Any news here?
> > > Expect something by this Friday. yesterday was a holiday in India.
> >
> > Any update?
> > This series is close to three weeks already and I would like to progress with it.
> Hi Leon,
> 
> The host crash I indicated earlier is actually caused by patch 4 and
> not by patch 3 from this series. I spent time to root cause the
> problem and realized that patch-4 is touching quite many areas which
> would require much intrusive testing and validation.
> As I indicated earlier, we are implementing the PCI Aux driver
> interface at a faster pace. While PCI Aux changes are in progress we
> are willing to retain the existing bnxt_re and bnxt_en interface
> untouched.
> The problem of module referencing would be rectified with PCI aux
> change by inheritance.

Sorry no, the first three patches are not controversial and better to be
applied now. They do the right thing and they are correct.

There is a little trust in your promises above after you didn't show us kernel
panic despite our numerous requests. I also very sceptical in Broadcom ability
to provide auxbus implementation in timely manner.

It is worth to mention that auxbus won't eliminate the patches #4 and #5, but
will embed them into your auxbus conversion.

Jason, please take first three patches so internal HW IB driver won't do the crazy
module management that is totally out of scope for drivers/infiniband and not needed.

Thanks

> >
> > Thanks
> 
> 
> 
> 
> --
> -Regards
> Devesh

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Jason Gunthorpe]

On Sun, Apr 18, 2021 at 12:09:16AM +0530, Devesh Sharma wrote:

> The host crash I indicated earlier is actually caused by patch 4 and
> not by patch 3 from this series. I spent time to root cause the

This makes a lot more sense.

The ulp_id stuff does need to go away as well though.

> problem and realized that patch-4 is touching quite many areas which
> would require much intrusive testing and validation.
> As I indicated earlier, we are implementing the PCI Aux driver
> interface at a faster pace.

Doing an aux driver doesn't mean you get to keep all these single
implementation function pointers - see the discussion around Intel's
patches.

> The problem of module referencing would be rectified with PCI aux
> change by inheritance.

The first three patches are clearly an improvement, and quite trivial,
so I'm going to take them.

Jason

Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Devesh Sharma]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On Mon, Apr 19, 2021 at 11:08 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Sun, Apr 18, 2021 at 12:09:16AM +0530, Devesh Sharma wrote:
>
> > The host crash I indicated earlier is actually caused by patch 4 and
> > not by patch 3 from this series. I spent time to root cause the
>
> This makes a lot more sense.
>
> The ulp_id stuff does need to go away as well though.
We shall address this concern with Aux implementation.
>
> > problem and realized that patch-4 is touching quite many areas which
> > would require much intrusive testing and validation.
> > As I indicated earlier, we are implementing the PCI Aux driver
> > interface at a faster pace.
>
> Doing an aux driver doesn't mean you get to keep all these single
> implementation function pointers - see the discussion around Intel's
> patches.
Sure, We will try addressing this concern as well. Could you please
point me to the exact patches please...
>
> > The problem of module referencing would be rectified with PCI aux
> > change by inheritance.
>
> The first three patches are clearly an improvement, and quite trivial,
> so I'm going to take them.
To my mind the first 3 patches in this series are fine and I agree to
Ack those. Of-course I want to spend some more time
establishing all of our internal test harness  passing. I can Ack
those in a couple of days at the earliest.
Further, I do not want to proceed with patch 4 and 5 as those patches
are too intrusive and cause host hang/crash during rmmod bnxt_re
followed by rmmod bnxt_en.
Probably this is some kind of race. If I try to add a few prints to
the debug, the problem does not appear.
Since, we want to remain focused on PCI Aux implementation instead of
resolving instabilities at this point, we would like to pick up the
idea from 4 and 5 in our PCI Aux implementation.

>
> Jason



-- 
-Regards
Devesh

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]