[PATCH for-next 1/2] RDMA/bnxt_re: Initialize mutex dbq_lock

[PATCH for-next 1/2] RDMA/bnxt_re: Initialize mutex dbq_lock

[Selvin Xavier]

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

From: Kashyap Desai <kashyap.desai@broadcom.com>

Fix the missing dbq_lock mutex initialization

Fixes: 2ad4e6303a6d ("RDMA/bnxt_re: Implement doorbell pacing algorithm")
Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index f34ce49..061a89b 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -920,6 +920,7 @@ static struct bnxt_re_dev *bnxt_re_dev_add(struct bnxt_aux_priv *aux_priv,
 	rdev->id = rdev->en_dev->pdev->devfn;
 	INIT_LIST_HEAD(&rdev->qp_list);
 	mutex_init(&rdev->qp_lock);
+	mutex_init(&rdev->pacing.dbq_lock);
 	atomic_set(&rdev->stats.res.qp_count, 0);
 	atomic_set(&rdev->stats.res.cq_count, 0);
 	atomic_set(&rdev->stats.res.srq_count, 0);
-- 
2.5.5


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

[PATCH for-next 2/2] RDMA/bnxt_re: Protect the PD table bitmap

[Selvin Xavier]

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

Syncrhonization is required to avoid simultaneous allocation
of the PD. Add a new mutex lock to handle allocation from
the PD table.

Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c  |  2 +-
 drivers/infiniband/hw/bnxt_re/qplib_res.c | 26 ++++++++++++++++++++------
 drivers/infiniband/hw/bnxt_re/qplib_res.h |  4 +++-
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index c0a7181..b19334c 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -619,7 +619,7 @@ int bnxt_re_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
 	int rc = 0;
 
 	pd->rdev = rdev;
-	if (bnxt_qplib_alloc_pd(&rdev->qplib_res.pd_tbl, &pd->qplib_pd)) {
+	if (bnxt_qplib_alloc_pd(&rdev->qplib_res, &pd->qplib_pd)) {
 		ibdev_err(&rdev->ibdev, "Failed to allocate HW PD");
 		rc = -ENOMEM;
 		goto fail;
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c
index 6f1e8b7..79c43c2 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
@@ -642,31 +642,44 @@ static void bnxt_qplib_init_sgid_tbl(struct bnxt_qplib_sgid_tbl *sgid_tbl,
 }
 
 /* PDs */
-int bnxt_qplib_alloc_pd(struct bnxt_qplib_pd_tbl *pdt, struct bnxt_qplib_pd *pd)
+int bnxt_qplib_alloc_pd(struct bnxt_qplib_res  *res, struct bnxt_qplib_pd *pd)
 {
+	struct bnxt_qplib_pd_tbl *pdt = &res->pd_tbl;
 	u32 bit_num;
+	int rc = 0;
 
+	mutex_lock(&res->pd_tbl_lock);
 	bit_num = find_first_bit(pdt->tbl, pdt->max);
-	if (bit_num == pdt->max)
-		return -ENOMEM;
+	if (bit_num == pdt->max) {
+		rc = -ENOMEM;
+		goto exit;
+	}
 
 	/* Found unused PD */
 	clear_bit(bit_num, pdt->tbl);
 	pd->id = bit_num;
-	return 0;
+exit:
+	mutex_unlock(&res->pd_tbl_lock);
+	return rc;
 }
 
 int bnxt_qplib_dealloc_pd(struct bnxt_qplib_res *res,
 			  struct bnxt_qplib_pd_tbl *pdt,
 			  struct bnxt_qplib_pd *pd)
 {
+	int rc = 0;
+
+	mutex_lock(&res->pd_tbl_lock);
 	if (test_and_set_bit(pd->id, pdt->tbl)) {
 		dev_warn(&res->pdev->dev, "Freeing an unused PD? pdn = %d\n",
 			 pd->id);
-		return -EINVAL;
+		rc = -EINVAL;
+		goto exit;
 	}
 	pd->id = 0;
-	return 0;
+exit:
+	mutex_unlock(&res->pd_tbl_lock);
+	return rc;
 }
 
 static void bnxt_qplib_free_pd_tbl(struct bnxt_qplib_pd_tbl *pdt)
@@ -691,6 +704,7 @@ static int bnxt_qplib_alloc_pd_tbl(struct bnxt_qplib_res *res,
 
 	pdt->max = max;
 	memset((u8 *)pdt->tbl, 0xFF, bytes);
+	mutex_init(&res->pd_tbl_lock);
 
 	return 0;
 }
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.h b/drivers/infiniband/hw/bnxt_re/qplib_res.h
index 57161d3..5949f00 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_res.h
+++ b/drivers/infiniband/hw/bnxt_re/qplib_res.h
@@ -277,6 +277,8 @@ struct bnxt_qplib_res {
 	struct net_device		*netdev;
 	struct bnxt_qplib_rcfw		*rcfw;
 	struct bnxt_qplib_pd_tbl	pd_tbl;
+	/* To protect the pd table bit map */
+	struct mutex			pd_tbl_lock;
 	struct bnxt_qplib_sgid_tbl	sgid_tbl;
 	struct bnxt_qplib_dpi_tbl	dpi_tbl;
 	/* To protect the dpi table bit map */
@@ -368,7 +370,7 @@ void bnxt_qplib_free_hwq(struct bnxt_qplib_res *res,
 			 struct bnxt_qplib_hwq *hwq);
 int bnxt_qplib_alloc_init_hwq(struct bnxt_qplib_hwq *hwq,
 			      struct bnxt_qplib_hwq_attr *hwq_attr);
-int bnxt_qplib_alloc_pd(struct bnxt_qplib_pd_tbl *pd_tbl,
+int bnxt_qplib_alloc_pd(struct bnxt_qplib_res *res,
 			struct bnxt_qplib_pd *pd);
 int bnxt_qplib_dealloc_pd(struct bnxt_qplib_res *res,
 			  struct bnxt_qplib_pd_tbl *pd_tbl,
-- 
2.5.5


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

Re: [PATCH for-next 2/2] RDMA/bnxt_re: Protect the PD table bitmap

[Jason Gunthorpe]

On Mon, Aug 14, 2023 at 10:00:19AM -0700, Selvin Xavier wrote:
> Syncrhonization is required to avoid simultaneous allocation
> of the PD. Add a new mutex lock to handle allocation from
> the PD table.
> 
> Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c  |  2 +-
>  drivers/infiniband/hw/bnxt_re/qplib_res.c | 26 ++++++++++++++++++++------
>  drivers/infiniband/hw/bnxt_re/qplib_res.h |  4 +++-
>  3 files changed, 24 insertions(+), 8 deletions(-)

This needs a fixes line, it seems like a serious bug??

> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> index 6f1e8b7..79c43c2 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> @@ -642,31 +642,44 @@ static void bnxt_qplib_init_sgid_tbl(struct bnxt_qplib_sgid_tbl *sgid_tbl,
>  }
>  
>  /* PDs */
> -int bnxt_qplib_alloc_pd(struct bnxt_qplib_pd_tbl *pdt, struct bnxt_qplib_pd *pd)
> +int bnxt_qplib_alloc_pd(struct bnxt_qplib_res  *res, struct bnxt_qplib_pd *pd)
>  {
> +	struct bnxt_qplib_pd_tbl *pdt = &res->pd_tbl;
>  	u32 bit_num;
> +	int rc = 0;
>  
> +	mutex_lock(&res->pd_tbl_lock);
>  	bit_num = find_first_bit(pdt->tbl, pdt->max);

Please make a followup patch to change this into an IDA unless the pd
max is really small. Don't opencode IDAs in drivers..

Jason

Re: [PATCH for-next 2/2] RDMA/bnxt_re: Protect the PD table bitmap

[Selvin Xavier]

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

On Fri, Aug 18, 2023 at 9:50 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Aug 14, 2023 at 10:00:19AM -0700, Selvin Xavier wrote:
> > Syncrhonization is required to avoid simultaneous allocation
> > of the PD. Add a new mutex lock to handle allocation from
> > the PD table.
> >
> > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > ---
> >  drivers/infiniband/hw/bnxt_re/ib_verbs.c  |  2 +-
> >  drivers/infiniband/hw/bnxt_re/qplib_res.c | 26 ++++++++++++++++++++------
> >  drivers/infiniband/hw/bnxt_re/qplib_res.h |  4 +++-
> >  3 files changed, 24 insertions(+), 8 deletions(-)
>
> This needs a fixes line, it seems like a serious bug??
Yes. It is a critical fix. Will add fixes line and post a v2.
>
> > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> > index 6f1e8b7..79c43c2 100644
> > --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
> > +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> > @@ -642,31 +642,44 @@ static void bnxt_qplib_init_sgid_tbl(struct bnxt_qplib_sgid_tbl *sgid_tbl,
> >  }
> >
> >  /* PDs */
> > -int bnxt_qplib_alloc_pd(struct bnxt_qplib_pd_tbl *pdt, struct bnxt_qplib_pd *pd)
> > +int bnxt_qplib_alloc_pd(struct bnxt_qplib_res  *res, struct bnxt_qplib_pd *pd)
> >  {
> > +     struct bnxt_qplib_pd_tbl *pdt = &res->pd_tbl;
> >       u32 bit_num;
> > +     int rc = 0;
> >
> > +     mutex_lock(&res->pd_tbl_lock);
> >       bit_num = find_first_bit(pdt->tbl, pdt->max);
>
> Please make a followup patch to change this into an IDA unless the pd
> max is really small. Don't opencode IDAs in drivers..
pd max is 64k. We will create a followup patch . Thanks.
>
> Jason

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

Re: [PATCH for-next 2/2] RDMA/bnxt_re: Protect the PD table bitmap

[Leon Romanovsky]

On Sun, Aug 20, 2023 at 01:34:17AM +0530, Selvin Xavier wrote:
> On Fri, Aug 18, 2023 at 9:50 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Mon, Aug 14, 2023 at 10:00:19AM -0700, Selvin Xavier wrote:
> > > Syncrhonization is required to avoid simultaneous allocation
> > > of the PD. Add a new mutex lock to handle allocation from
> > > the PD table.
> > >
> > > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > ---
> > >  drivers/infiniband/hw/bnxt_re/ib_verbs.c  |  2 +-
> > >  drivers/infiniband/hw/bnxt_re/qplib_res.c | 26 ++++++++++++++++++++------
> > >  drivers/infiniband/hw/bnxt_re/qplib_res.h |  4 +++-
> > >  3 files changed, 24 insertions(+), 8 deletions(-)
> >
> > This needs a fixes line, it seems like a serious bug??
> Yes. It is a critical fix. Will add fixes line and post a v2.

We already applied v1 and tagged for-next with it. Unless Jason wants to
rebase that branch, it is too late for v2.

Thanks

Re: [PATCH for-next 2/2] RDMA/bnxt_re: Protect the PD table bitmap

[Selvin Xavier]

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

On Sun, Aug 20, 2023 at 3:02 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sun, Aug 20, 2023 at 01:34:17AM +0530, Selvin Xavier wrote:
> > On Fri, Aug 18, 2023 at 9:50 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Mon, Aug 14, 2023 at 10:00:19AM -0700, Selvin Xavier wrote:
> > > > Syncrhonization is required to avoid simultaneous allocation
> > > > of the PD. Add a new mutex lock to handle allocation from
> > > > the PD table.
> > > >
> > > > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > ---
> > > >  drivers/infiniband/hw/bnxt_re/ib_verbs.c  |  2 +-
> > > >  drivers/infiniband/hw/bnxt_re/qplib_res.c | 26 ++++++++++++++++++++------
> > > >  drivers/infiniband/hw/bnxt_re/qplib_res.h |  4 +++-
> > > >  3 files changed, 24 insertions(+), 8 deletions(-)
> > >
> > > This needs a fixes line, it seems like a serious bug??
> > Yes. It is a critical fix. Will add fixes line and post a v2.
>
> We already applied v1 and tagged for-next with it. Unless Jason wants to
> rebase that branch, it is too late for v2.
ok. I added only the fixes tag as per Jason's comment in v2.
>
> Thanks

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