opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

Signed-off-by Hal Rosenstock <[email protected]>

---
 opensm/opensm/osm_sa.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

opensm/opensm/osm_sa.c +1 -1

index 0aca81f..8325632 100644
@@ -3,6 +3,7 @@
  * Copyright (c) 2002-2010 Mellanox Technologies LTD. All rights reserved.
  * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
  * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
+ * Copyright (c) 2010 HNR Consulting. 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

@@ -454,8 +455,15 @@ void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size,
 	/* C15-0.1.5 - always return SM_Key = 0 (table 185 p 884) */
 	resp_sa_mad->sm_key = 0;
 
-	/* Fill in the offset (paylen will be done by the rmpp SAR) */

opensm/opensm/osm_sa.c +0 -0

 	p = ib_sa_mad_get_payload_ptr(resp_sa_mad);
 

Signature · 1 line

1.5.6.4

Hi Hal,

Quoted 22 lines · Hal Rosenstock

Signed-off-by Hal Rosenstock <[email protected]>

---
 opensm/opensm/osm_sa.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

opensm/opensm/osm_sa.c +1 -0

index 0aca81f..8325632 100644
@@ -3,6 +3,7 @@
  * Copyright (c) 2002-2010 Mellanox Technologies LTD. All rights reserved.
  * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
  * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
+ * Copyright (c) 2010 HNR Consulting. 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

@@ -454,8 +455,15 @@ void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size,
      /* C15-0.1.5 - always return SM_Key = 0 (table 185 p 884) */
      resp_sa_mad->sm_key = 0;
 

opensm/opensm/osm_sa.c void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size, · line 466

Quoted 11 lines

opensm/opensm/osm_sa.c +9 -2

-     /* Fill in the offset (paylen will be done by the rmpp SAR) */
-     resp_sa_mad->attr_offset = num_rec ? ib_get_attr_offset(attr_size) : 0;
+#ifdef DUAL_SIDED_RMPP
+     if (resp_sa_mad->method == IB_MAD_METHOD_GETTABLE_RESP ||
+         resp_sa_mad->method == IB_MAD_METHOD_GETMULTI_RESP) {
+#else
+     if (resp_sa_mad->method == IB_MAD_METHOD_GETTABLE_RESP) {
+#endif
+             /* Fill in the offset (paylen will be done by the rmpp SAR) */
+             resp_sa_mad->attr_offset = num_rec ? ib_get_attr_offset(attr_size) : 0;
+     }

What is wrong with current implementation?

Sasha

Quoted 3 lines

opensm/opensm/osm_sa.c +0 -0

      p = ib_sa_mad_get_payload_ptr(resp_sa_mad);
 

Quoted 3 lines

opensm/opensm/osm_sa.c +0 -1

--

1.5.6.4

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

Quoted 22 lines

Quoted 22 lines

Signed-off-by Hal Rosenstock <[email protected]>

---
 opensm/opensm/osm_sa.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

opensm/opensm/osm_sa.c +1 -0

index 0aca81f..8325632 100644
@@ -3,6 +3,7 @@
  * Copyright (c) 2002-2010 Mellanox Technologies LTD. All rights reserved.
  * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
  * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
+ * Copyright (c) 2010 HNR Consulting. 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

@@ -454,8 +455,15 @@ void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size,
      /* C15-0.1.5 - always return SM_Key = 0 (table 185 p 884) */
      resp_sa_mad->sm_key = 0;
 

Quoted 13 lines

opensm/opensm/osm_sa.c void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size, · line 466

Quoted 11 lines

opensm/opensm/osm_sa.c +9 -2

-     /* Fill in the offset (paylen will be done by the rmpp SAR) */
-     resp_sa_mad->attr_offset = num_rec ? ib_get_attr_offset(attr_size) : 0;
+#ifdef DUAL_SIDED_RMPP
+     if (resp_sa_mad->method == IB_MAD_METHOD_GETTABLE_RESP ||
+         resp_sa_mad->method == IB_MAD_METHOD_GETMULTI_RESP) {
+#else
+     if (resp_sa_mad->method == IB_MAD_METHOD_GETTABLE_RESP) {
+#endif
+             /* Fill in the offset (paylen will be done by the rmpp SAR) */
+             resp_sa_mad->attr_offset = num_rec ? ib_get_attr_offset(attr_size) : 0;
+     }

What is wrong with current implementation?

It's now needed for the debug version of OpenSM not to assert due to:

7fc6cd3037f07190e483a047f17d37b6bebbb2b3

Author: Smith, Stan <[email protected]>
Date:   Fri May 21 10:49:27 2010 -0700

ib_types.h add debug assert

Add a debug assert to catch incorrect MAD attr offset size. This proved to be useful in catching incorrect struct sizes as MAD attrs nee
d to be a multiple of 8 bytes.

Signed-off-by stan smith <[email protected]>

Signed-off-by Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/[email protected]>

as not all SA attributes are multiple of 8 bytes.

Quoted 48 lines · Hal Rosenstock

Quoted 22 lines

Quoted 22 lines

Signed-off-by Hal Rosenstock <[email protected]>

---
 opensm/opensm/osm_sa.c |   12 ++++++++++--

 1 files changed, 10 insertions(+), 2 deletions(-)

opensm/opensm/osm_sa.c +1 -0

index 0aca81f..8325632 100644
@@ -3,6 +3,7 @@
   * Copyright (c) 2002-2010 Mellanox Technologies LTD. All rights reserved.
   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
   * Copyright (c) 2008 Xsigo Systems Inc.  All rights reserved.
+ * Copyright (c) 2010 HNR Consulting. 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

@@ -454,8 +455,15 @@ void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size,
       /* C15-0.1.5 - always return SM_Key = 0 (table 185 p 884) */
       resp_sa_mad->sm_key = 0;
 

Quoted 13 lines

opensm/opensm/osm_sa.c void osm_sa_respond(osm_sa_t *sa, osm_madw_t *madw, size_t attr_size, · line 466

Quoted 11 lines

opensm/opensm/osm_sa.c +9 -2

-     /* Fill in the offset (paylen will be done by the rmpp SAR) */
-     resp_sa_mad->attr_offset = num_rec ? ib_get_attr_offset(attr_size) : 0;
+#ifdef DUAL_SIDED_RMPP
+     if (resp_sa_mad->method == IB_MAD_METHOD_GETTABLE_RESP ||
+         resp_sa_mad->method == IB_MAD_METHOD_GETMULTI_RESP) {
+#else
+     if (resp_sa_mad->method == IB_MAD_METHOD_GETTABLE_RESP) {
+#endif
+             /* Fill in the offset (paylen will be done by the rmpp SAR) */
+             resp_sa_mad->attr_offset = num_rec ? ib_get_attr_offset(attr_size) : 0;
+     }

What is wrong with current implementation?

It's now needed for the debug version of OpenSM not to assert due to:

7fc6cd3037f07190e483a047f17d37b6bebbb2b3
Author: Smith, Stan <[email protected]>
Date:   Fri May 21 10:49:27 2010 -0700

    ib_types.h add debug assert

    Add a debug assert to catch incorrect MAD attr offset size. This proved to be useful in catching incorrect struct sizes as MAD attrs nee d to be a multiple of 8 bytes.

Quoted 4 lines

Signed-off-by stan smith <[email protected]>

Signed-off-by Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/[email protected]>

as not all SA attributes are multiple of 8 bytes.

Then it seems that such assertion is incorrect and the patch should be reverted. Right?

Sasha

I think the use of ib_get_attr_offset is being "overused". I'd rather see this patch used as it is the simplest way to address that rather than conditionalize ib_get_attr_offset on the SA attribute or would you rather see a patch along those lines ?

Quoted 5 lines · Hal Rosenstock

I think the use of ib_get_attr_offset is being "overused". I'd rather see this patch used as it is the simplest way to address that rather than conditionalize ib_get_attr_offset on the SA attribute or would you rather see a patch along those lines ?

I just think that if the assertion is incorrect (and ib_get_attr_offset() is generic function) we need to remove this. That is simplest.

Sasha

Quoted 7 lines · Sasha Khapyorsky

Quoted 5 lines · Hal Rosenstock

I think the use of ib_get_attr_offset is being "overused". I'd rather see this patch used as it is the simplest way to address that rather than conditionalize ib_get_attr_offset on the SA attribute or would you rather see a patch along those lines ?

Quoted 2 lines

I just think that if the assertion is incorrect (and ib_get_attr_offset() is generic function) we need to remove this. That is simplest.

The downside is that it loses the ability to catch SA records which were not properly padded out.

It seems like a reasonable tradeoff to
me to not set this field on the transmit side when it's going to be ignored on receive (treat it like a reserved field for this case) and keep the ability to catch the SA record issue which has bit the community a number of times.

The only other approach I see to maintain this is to add an additional calling parameter (allribute ID) to get_attr_offset and then inside check the attribute ID and return 0 for those SA attributes.

Quoted 8 lines · Hal Rosenstock

Quoted 7 lines · Sasha Khapyorsky

Quoted 5 lines · Hal Rosenstock

I think the use of ib_get_attr_offset is being "overused". I'd rather see this patch used as it is the simplest way to address that rather than conditionalize ib_get_attr_offset on the SA attribute or would you rather see a patch along those lines ?

Quoted 5 lines

Quoted 2 lines

I just think that if the assertion is incorrect (and ib_get_attr_offset() is generic function) we need to remove this. That is simplest.

The downside is that it loses the ability to catch SA records which were not properly padded out.

It is not run-time error, so I don't think that such sort of debugging is a main function of OpenSM.

Quoted 5 lines

It seems like a reasonable tradeoff to
me to not set this field on the transmit side when it's going to be ignored on receive (treat it like a reserved field for this case) and keep the ability to catch the SA record issue which has bit the community a number of times.

This will cost in extra code.

Quoted 3 lines

The only other approach I see to maintain this is to add an additional calling parameter (allribute ID) to get_attr_offset and then inside check the attribute ID and return 0 for those SA attributes.

Which SA attributes?

Sasha

Quoted 4 lines · Hal Rosenstock

Quoted 2 lines · Sasha Khapyorsky

Quoted 1 line · Hal Rosenstock

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

I think the use of ib_get_attr_offset is being "overused". I'd rather see this patch used as it is the simplest way to address that rather than conditionalize ib_get_attr_offset on the SA attribute or would you rather see a patch along those lines ?

I just think that if the assertion is incorrect (and ib_get_attr_offset() is generic function) we need to remove this. That is simplest.

Quoted 3 lines

The downside is that it loses the ability to catch SA records which were not properly padded out.

It is not run-time error, so I don't think that such sort of debugging is a main function of OpenSM.

Quoted 5 lines

It seems like a reasonable tradeoff to
me to not set this field on the transmit side when it's going to be ignored on receive (treat it like a reserved field for this case) and keep the ability to catch the SA record issue which has bit the community a number of times.

This will cost in extra code.

Quoted 4 lines

The only other approach I see to maintain this is to add an additional calling parameter (allribute ID) to get_attr_offset and then inside
check the attribute ID and return 0 for those SA attributes.

Which SA attributes?

Sasha

<snip...>

Following discussion is tangential to Hal's patch.

Last night I came across an curious discovery.
sizeof(ib_inform_info_t)        == 36.
sizeof(ib_inform_info_record_t) == 64.

My debug version of 'osmtest -f v' trips CL_ASSERT() in ib_get_attr_offset() osmteset.c::osmtest_informinfo_request()for ib_get_attr_offset(rec), where rec is ib_inform_info_t.

The winOFED ib_types.h & OFED ib_types.h match w.r.t. definitions for ib_inform_info_t & ib_inform_info_record_t.

Question: should ib_inform_info_t be 8-bye aligned with padding in ib_inform_info_record_t adjusted or should we remove the CL_ASSERT() from ib_get_attr_offset()?

stan.

<snip...>

Following discussion is tangential to Hal's patch.

Last night I came across an curious discovery.
sizeof(ib_inform_info_t) == 36.
sizeof(ib_inform_info_record_t) == 64.

My debug version of 'osmtest -f v' trips CL_ASSERT() in ib_get_attr_offset() osmteset.c::osmtest_informinfo_request()for ib_get_attr_offset(rec), where rec is ib_inform_info_t.

The winOFED ib_types.h & OFED ib_types.h match w.r.t. definitions for ib_inform_info_t & ib_inform_info_record_t.

Question: should ib_inform_info_t be 8-byte aligned with padding in ib_inform_info_record_t adjusted or should we remove the CL_ASSERT() from ib_get_attr_offset()?

stan.

Since not all SA attributes should be multiple of 8 bytes (specifically InformInfo).