diff mbox

[05/10] IB/qib: Use rdmavt lid defines in qib

Message ID 20151207204911.11219.50129.stgit@phlsvslse11.ph.intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Dennis Dalessandro Dec. 7, 2015, 8:49 p.m. UTC 
Original patch for AH changes from Kamal Heib <kamalh@mellanox.com>, split
apart from original. This patch also removes the qib specific multicast
lid base and permissive lid defines since they are no longer needed.

Use common LID defines in qib driver.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Kamal Heib <kamalh@mellanox.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/qib/qib_common.h |    2 --
 drivers/infiniband/hw/qib/qib_qp.c     |    1 +
 drivers/infiniband/hw/qib/qib_verbs.c  |    4 ++--
 3 files changed, 3 insertions(+), 4 deletions(-)


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

Comments

Jason Gunthorpe Dec. 7, 2015, 8:54 p.m. UTC | #1 
On Mon, Dec 07, 2015 at 03:49:12PM -0500, Dennis Dalessandro wrote:
>  	/* A multicast address requires a GRH (see ch. 8.4.1). */
> -	if (ah_attr->dlid >= QIB_MULTICAST_LID_BASE &&
> -	    ah_attr->dlid != QIB_PERMISSIVE_LID &&
> +	if (ah_attr->dlid >= be16_to_cpu(IB_MULTICAST_LID_BASE) &&
> +	    ah_attr->dlid != be16_to_cpu(IB_LID_PERMISSIVE) &&

Uh cpu_to_be16 please..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny Dec. 8, 2015, 7:24 p.m. UTC | #2 
On Mon, Dec 07, 2015 at 01:54:39PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 07, 2015 at 03:49:12PM -0500, Dennis Dalessandro wrote:
> >  	/* A multicast address requires a GRH (see ch. 8.4.1). */
> > -	if (ah_attr->dlid >= QIB_MULTICAST_LID_BASE &&
> > -	    ah_attr->dlid != QIB_PERMISSIVE_LID &&
> > +	if (ah_attr->dlid >= be16_to_cpu(IB_MULTICAST_LID_BASE) &&
> > +	    ah_attr->dlid != be16_to_cpu(IB_LID_PERMISSIVE) &&
> 
> Uh cpu_to_be16 please..

But, the defines are big endian and the dlid here is cpu endian.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Dec. 8, 2015, 8:55 p.m. UTC | #3 
On Tue, Dec 08, 2015 at 02:24:55PM -0500, ira.weiny wrote:
> On Mon, Dec 07, 2015 at 01:54:39PM -0700, Jason Gunthorpe wrote:
> > On Mon, Dec 07, 2015 at 03:49:12PM -0500, Dennis Dalessandro wrote:
> > >  	/* A multicast address requires a GRH (see ch. 8.4.1). */
> > > -	if (ah_attr->dlid >= QIB_MULTICAST_LID_BASE &&
> > > -	    ah_attr->dlid != QIB_PERMISSIVE_LID &&
> > > +	if (ah_attr->dlid >= be16_to_cpu(IB_MULTICAST_LID_BASE) &&
> > > +	    ah_attr->dlid != be16_to_cpu(IB_LID_PERMISSIVE) &&
> > 
> > Uh cpu_to_be16 please..
> 
> But, the defines are big endian and the dlid here is cpu endian.

Hurm, I can't even find the patch that adds IB_MULTICAST_LID_BASE..

But I believe you, and think that is pretty gross to have the constant
be BE when typical uses like this are CPU..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dennis Dalessandro Dec. 8, 2015, 11:19 p.m. UTC | #4 
On Tue, Dec 08, 2015 at 01:55:11PM -0700, Jason Gunthorpe wrote:
>On Tue, Dec 08, 2015 at 02:24:55PM -0500, ira.weiny wrote:
>> On Mon, Dec 07, 2015 at 01:54:39PM -0700, Jason Gunthorpe wrote:
>> > On Mon, Dec 07, 2015 at 03:49:12PM -0500, Dennis Dalessandro wrote:
>> > >  	/* A multicast address requires a GRH (see ch. 8.4.1). */
>> > > -	if (ah_attr->dlid >= QIB_MULTICAST_LID_BASE &&
>> > > -	    ah_attr->dlid != QIB_PERMISSIVE_LID &&
>> > > +	if (ah_attr->dlid >= be16_to_cpu(IB_MULTICAST_LID_BASE) &&
>> > > +	    ah_attr->dlid != be16_to_cpu(IB_LID_PERMISSIVE) &&
>> > 
>> > Uh cpu_to_be16 please..
>> 
>> But, the defines are big endian and the dlid here is cpu endian.
>
>Hurm, I can't even find the patch that adds IB_MULTICAST_LID_BASE..
>
>But I believe you, and think that is pretty gross to have the constant
>be BE when typical uses like this are CPU..

So what is the consensus here? Should we leave it alone for now and 
potentially go back and deal with this separately?  Just define the new one 
as LE and use it, even though it doesn't match the rest?  Something else 
entirely?

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Dec. 8, 2015, 11:54 p.m. UTC | #5 
On Tue, Dec 08, 2015 at 06:19:41PM -0500, Dennis Dalessandro wrote:

> So what is the consensus here? Should we leave it alone for now and
> potentially go back and deal with this separately?  Just define the new one
> as LE and use it, even though it doesn't match the rest?  Something else
> entirely?

I'm okay if you stick with what you have, there are too many other
cases of byteswapped constants to get too excited I guess. :(

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

Patch

diff --git a/drivers/infiniband/hw/qib/qib_common.h b/drivers/infiniband/hw/qib/qib_common.h
index 4fb78ab..1d87ec0 100644
--- a/drivers/infiniband/hw/qib/qib_common.h
+++ b/drivers/infiniband/hw/qib/qib_common.h
@@ -742,14 +742,12 @@  struct qib_tid_session_member {
 #define SIZE_OF_CRC 1
 
 #define QIB_DEFAULT_P_KEY 0xFFFF
-#define QIB_PERMISSIVE_LID 0xFFFF
 #define QIB_AETH_CREDIT_SHIFT 24
 #define QIB_AETH_CREDIT_MASK 0x1F
 #define QIB_AETH_CREDIT_INVAL 0x1F
 #define QIB_PSN_MASK 0xFFFFFF
 #define QIB_MSN_MASK 0xFFFFFF
 #define QIB_QPN_MASK 0xFFFFFF
-#define QIB_MULTICAST_LID_BASE 0xC000
 #define QIB_EAGER_TID_ID QLOGIC_IB_I_TID_MASK
 #define QIB_MULTICAST_QPN 0xFFFFFF
 
diff --git a/drivers/infiniband/hw/qib/qib_qp.c b/drivers/infiniband/hw/qib/qib_qp.c
index e6544dc..116d323 100644
--- a/drivers/infiniband/hw/qib/qib_qp.c
+++ b/drivers/infiniband/hw/qib/qib_qp.c
@@ -35,6 +35,7 @@ 
 #include <linux/err.h>
 #include <linux/vmalloc.h>
 #include <linux/jhash.h>
+#include <rdma/rdma_vt.h>
 #ifdef CONFIG_DEBUG_FS
 #include <linux/seq_file.h>
 #endif
diff --git a/drivers/infiniband/hw/qib/qib_verbs.c b/drivers/infiniband/hw/qib/qib_verbs.c
index c64e054..c93ace0 100644
--- a/drivers/infiniband/hw/qib/qib_verbs.c
+++ b/drivers/infiniband/hw/qib/qib_verbs.c
@@ -1754,8 +1754,8 @@  static int qib_query_gid(struct ib_device *ibdev, u8 port,
 int qib_check_ah(struct ib_device *ibdev, struct ib_ah_attr *ah_attr)
 {
 	/* A multicast address requires a GRH (see ch. 8.4.1). */
-	if (ah_attr->dlid >= QIB_MULTICAST_LID_BASE &&
-	    ah_attr->dlid != QIB_PERMISSIVE_LID &&
+	if (ah_attr->dlid >= be16_to_cpu(IB_MULTICAST_LID_BASE) &&
+	    ah_attr->dlid != be16_to_cpu(IB_LID_PERMISSIVE) &&
 	    !(ah_attr->ah_flags & IB_AH_GRH))
 		goto bail;
 	if ((ah_attr->ah_flags & IB_AH_GRH) &&