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

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

[Hal Rosenstock]

Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 opensm/opensm/osm_sa.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/opensm/opensm/osm_sa.c b/opensm/opensm/osm_sa.c
index 0aca81f..8325632 100644
--- a/opensm/opensm/osm_sa.c
+++ b/opensm/opensm/osm_sa.c
@@ -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) */
-	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;
+	}
 
 	p = ib_sa_mad_get_payload_ptr(resp_sa_mad);
 
-- 
1.5.6.4

--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Sasha Khapyorsky]

Hi Hal,

On 09:42 Thu 03 Jun     , Hal Rosenstock wrote:
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  opensm/opensm/osm_sa.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/opensm/opensm/osm_sa.c b/opensm/opensm/osm_sa.c
> index 0aca81f..8325632 100644
> --- a/opensm/opensm/osm_sa.c
> +++ b/opensm/opensm/osm_sa.c
> @@ -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) */
> -	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

>  
>  	p = ib_sa_mad_get_payload_ptr(resp_sa_mad);
>  
> -- 
> 1.5.6.4
> 
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Hal Rosenstock]

Hi Sasha,

On Wed, Jun 9, 2010 at 1:43 AM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> Hi Hal,
>
> On 09:42 Thu 03 Jun     , Hal Rosenstock wrote:
>>
>> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  opensm/opensm/osm_sa.c |   12 ++++++++++--
>>  1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/opensm/opensm/osm_sa.c b/opensm/opensm/osm_sa.c
>> index 0aca81f..8325632 100644
>> --- a/opensm/opensm/osm_sa.c
>> +++ b/opensm/opensm/osm_sa.c
>> @@ -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) */
>> -     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 <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
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 <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
    Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>

as not all SA attributes are multiple of 8 bytes.

-- Hal

> Sasha
>
>>
>>       p = ib_sa_mad_get_payload_ptr(resp_sa_mad);
>>
>> --
>> 1.5.6.4
>>
> --
> 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
>
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Sasha Khapyorsky]

On 06:23 Wed 09 Jun     , Hal Rosenstock wrote:
> >>
> >> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  opensm/opensm/osm_sa.c |   12 ++++++++++--
> >>  1 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/opensm/opensm/osm_sa.c b/opensm/opensm/osm_sa.c
> >> index 0aca81f..8325632 100644
> >> --- a/opensm/opensm/osm_sa.c
> >> +++ b/opensm/opensm/osm_sa.c
> >> @@ -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) */
> >> -     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 <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 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 <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
> 
> 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
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Hal Rosenstock]

On Wed, Jun 9, 2010 at 8:10 AM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> On 06:23 Wed 09 Jun     , Hal Rosenstock wrote:
>> >>
>> >> Signed-off-by: Hal Rosenstock <hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  opensm/opensm/osm_sa.c |   12 ++++++++++--
>> >>  1 files changed, 10 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/opensm/opensm/osm_sa.c b/opensm/opensm/osm_sa.c
>> >> index 0aca81f..8325632 100644
>> >> --- a/opensm/opensm/osm_sa.c
>> >> +++ b/opensm/opensm/osm_sa.c
>> >> @@ -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) */
>> >> -     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 <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> 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 <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>     Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
>>
>> 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?

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 ?

-- Hal

>
> Sasha
>
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Sasha Khapyorsky]

On 08:40 Wed 09 Jun     , Hal Rosenstock wrote:
> 
> 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
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Hal Rosenstock]

On Wed, Jun 9, 2010 at 8:48 AM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> On 08:40 Wed 09 Jun     , Hal Rosenstock wrote:
>>
>> 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.

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.

-- Hal

> Sasha
>
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Sasha Khapyorsky]

On 11:27 Wed 09 Jun     , Hal Rosenstock wrote:
> On Wed, Jun 9, 2010 at 8:48 AM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> > On 08:40 Wed 09 Jun     , Hal Rosenstock wrote:
> >>
> >> 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.
> 
> 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.

> 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.

> 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
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Hal Rosenstock]

On Wed, Jun 9, 2010 at 1:15 PM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> On 11:27 Wed 09 Jun     , Hal Rosenstock wrote:
>> On Wed, Jun 9, 2010 at 8:48 AM, Sasha Khapyorsky <sashak-smomgflXvObQFizaE/u3fw@public.gmane.orgm> wrote:
>> > On 08:40 Wed 09 Jun     , Hal Rosenstock wrote:
>> >>
>> >> 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.
>>
>> 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.
>
>> 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.

Sure; that's the tradeoff. I doubt the code size difference is significant.

>> 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?

Any of the non record ones but AFAIT the only one of those going
through this code is InformInfo.

-- Hal

> Sasha
>
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Smith, Stan]

Sasha Khapyorsky wrote:
> On 11:27 Wed 09 Jun     , Hal Rosenstock wrote:
>> On Wed, Jun 9, 2010 at 8:48 AM, Sasha Khapyorsky
>> <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
>>> On 08:40 Wed 09 Jun     , Hal Rosenstock wrote:
>>>>
>>>> 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.


The ASSERT() in ib_get_attr_offset() is only valid for the debug case; no runtime impact for the 'normal' build.


>>
>> 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.
>
>> 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.
>
>> 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

--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Smith, 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-bye aligned with padding in ib_inform_info_record_t adjusted or should we remove the CL_ASSERT() from ib_get_attr_offset()?

stan.
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Smith, Stan]

Smith, Stan wrote:
> <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.

According to the IB spec, ib_inform_info_t is not 8-byte aligned, so removing the CL_ASSERT() makes sense.
Sorry about extra work incurred, I thought I had tested all cases.

stan.
--
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] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method

[Hal Rosenstock]

On Fri, Jun 11, 2010 at 12:02 PM, Smith, Stan <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> <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.

Those match the IBA spec (InformInfoRecord being padded whereas
InformInfo is not).

> 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()?

Why does osmtest need to call this routine ? I think most if not all
of those calls should be removed. Attribute offset only needs setting
when RMPP and most SA client invocations of things like Get and
GetTable are not RMPP (response is). I know osmtest does some negative
testing of SA like invalid methods for a given attribute.

-- Hal

>
> stan.
>
--
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] iba/ib_types.h: remove assertion in ib_get_attr_offset()

[Sasha Khapyorsky]

This reverts the commit:

commit 7fc6cd3037f07190e483a047f17d37b6bebbb2b3
Author: Smith, Stan <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
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 need to be a multiple of 8 bytes.

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

The issue was discovered by Hal Rosenstock.

Signed-off-by: Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>
---
 opensm/include/iba/ib_types.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
index 203c319..e1bc102 100644
--- a/opensm/include/iba/ib_types.h
+++ b/opensm/include/iba/ib_types.h
@@ -4395,7 +4395,6 @@ static inline uint32_t OSM_API ib_get_attr_size(IN const ib_net16_t attr_offset)
 
 static inline ib_net16_t OSM_API ib_get_attr_offset(IN const uint32_t attr_size)
 {
-	CL_ASSERT((attr_size & 0x07) == 0);
 	return (cl_hton16((uint16_t) (attr_size >> 3)));
 }
 
-- 
1.7.0.4

--
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] iba/ib_types.h: remove assertion in ib_get_attr_offset()

[Hefty, Sean]

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

I thought the issue was that the C-struct definition was required to be a multiple of 8-bytes, not the actual SA attribute definition, ..

>  static inline ib_net16_t OSM_API ib_get_attr_offset(IN const uint32_t
> attr_size)
>  {
> -	CL_ASSERT((attr_size & 0x07) == 0);
>  	return (cl_hton16((uint16_t) (attr_size >> 3)));
>  }

which causes this to fail, since it lops off the lower bits of the size, rather than rounding up.

I would either expect the assert to always work or the code to round up properly.

- Sean
--
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