diff mbox

[v2,02/11] IB/core: Change port_attr.sm_lid from 16 to 32 bits

Message ID 1479843532-47496-3-git-send-email-dasaratharaman.chandramouli@intel.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Dasaratharaman Chandramouli Nov. 22, 2016, 7:38 p.m. UTC 
sm_lid field in port_attr is increased to 32 bits. This
enables core components to use the larger addresses if needed.
The user ABI is unchanged and return 16 bit values when queried.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
Signed-off-by: Don Hiatt <don.hiatt@intel.com>
---
 drivers/infiniband/core/sa_query.c    |  2 +-
 drivers/infiniband/core/uverbs_cmd.c  |  6 +++++-
 drivers/infiniband/ulp/srpt/ib_srpt.c |  2 +-
 include/rdma/ib_verbs.h               |  2 +-
 include/rdma/opa_addr.h               | 38 +++++++++++++++++++++++++++++++++++
 5 files changed, 46 insertions(+), 4 deletions(-)
 create mode 100644 include/rdma/opa_addr.h

Comments

Jason Gunthorpe Nov. 22, 2016, 8:57 p.m. UTC | #1 
On Tue, Nov 22, 2016 at 02:38:43PM -0500, Dasaratharaman Chandramouli wrote:
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -958,7 +958,7 @@ static void update_sm_ah(struct work_struct *work)
>  		pr_err("Couldn't find index for default PKey\n");
>  
>  	memset(&ah_attr, 0, sizeof ah_attr);
> -	ah_attr.dlid     = port_attr.sm_lid;
> +	ah_attr.dlid     = (u16)port_attr.sm_lid;

Why are we dropping bits here?

> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -514,7 +514,7 @@ static int srpt_refresh_port(struct srpt_port *sport)
>  	if (ret)
>  		goto err_query_port;
>  
> -	sport->sm_lid = port_attr.sm_lid;
> +	sport->sm_lid = (u16)port_attr.sm_lid;
>  	sport->lid = port_attr.lid;

And here..

> +#if !defined(OPA_ADDR_H)
> +#define OPA_ADDR_H

I don't think we need a header file for this.

> +#define OPA_TO_IB_UCAST_LID(x)	(((x) >= be16_to_cpu(IB_MULTICAST_LID_BASE)) \
> +				 ? 0 : x)

static inline function 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
Dasaratharaman Chandramouli Nov. 22, 2016, 9:13 p.m. UTC | #2 
On 11/22/2016 12:57 PM, Jason Gunthorpe wrote:
> On Tue, Nov 22, 2016 at 02:38:43PM -0500, Dasaratharaman Chandramouli wrote:
>> +++ b/drivers/infiniband/core/sa_query.c
>> @@ -958,7 +958,7 @@ static void update_sm_ah(struct work_struct *work)
>>  		pr_err("Couldn't find index for default PKey\n");
>>
>>  	memset(&ah_attr, 0, sizeof ah_attr);
>> -	ah_attr.dlid     = port_attr.sm_lid;
>> +	ah_attr.dlid     = (u16)port_attr.sm_lid;
>
> Why are we dropping bits here?

Patch 3 increases ah_attr.dlid to 32 bits and the typecast is dropped.
We added it in this patch just to avoid compiler warnings, if any.
>
>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> @@ -514,7 +514,7 @@ static int srpt_refresh_port(struct srpt_port *sport)
>>  	if (ret)
>>  		goto err_query_port;
>>
>> -	sport->sm_lid = port_attr.sm_lid;
>> +	sport->sm_lid = (u16)port_attr.sm_lid;
>>  	sport->lid = port_attr.lid;
>
> And here..
Patch 11 increases lid and sm_lid fields in struct srpt_port and the 
typecast is dropped.
We added it in this patch just to avoid compiler warnings, if any.
>
>> +#if !defined(OPA_ADDR_H)
>> +#define OPA_ADDR_H
>
> I don't think we need a header file for this.
There are other helpers specific to OPA addresses that were required in 
later patches which we thought can go in a separate file.
>
>> +#define OPA_TO_IB_UCAST_LID(x)	(((x) >= be16_to_cpu(IB_MULTICAST_LID_BASE)) \
>> +				 ? 0 : x)
>
> static inline function please.
Will change. Thanks.
>
> 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
Jason Gunthorpe Nov. 22, 2016, 9:24 p.m. UTC | #3 
On Tue, Nov 22, 2016 at 01:13:26PM -0800, Chandramouli, Dasaratharaman wrote:
> 
> 
> On 11/22/2016 12:57 PM, Jason Gunthorpe wrote:
> >On Tue, Nov 22, 2016 at 02:38:43PM -0500, Dasaratharaman Chandramouli wrote:
> >>+++ b/drivers/infiniband/core/sa_query.c
> >>@@ -958,7 +958,7 @@ static void update_sm_ah(struct work_struct *work)
> >> 		pr_err("Couldn't find index for default PKey\n");
> >>
> >> 	memset(&ah_attr, 0, sizeof ah_attr);
> >>-	ah_attr.dlid     = port_attr.sm_lid;
> >>+	ah_attr.dlid     = (u16)port_attr.sm_lid;
> >
> >Why are we dropping bits here?
> 
> Patch 3 increases ah_attr.dlid to 32 bits and the typecast is dropped.
> We added it in this patch just to avoid compiler warnings, if any.

It would be alot better to fix this series so adding typecasts and
then dropping them didn't happen so extensively. Just reodering some
patched would do the trick. That would make it alot less churny and
easy to read..

> >>-	sport->sm_lid = port_attr.sm_lid;
> >>+	sport->sm_lid = (u16)port_attr.sm_lid;
> >> 	sport->lid = port_attr.lid;
> >
> >And here..

> Patch 11 increases lid and sm_lid fields in struct srpt_port and the
> typecast is dropped.  We added it in this patch just to avoid
> compiler warnings, if any.

Eg if you do patch 11 first this hunk goes away.

I also don't really care for all the added u16 casts, the compiler
doesn't warn on implicit demotion without a higher warning level...

> >
> >>+#define OPA_TO_IB_UCAST_LID(x)	(((x) >= be16_to_cpu(IB_MULTICAST_LID_BASE)) \
> >>+				 ? 0 : x)
> >
> >static inline function please.
> Will change. Thanks.

All of them please.

And I think you should re-think how this is being used, the pattern
around OPA_TO_IB_UCAST_LID is copied too many times for my liking, and
isn't this UAPI compatability only?

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
Dasaratharaman Chandramouli Nov. 23, 2016, 5:43 a.m. UTC | #4 
On 11/22/2016 1:24 PM, Jason Gunthorpe wrote:
> On Tue, Nov 22, 2016 at 01:13:26PM -0800, Chandramouli, Dasaratharaman wrote:
>>
>>
>> On 11/22/2016 12:57 PM, Jason Gunthorpe wrote:
>>> On Tue, Nov 22, 2016 at 02:38:43PM -0500, Dasaratharaman Chandramouli wrote:
>>>> +++ b/drivers/infiniband/core/sa_query.c
>>>> @@ -958,7 +958,7 @@ static void update_sm_ah(struct work_struct *work)
>>>> 		pr_err("Couldn't find index for default PKey\n");
>>>>
>>>> 	memset(&ah_attr, 0, sizeof ah_attr);
>>>> -	ah_attr.dlid     = port_attr.sm_lid;
>>>> +	ah_attr.dlid     = (u16)port_attr.sm_lid;
>>>
>>> Why are we dropping bits here?
>>
>> Patch 3 increases ah_attr.dlid to 32 bits and the typecast is dropped.
>> We added it in this patch just to avoid compiler warnings, if any.
>
> It would be alot better to fix this series so adding typecasts and
> then dropping them didn't happen so extensively. Just reodering some
> patched would do the trick. That would make it alot less churny and
> easy to read..
Yes, will re-order them. Thanks
>
>>>> -	sport->sm_lid = port_attr.sm_lid;
>>>> +	sport->sm_lid = (u16)port_attr.sm_lid;
>>>> 	sport->lid = port_attr.lid;
>>>
>>> And here..
>
>> Patch 11 increases lid and sm_lid fields in struct srpt_port and the
>> typecast is dropped.  We added it in this patch just to avoid
>> compiler warnings, if any.
>
> Eg if you do patch 11 first this hunk goes away.
>
> I also don't really care for all the added u16 casts, the compiler
> doesn't warn on implicit demotion without a higher warning level...
>
It's that higher warning level that we were a little concerned about.
If you feel very strongly about not having these casts, they can be 
removed but i would prefer leaving them there to silence certain odd 
ball compiler configurations.
>>>
>>>> +#define OPA_TO_IB_UCAST_LID(x)	(((x) >= be16_to_cpu(IB_MULTICAST_LID_BASE)) \
>>>> +				 ? 0 : x)
>>>
>>> static inline function please.
>> Will change. Thanks.
>
> All of them please.
Will change.
>
> And I think you should re-think how this is being used, the pattern
> around OPA_TO_IB_UCAST_LID is copied too many times for my liking, and
> isn't this UAPI compatability only?
They are only used when there is a user-space query into the kernel for 
lid and sm_lid.
>
> 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/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 81b742c..0b0dc43 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -958,7 +958,7 @@  static void update_sm_ah(struct work_struct *work)
 		pr_err("Couldn't find index for default PKey\n");
 
 	memset(&ah_attr, 0, sizeof ah_attr);
-	ah_attr.dlid     = port_attr.sm_lid;
+	ah_attr.dlid     = (u16)port_attr.sm_lid;
 	ah_attr.sl       = port_attr.sm_sl;
 	ah_attr.port_num = port->port_num;
 	if (port_attr.grh_required) {
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index cb3f515a..7630c92 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -39,6 +39,7 @@ 
 #include <linux/sched.h>
 
 #include <asm/uaccess.h>
+#include <rdma/opa_addr.h>
 
 #include "uverbs.h"
 #include "core_priv.h"
@@ -515,7 +516,10 @@  ssize_t ib_uverbs_query_port(struct ib_uverbs_file *file,
 	resp.qkey_viol_cntr  = attr.qkey_viol_cntr;
 	resp.pkey_tbl_len    = attr.pkey_tbl_len;
 	resp.lid 	     = attr.lid;
-	resp.sm_lid 	     = attr.sm_lid;
+	if (rdma_cap_opa_ah(ib_dev, cmd.port_num))
+		resp.sm_lid  = OPA_TO_IB_UCAST_LID(attr.sm_lid);
+	else
+		resp.sm_lid  = (u16)attr.sm_lid;
 	resp.lmc 	     = attr.lmc;
 	resp.max_vl_num      = attr.max_vl_num;
 	resp.sm_sl 	     = attr.sm_sl;
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0b1f69e..c6d0c47 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -514,7 +514,7 @@  static int srpt_refresh_port(struct srpt_port *sport)
 	if (ret)
 		goto err_query_port;
 
-	sport->sm_lid = port_attr.sm_lid;
+	sport->sm_lid = (u16)port_attr.sm_lid;
 	sport->lid = port_attr.lid;
 
 	ret = ib_query_gid(sport->sdev->device, sport->port, 0, &sport->gid,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 30fa96e..52216f6 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -521,7 +521,7 @@  struct ib_port_attr {
 	u32			qkey_viol_cntr;
 	u16			pkey_tbl_len;
 	u16			lid;
-	u16			sm_lid;
+	u32			sm_lid;
 	u8			lmc;
 	u8			max_vl_num;
 	u8			sm_sl;
diff --git a/include/rdma/opa_addr.h b/include/rdma/opa_addr.h
new file mode 100644
index 0000000..142b327
--- /dev/null
+++ b/include/rdma/opa_addr.h
@@ -0,0 +1,38 @@ 
+/*
+ * Copyright (c) 2016 Intel Corporation.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#if !defined(OPA_ADDR_H)
+#define OPA_ADDR_H
+
+#define OPA_TO_IB_UCAST_LID(x)	(((x) >= be16_to_cpu(IB_MULTICAST_LID_BASE)) \
+				 ? 0 : x)
+#endif /* OPA_ADDR_H */