[PATCH for-3.5 0/3] mlx4 fixes

[PATCH for-3.5 0/3] mlx4 fixes

[Or Gerlitz]

Hi Roland, 

So here's a batch with three fixes to the mlx4 driver, I still don't
have the patch that deals with fixing the MTT scaling by RAM size ready,
hope to get there next week.

Jack Morgenstein (1):
  net/mlx4_core: Fix setting VL_cap in mlx4_SET_PORT wrapper flow

Sagi Grimberg (1):
  IB/mlx4: Fix max_wqe capacity report for query device

Shlomo Pongratz (1):
  IB/mlx4: Fix EQ deallocation under legacy mode

 drivers/infiniband/hw/mlx4/main.c         |   21 ++++++++-------------
 drivers/infiniband/hw/mlx4/mlx4_ib.h      |    8 ++++++++
 drivers/infiniband/hw/mlx4/qp.c           |   25 +++++++++++++++++++------
 drivers/net/ethernet/mellanox/mlx4/port.c |    4 ++--
 4 files changed, 37 insertions(+), 21 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] IB/mlx4: Fix EQ deallocation under legacy mode

[Or Gerlitz]

From: Shlomo Pongratz <shlomop-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Commit e605b743f "IB/mlx4: Increase the number of vectors (EQs) available
for ULPs" didn't handle correctly the case where there aren't enough MSIX
vectors to increase the number of EQs, such that only the legacy EQs are
allocated. This resulted in an attempt to memset to zero the eq table
which was never allocated, and a kernel crash. Fix that by checking in
the teardown flow  if the table of EQs was ever allocated. Also remove
some unneeded setting to zero of the EQ related fields in struct mlx4_ib_dev.

Signed-off-by: Shlomo Pongratz <shlomop-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/main.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index ee1c577..8afea12 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1084,12 +1084,9 @@ static void mlx4_ib_alloc_eqs(struct mlx4_dev *dev, struct mlx4_ib_dev *ibdev)
 	int total_eqs = 0;
 	int i, j, eq;
 
-	/* Init eq table */
-	ibdev->eq_table = NULL;
-	ibdev->eq_added = 0;
-
-	/* Legacy mode? */
-	if (dev->caps.comp_pool == 0)
+	/* Legacy mode or comp_pool is not large enough */
+	if (dev->caps.comp_pool == 0 ||
+	    dev->caps.num_ports > dev->caps.comp_pool)
 		return;
 
 	eq_per_port = rounddown_pow_of_two(dev->caps.comp_pool/
@@ -1135,7 +1132,10 @@ static void mlx4_ib_alloc_eqs(struct mlx4_dev *dev, struct mlx4_ib_dev *ibdev)
 static void mlx4_ib_free_eqs(struct mlx4_dev *dev, struct mlx4_ib_dev *ibdev)
 {
 	int i;
-	int total_eqs;
+
+	/* no additional eqs were added */
+	if (!ibdev->eq_table)
+		return;
 
 	/* Reset the advertised EQ number */
 	ibdev->ib_dev.num_comp_vectors = dev->caps.num_comp_vectors;
@@ -1148,12 +1148,7 @@ static void mlx4_ib_free_eqs(struct mlx4_dev *dev, struct mlx4_ib_dev *ibdev)
 		mlx4_release_eq(dev, ibdev->eq_table[i]);
 	}
 
-	total_eqs = dev->caps.num_comp_vectors + ibdev->eq_added;
-	memset(ibdev->eq_table, 0, total_eqs * sizeof(int));
 	kfree(ibdev->eq_table);
-
-	ibdev->eq_table = NULL;
-	ibdev->eq_added = 0;
 }
 
 static void *mlx4_ib_add(struct mlx4_dev *dev)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Or Gerlitz]

From: Sagi Grimberg <sagig-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>

1. Limit max number of wqes per QP reported when querying the device,
so that ib_create_qp will never fail due to any additional headroom WQEs
allocated.

2. Limit qp resources accepted for ib_create_qp() to the limits
reported in ib_query_device(). In kernel space, make sure that
the limits returned to the caller following qp creation also
lie within the reported device limits. For userspace, report
as before, and do adjustment in libmlx4 (so as not to break ABI).

Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---

Roland, you can see past correspondence from 2007 (...) here:
http://lists.openfabrics.org/pipermail/general/2007-October/042351.html

The libmlx4 patches will be send in a user space batch which has also 
has the user space RAW QP code and more fun stuff, after 3.5-rc1 is out,
but again, this specific patch was written as of not to introduce mlx4 
ABI changes.

 drivers/infiniband/hw/mlx4/main.c    |    2 +-
 drivers/infiniband/hw/mlx4/mlx4_ib.h |    8 ++++++++
 drivers/infiniband/hw/mlx4/qp.c      |   25 +++++++++++++++++++------
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 8afea12..3530c41 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -140,7 +140,7 @@ static int mlx4_ib_query_device(struct ib_device *ibdev,
 	props->max_mr_size	   = ~0ull;
 	props->page_size_cap	   = dev->dev->caps.page_size_cap;
 	props->max_qp		   = dev->dev->caps.num_qps - dev->dev->caps.reserved_qps;
-	props->max_qp_wr	   = dev->dev->caps.max_wqes;
+	props->max_qp_wr	   = dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE;
 	props->max_sge		   = min(dev->dev->caps.max_sq_sg,
 					 dev->dev->caps.max_rq_sg);
 	props->max_cq		   = dev->dev->caps.num_cqs - dev->dev->caps.reserved_cqs;
diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h
index e62297c..6c45b8f 100644
--- a/drivers/infiniband/hw/mlx4/mlx4_ib.h
+++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h
@@ -44,6 +44,14 @@
 #include <linux/mlx4/device.h>
 #include <linux/mlx4/doorbell.h>
 
+enum {
+	MLX4_IB_SQ_MIN_WQE_SHIFT = 6,
+	MLX4_IB_MAX_HEADROOM	 = 2048
+};
+
+#define MLX4_IB_SQ_HEADROOM(shift) ((MLX4_IB_MAX_HEADROOM >> (shift)) + 1)
+#define MLX4_IB_SQ_MAX_SPARE (MLX4_IB_SQ_HEADROOM(MLX4_IB_SQ_MIN_WQE_SHIFT))
+
 struct mlx4_ib_ucontext {
 	struct ib_ucontext	ibucontext;
 	struct mlx4_uar		uar;
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index ceb3332..ca8115f 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -310,8 +310,9 @@ static int set_rq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
 		       int is_user, int has_rq, struct mlx4_ib_qp *qp)
 {
 	/* Sanity check RQ size before proceeding */
-	if (cap->max_recv_wr  > dev->dev->caps.max_wqes  ||
-	    cap->max_recv_sge > dev->dev->caps.max_rq_sg)
+	if (cap->max_recv_wr > dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE ||
+	    cap->max_recv_sge >
+		min(dev->dev->caps.max_sq_sg, dev->dev->caps.max_rq_sg))
 		return -EINVAL;
 
 	if (!has_rq) {
@@ -329,8 +330,19 @@ static int set_rq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
 		qp->rq.wqe_shift = ilog2(qp->rq.max_gs * sizeof (struct mlx4_wqe_data_seg));
 	}
 
-	cap->max_recv_wr  = qp->rq.max_post = qp->rq.wqe_cnt;
-	cap->max_recv_sge = qp->rq.max_gs;
+	/* leave userspace return values as they were, so as not to break ABI */
+	if (is_user) {
+		cap->max_recv_wr  = qp->rq.max_post = qp->rq.wqe_cnt;
+		cap->max_recv_sge = qp->rq.max_gs;
+	} else {
+		cap->max_recv_wr  = qp->rq.max_post =
+			min(dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE, qp->rq.wqe_cnt);
+		cap->max_recv_sge = min(qp->rq.max_gs,
+					min(dev->dev->caps.max_sq_sg,
+					dev->dev->caps.max_rq_sg));
+	}
+	/* We don't support inline sends for kernel QPs (yet) */
+
 
 	return 0;
 }
@@ -341,8 +353,9 @@ static int set_kernel_sq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
 	int s;
 
 	/* Sanity check SQ size before proceeding */
-	if (cap->max_send_wr	 > dev->dev->caps.max_wqes  ||
-	    cap->max_send_sge	 > dev->dev->caps.max_sq_sg ||
+	if (cap->max_send_wr	 > (dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE) ||
+	    cap->max_send_sge	 >
+		min(dev->dev->caps.max_sq_sg, dev->dev->caps.max_rq_sg) ||
 	    cap->max_inline_data + send_wqe_overhead(type, qp->flags) +
 	    sizeof (struct mlx4_wqe_inline_seg) > dev->dev->caps.max_sq_desc_sz)
 		return -EINVAL;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] net/mlx4_core: Fix setting VL_cap in mlx4_SET_PORT wrapper flow

[Or Gerlitz]

From: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>

Commit 096335b3f (mlx4_core: Allow dynamic MTU configuration for IB
ports) modifies the port VL setting.  This exposes a bug in procedure
mlx4_common_set_port(), where the VL cap value passed in (inside the
command mailbox) is incorrectly zeroed-out. The problem flow is as follows:

mlx4_SET_PORT modifies the VL_cap field (byte 3 of the mailbox).
Since the SET_PORT command is paravirtualized on the master as
well as on the slaves, mlx4_SET_PORT_wrapper() is invoked on the
master. This procedure calls mlx4_common_set_port() where mailbox
byte 3 gets overwritten by a code which should only set a single
bit in that byte (for the reset qkey counter flag) -- but instead
overwrites the entire byte.

The result is that when running in SRIOV mode, the VL_cap will
be set to zero, fix that.

Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/net/ethernet/mellanox/mlx4/port.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/port.c b/drivers/net/ethernet/mellanox/mlx4/port.c
index 1fe2c7a..a8fb529 100644
--- a/drivers/net/ethernet/mellanox/mlx4/port.c
+++ b/drivers/net/ethernet/mellanox/mlx4/port.c
@@ -697,10 +697,10 @@ static int mlx4_common_set_port(struct mlx4_dev *dev, int slave, u32 in_mod,
 	if (slave != dev->caps.function)
 		memset(inbox->buf, 0, 256);
 	if (dev->flags & MLX4_FLAG_OLD_PORT_CMDS) {
-		*(u8 *) inbox->buf	   = !!reset_qkey_viols << 6;
+		*(u8 *) inbox->buf	   |= !!reset_qkey_viols << 6;
 		((__be32 *) inbox->buf)[2] = agg_cap_mask;
 	} else {
-		((u8 *) inbox->buf)[3]     = !!reset_qkey_viols;
+		((u8 *) inbox->buf)[3]     |= !!reset_qkey_viols;
 		((__be32 *) inbox->buf)[1] = agg_cap_mask;
 	}
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Hefty, Sean]

> 1. Limit max number of wqes per QP reported when querying the device,
> so that ib_create_qp will never fail due to any additional headroom WQEs
> allocated.
> 
> 2. Limit qp resources accepted for ib_create_qp() to the limits
> reported in ib_query_device(). In kernel space, make sure that
> the limits returned to the caller following qp creation also
> lie within the reported device limits. For userspace, report
> as before, and do adjustment in libmlx4 (so as not to break ABI).

I believe that I was *just* hitting into this bug.

I don't understand the part about breaking the ABI.  Why wouldn't you just return the correct values from the kernel?  Are the values being returned now usable?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Or Gerlitz]

On Thu, May 24, 2012 at 6:42 PM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> I believe that I was *just* hitting into this bug.

Good, so we came just in time.

> I don't understand the part about breaking the ABI.  Why wouldn't you just
> return the correct values from the kernel?  Are the values being returned
> now usable?

Addressing the 2nd part of your question, the patch makes sure that
the values returned now are usable for kernel consumers, as for user
consumers, that's work in done in the patches I just sent to libmlx4.

As for the 1st part, this goes back to that 2007 discussion... see
http://lists.openfabrics.org/pipermail/general/2007-October/042529.html
and following mails. So the way we went is to allow unpatched libmlx4
work with patched kernels, and patch libmlx4 separately

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Or Gerlitz]

On 5/25/2012 6:04 AM, Or Gerlitz wrote:
>> I don't understand the part about breaking the ABI.  Why wouldn't you just
>> return the correct values from the kernel?  Are the values being returned now usable?
>
> Addressing the 2nd part of your question, the patch makes sure that
> the values returned now are usable for kernel consumers, as for user
> consumers, that's work in done in the patches I just sent to libmlx4.

Sean,

Did you try out the patches? was it helpful to address the problem 
you're facing?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Hefty, Sean]

> Did you try out the patches? was it helpful to address the problem
> you're facing?

I have not had time to test it yet
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Or Gerlitz]

On Wed, May 30, 2012 at 2:25 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

>> Did you try out the patches? was it helpful to address the problem
>> you're facing?

> I have not had time to test it yet

so whenever you do have that time...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] IB/mlx4: Fix EQ deallocation under legacy mode

[Roland Dreier]

thanks, applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net/mlx4_core: Fix setting VL_cap in mlx4_SET_PORT wrapper flow

[Roland Dreier]

thanks, applied
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] net/mlx4_core: Fix setting VL_cap in mlx4_SET_PORT wrapper flow

[Or Gerlitz]

On 6/4/2012 9:05 AM, Roland Dreier wrote:
> thanks, applied

Happy to hear that! any questions/clarifications you still need on patch #2?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Roland Dreier]

Looks OK, applied.  One question:

>        /* Sanity check RQ size before proceeding */
> -       if (cap->max_recv_wr  > dev->dev->caps.max_wqes  ||
> -           cap->max_recv_sge > dev->dev->caps.max_rq_sg)
> +       if (cap->max_recv_wr > dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE ||
> +           cap->max_recv_sge >
> +               min(dev->dev->caps.max_sq_sg, dev->dev->caps.max_rq_sg))

Why do we need to check max_recv_sge against caps.max_sq_sg as well as
caps.max_rq_sg?

> @@ -329,8 +330,19 @@ static int set_rq_size(struct mlx4_ib_dev *dev, struct ib_qp_cap *cap,
>                qp->rq.wqe_shift = ilog2(qp->rq.max_gs * sizeof (struct mlx4_wqe_data_seg));
>        }
>
> -       cap->max_recv_wr  = qp->rq.max_post = qp->rq.wqe_cnt;
> -       cap->max_recv_sge = qp->rq.max_gs;
> +       /* leave userspace return values as they were, so as not to break ABI */
> +       if (is_user) {
> +               cap->max_recv_wr  = qp->rq.max_post = qp->rq.wqe_cnt;
> +               cap->max_recv_sge = qp->rq.max_gs;
> +       } else {
> +               cap->max_recv_wr  = qp->rq.max_post =
> +                       min(dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE, qp->rq.wqe_cnt);
> +               cap->max_recv_sge = min(qp->rq.max_gs,
> +                                       min(dev->dev->caps.max_sq_sg,
> +                                       dev->dev->caps.max_rq_sg));
> +       }
> +       /* We don't support inline sends for kernel QPs (yet) */
> +

That comment looks like cut and paste detritus, I deleted it...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Jack Morgenstein]

On Wednesday 06 June 2012 20:05, Roland Dreier wrote:
> >        /* Sanity check RQ size before proceeding */
> > -       if (cap->max_recv_wr  > dev->dev->caps.max_wqes  ||
> > -           cap->max_recv_sge > dev->dev->caps.max_rq_sg)
> > +       if (cap->max_recv_wr > dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE ||
> > +           cap->max_recv_sge >
> > +               min(dev->dev->caps.max_sq_sg, dev->dev->caps.max_rq_sg))
> 
> Why do we need to check max_recv_sge against caps.max_sq_sg as well as
> caps.max_rq_sg?
> 
Because the HCA provides separate sizes for max_recv_sge and max_send_sge, but
the IB spec only allows for a single max_sg_entries value.

We therefore enforce the lower limit of the 2.

-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/mlx4: Fix max_wqe capacity report for query device

[Jack Morgenstein]

On Wednesday 06 June 2012 20:05, Roland Dreier wrote:
> Looks OK, applied.  One question:
> 
P.S., thanks for taking this so quickly!

-Jack

P.S. it would help us with our SRIOV IB work if you could
rebase your for-next git to 3.5-rc1. There are a couple of
commits to the mlx4_core there which impact the SRIOV-IB
patch set.

Thanks in advance!

-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html