diff mbox series

[opensm,6/6] opensm/osm_resp.c: Fix an error in swapping directed route source and destination lid

Message ID 20181121065811.21806-6-honli@redhat.com (mailing list archive)
State Not Applicable
Headers show
Series [opensm,1/6] opensm/osm_ucast_cache.h: Improve coding style and comments | expand

Commit Message

Honggang LI Nov. 21, 2018, 6:58 a.m. UTC 
From: Honggang Li <honli@redhat.com>

Signed-off-by: Honggang Li <honli@redhat.com>
---
 opensm/osm_resp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Hal Rosenstock Nov. 21, 2018, 2:01 p.m. UTC | #1 
On 11/21/2018 1:58 AM, Honggang LI wrote:
> From: Honggang Li <honli@redhat.com>
> 
> Signed-off-by: Honggang Li <honli@redhat.com>
> ---
>  opensm/osm_resp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/opensm/osm_resp.c b/opensm/osm_resp.c
> index 3f270e66..42015564 100644
> --- a/opensm/osm_resp.c
> +++ b/opensm/osm_resp.c
> @@ -85,8 +85,8 @@ static void resp_make_resp_smp(IN osm_sm_t * sm, IN const ib_smp_t * p_src_smp,
>  	if (p_src_smp->mgmt_class == IB_MCLASS_SUBN_DIR)
>  		p_dest_smp->status |= IB_SMP_DIRECTION;
>  
> -	p_dest_smp->dr_dlid = p_dest_smp->dr_slid;
> -	p_dest_smp->dr_slid = p_dest_smp->dr_dlid;
> +	p_dest_smp->dr_dlid = p_src_smp->dr_slid;
> +	p_dest_smp->dr_slid = p_src_smp->dr_dlid;

Please elaborate on why you think that this change is needed. Is it just
by code inspection where a number of dest SMP parameters are updated
from their src equivalents ?

Earlier in this code block, the src SMP is copied to the dest SMP so
that flipping the dest DR LIDs seems just fine to me and has worked for
more than the last 10-15 years.

>  	memcpy(&p_dest_smp->data, p_payload, IB_SMP_DATA_SIZE);
>  
>  Exit:
>
Honggang LI Nov. 22, 2018, 1:50 a.m. UTC | #2 
On Wed, Nov 21, 2018 at 09:01:16AM -0500, Hal Rosenstock wrote:
> On 11/21/2018 1:58 AM, Honggang LI wrote:
> > From: Honggang Li <honli@redhat.com>
> > 
> > Signed-off-by: Honggang Li <honli@redhat.com>
> > ---
> >  opensm/osm_resp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/opensm/osm_resp.c b/opensm/osm_resp.c
> > index 3f270e66..42015564 100644
> > --- a/opensm/osm_resp.c
> > +++ b/opensm/osm_resp.c
> > @@ -85,8 +85,8 @@ static void resp_make_resp_smp(IN osm_sm_t * sm, IN const ib_smp_t * p_src_smp,
> >  	if (p_src_smp->mgmt_class == IB_MCLASS_SUBN_DIR)
> >  		p_dest_smp->status |= IB_SMP_DIRECTION;
> >  
> > -	p_dest_smp->dr_dlid = p_dest_smp->dr_slid;
> > -	p_dest_smp->dr_slid = p_dest_smp->dr_dlid;
> > +	p_dest_smp->dr_dlid = p_src_smp->dr_slid;
> > +	p_dest_smp->dr_slid = p_src_smp->dr_dlid;
> 
> Please elaborate on why you think that this change is needed. Is it just
> by code inspection where a number of dest SMP parameters are updated
> from their src equivalents ?

yes, just by code inspection.

> 
> Earlier in this code block, the src SMP is copied to the dest SMP so
> that flipping the dest DR LIDs seems just fine to me and has worked for
> more than the last 10-15 years.

Yes, flipping the dest DR LIDs works. But flipping the src DR LIDs does
not.

Please let's take random LIDs for illustration.

p_src_smp->dr_slid = 1
p_src_smp->dr_dlid = 2



 60 static void resp_make_resp_smp(IN osm_sm_t * sm, IN const ib_smp_t * p_src_smp,
 .....
 70
 71         *p_dest_smp = *p_src_smp;
 72         if (p_src_smp->method == IB_MAD_METHOD_GET ||

Line 71 will copy the dest SMP to src SMP, which means:

p_dest_smp->dr_slid = p_src_smp->dr_slid = 1;
p_dest_smp->dr_dlid = p_src_smp->dr_dlid = 2;

 88         p_dest_smp->dr_dlid = p_dest_smp->dr_slid;

line 88 will set p_dest_smp->dr_dlid to 1

 89         p_dest_smp->dr_slid = p_dest_smp->dr_dlid;

line 89 will set p_dest_smp->dr_slid to 1.

Both p_dest_smp->dr_slid and p_dest_smp->dr_dlid will be set to same
value 1.

Line 88 and 89 is really just like:
A = B
B = A

The second line (B = A) is redundant, if you just want to flip the dest
DR LID.


> 
> >  	memcpy(&p_dest_smp->data, p_payload, IB_SMP_DATA_SIZE);
> >  
> >  Exit:
> >
Hal Rosenstock Nov. 26, 2018, 1:26 p.m. UTC | #3 
On 11/21/2018 8:50 PM, Honggang LI wrote:
> On Wed, Nov 21, 2018 at 09:01:16AM -0500, Hal Rosenstock wrote:
>> On 11/21/2018 1:58 AM, Honggang LI wrote:
>>> From: Honggang Li <honli@redhat.com>
>>>
>>> Signed-off-by: Honggang Li <honli@redhat.com>
>>> ---
>>>  opensm/osm_resp.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/opensm/osm_resp.c b/opensm/osm_resp.c
>>> index 3f270e66..42015564 100644
>>> --- a/opensm/osm_resp.c
>>> +++ b/opensm/osm_resp.c
>>> @@ -85,8 +85,8 @@ static void resp_make_resp_smp(IN osm_sm_t * sm, IN const ib_smp_t * p_src_smp,
>>>  	if (p_src_smp->mgmt_class == IB_MCLASS_SUBN_DIR)
>>>  		p_dest_smp->status |= IB_SMP_DIRECTION;
>>>  
>>> -	p_dest_smp->dr_dlid = p_dest_smp->dr_slid;
>>> -	p_dest_smp->dr_slid = p_dest_smp->dr_dlid;
>>> +	p_dest_smp->dr_dlid = p_src_smp->dr_slid;
>>> +	p_dest_smp->dr_slid = p_src_smp->dr_dlid;
>>
>> Please elaborate on why you think that this change is needed. Is it just
>> by code inspection where a number of dest SMP parameters are updated
>> from their src equivalents ?
> 
> yes, just by code inspection.
> 
>>
>> Earlier in this code block, the src SMP is copied to the dest SMP so
>> that flipping the dest DR LIDs seems just fine to me and has worked for
>> more than the last 10-15 years.
> 
> Yes, flipping the dest DR LIDs works. But flipping the src DR LIDs does
> not.
> 
> Please let's take random LIDs for illustration.
> 
> p_src_smp->dr_slid = 1
> p_src_smp->dr_dlid = 2
> 
> 
> 
>  60 static void resp_make_resp_smp(IN osm_sm_t * sm, IN const ib_smp_t * p_src_smp,
>  .....
>  70
>  71         *p_dest_smp = *p_src_smp;
>  72         if (p_src_smp->method == IB_MAD_METHOD_GET ||
> 
> Line 71 will copy the dest SMP to src SMP, which means:
> 
> p_dest_smp->dr_slid = p_src_smp->dr_slid = 1;
> p_dest_smp->dr_dlid = p_src_smp->dr_dlid = 2;
> 
>  88         p_dest_smp->dr_dlid = p_dest_smp->dr_slid;
> 
> line 88 will set p_dest_smp->dr_dlid to 1
> 
>  89         p_dest_smp->dr_slid = p_dest_smp->dr_dlid;
> 
> line 89 will set p_dest_smp->dr_slid to 1.
> 
> Both p_dest_smp->dr_slid and p_dest_smp->dr_dlid will be set to same
> value 1.
> 
> Line 88 and 89 is really just like:
> A = B
> B = A
> 
> The second line (B = A) is redundant, if you just want to flip the dest
> DR LID.

I see what you mean. In looking at this further, per IBA vol 1
C14-10.1.1, there is no need to swap DrDLID and DrSLID in the response.
Patch to follow shortly.

> 
>>
>>>  	memcpy(&p_dest_smp->data, p_payload, IB_SMP_DATA_SIZE);
>>>  
>>>  Exit:
>>>
>
diff mbox series

Patch

diff --git a/opensm/osm_resp.c b/opensm/osm_resp.c
index 3f270e66..42015564 100644
--- a/opensm/osm_resp.c
+++ b/opensm/osm_resp.c
@@ -85,8 +85,8 @@  static void resp_make_resp_smp(IN osm_sm_t * sm, IN const ib_smp_t * p_src_smp,
 	if (p_src_smp->mgmt_class == IB_MCLASS_SUBN_DIR)
 		p_dest_smp->status |= IB_SMP_DIRECTION;
 
-	p_dest_smp->dr_dlid = p_dest_smp->dr_slid;
-	p_dest_smp->dr_slid = p_dest_smp->dr_dlid;
+	p_dest_smp->dr_dlid = p_src_smp->dr_slid;
+	p_dest_smp->dr_slid = p_src_smp->dr_dlid;
 	memcpy(&p_dest_smp->data, p_payload, IB_SMP_DATA_SIZE);
 
 Exit: