[PATCH v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[PATCH v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Hal Rosenstock]

Rather than printing "Could be FDR10", print
"Could be FDR10 (Found link at QDR but expected speed is FDR10)"
for this case.

Confusion is with words "Could be" and fact that both
QDR and FDR10 are both 10.0 Gbps.

Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
Change since v1:
Preserved original "Could be FDR10" so can grep for "Could be"
along with adding new language.

 src/ibdiag_common.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
index f74eed4..ad8551f 100644
--- a/src/ibdiag_common.c
+++ b/src/ibdiag_common.c
@@ -814,8 +814,14 @@ check_ext_speed:
 
 check_fdr10_active:
 	if ((mad_get_field(port->ext_info, 0,
-			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
-		snprintf(speed_msg, msg_size, "Could be FDR10");
+			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
+		/* Special case QDR to try to avoid confusion with FDR10 */
+		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
+			snprintf(speed_msg, msg_size,
+				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
+		else
+			snprintf(speed_msg, msg_size, "Could be FDR10");
+	}
 }
 
 int vsnprint_field(char *buf, size_t n, enum MAD_FIELDS f, int spacing,
--
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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Yuval Shaia]

On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
> 
> Rather than printing "Could be FDR10", print
> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
> for this case.
> 
> Confusion is with words "Could be" and fact that both
> QDR and FDR10 are both 10.0 Gbps.
> 
> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
> Change since v1:
> Preserved original "Could be FDR10" so can grep for "Could be"
> along with adding new language.
> 
>  src/ibdiag_common.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
> index f74eed4..ad8551f 100644
> --- a/src/ibdiag_common.c
> +++ b/src/ibdiag_common.c
> @@ -814,8 +814,14 @@ check_ext_speed:
>  
>  check_fdr10_active:
>  	if ((mad_get_field(port->ext_info, 0,
> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
> -		snprintf(speed_msg, msg_size, "Could be FDR10");
> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
> +		/* Special case QDR to try to avoid confusion with FDR10 */
> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
> +			snprintf(speed_msg, msg_size,
> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
> +		else
> +			snprintf(speed_msg, msg_size, "Could be FDR10");
> +	}

Since there is no "formatting" here can you take advantage with this change
and consider using strncpy instead of snprintf?

>  }
>  
>  int vsnprint_field(char *buf, size_t n, enum MAD_FIELDS f, int spacing,
> --
> 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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Hal Rosenstock]

On 12/20/2016 5:41 AM, Yuval Shaia wrote:
> On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
>>
>> Rather than printing "Could be FDR10", print
>> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
>> for this case.
>>
>> Confusion is with words "Could be" and fact that both
>> QDR and FDR10 are both 10.0 Gbps.
>>
>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>> Change since v1:
>> Preserved original "Could be FDR10" so can grep for "Could be"
>> along with adding new language.
>>
>>  src/ibdiag_common.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
>> index f74eed4..ad8551f 100644
>> --- a/src/ibdiag_common.c
>> +++ b/src/ibdiag_common.c
>> @@ -814,8 +814,14 @@ check_ext_speed:
>>  
>>  check_fdr10_active:
>>  	if ((mad_get_field(port->ext_info, 0,
>> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
>> -		snprintf(speed_msg, msg_size, "Could be FDR10");
>> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
>> +		/* Special case QDR to try to avoid confusion with FDR10 */
>> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
>> +			snprintf(speed_msg, msg_size,
>> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
>> +		else
>> +			snprintf(speed_msg, msg_size, "Could be FDR10");
>> +	}
> 
> Since there is no "formatting" here can you take advantage with this change
> and consider using strncpy instead of snprintf?


This patch was merely changing the messages.

Sure; strncpy rather than snprintf could be used in these cases. That
could be done in a separate patch which would change all such
occurrences and not just here.

>>  }
>>  
>>  int vsnprint_field(char *buf, size_t n, enum MAD_FIELDS f, int spacing,
>> --
>> 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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Yuval Shaia]

On Tue, Dec 20, 2016 at 07:31:59AM -0500, Hal Rosenstock wrote:
> On 12/20/2016 5:41 AM, Yuval Shaia wrote:
> > On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
> >>
> >> Rather than printing "Could be FDR10", print
> >> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
> >> for this case.
> >>
> >> Confusion is with words "Could be" and fact that both
> >> QDR and FDR10 are both 10.0 Gbps.
> >>
> >> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> ---
> >> Change since v1:
> >> Preserved original "Could be FDR10" so can grep for "Could be"
> >> along with adding new language.
> >>
> >>  src/ibdiag_common.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
> >> index f74eed4..ad8551f 100644
> >> --- a/src/ibdiag_common.c
> >> +++ b/src/ibdiag_common.c
> >> @@ -814,8 +814,14 @@ check_ext_speed:
> >>  
> >>  check_fdr10_active:
> >>  	if ((mad_get_field(port->ext_info, 0,
> >> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
> >> -		snprintf(speed_msg, msg_size, "Could be FDR10");
> >> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
> >> +		/* Special case QDR to try to avoid confusion with FDR10 */
> >> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)

Should this "4" be #define?

> >> +			snprintf(speed_msg, msg_size,
> >> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
> >> +		else
> >> +			snprintf(speed_msg, msg_size, "Could be FDR10");
> >> +	}
--
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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Hal Rosenstock]

On 12/20/2016 9:42 AM, Yuval Shaia wrote:
> On Tue, Dec 20, 2016 at 07:31:59AM -0500, Hal Rosenstock wrote:
>> On 12/20/2016 5:41 AM, Yuval Shaia wrote:
>>> On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
>>>>
>>>> Rather than printing "Could be FDR10", print
>>>> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
>>>> for this case.
>>>>
>>>> Confusion is with words "Could be" and fact that both
>>>> QDR and FDR10 are both 10.0 Gbps.
>>>>
>>>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>> Change since v1:
>>>> Preserved original "Could be FDR10" so can grep for "Could be"
>>>> along with adding new language.
>>>>
>>>>  src/ibdiag_common.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
>>>> index f74eed4..ad8551f 100644
>>>> --- a/src/ibdiag_common.c
>>>> +++ b/src/ibdiag_common.c
>>>> @@ -814,8 +814,14 @@ check_ext_speed:
>>>>  
>>>>  check_fdr10_active:
>>>>  	if ((mad_get_field(port->ext_info, 0,
>>>> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
>>>> -		snprintf(speed_msg, msg_size, "Could be FDR10");
>>>> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
>>>> +		/* Special case QDR to try to avoid confusion with FDR10 */
>>>> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
> 
> Should this "4" be #define?

I think that such defines belong in the MAD libraries/headers but
unfortunately are missing.

>>>> +			snprintf(speed_msg, msg_size,
>>>> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
>>>> +		else
>>>> +			snprintf(speed_msg, msg_size, "Could be FDR10");
>>>> +	}
> 
--
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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Yuval Shaia]

On Tue, Dec 20, 2016 at 10:01:46AM -0500, Hal Rosenstock wrote:
> On 12/20/2016 9:42 AM, Yuval Shaia wrote:
> > On Tue, Dec 20, 2016 at 07:31:59AM -0500, Hal Rosenstock wrote:
> >> On 12/20/2016 5:41 AM, Yuval Shaia wrote:
> >>> On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
> >>>>
> >>>> Rather than printing "Could be FDR10", print
> >>>> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
> >>>> for this case.
> >>>>
> >>>> Confusion is with words "Could be" and fact that both
> >>>> QDR and FDR10 are both 10.0 Gbps.
> >>>>
> >>>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >>>> ---
> >>>> Change since v1:
> >>>> Preserved original "Could be FDR10" so can grep for "Could be"
> >>>> along with adding new language.
> >>>>
> >>>>  src/ibdiag_common.c | 10 ++++++++--
> >>>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
> >>>> index f74eed4..ad8551f 100644
> >>>> --- a/src/ibdiag_common.c
> >>>> +++ b/src/ibdiag_common.c
> >>>> @@ -814,8 +814,14 @@ check_ext_speed:
> >>>>  
> >>>>  check_fdr10_active:
> >>>>  	if ((mad_get_field(port->ext_info, 0,
> >>>> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
> >>>> -		snprintf(speed_msg, msg_size, "Could be FDR10");
> >>>> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
> >>>> +		/* Special case QDR to try to avoid confusion with FDR10 */
> >>>> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
> > 
> > Should this "4" be #define?
> 
> I think that such defines belong in the MAD libraries/headers but
> unfortunately are missing.

Suggesting to add it as (at least for me) 4 does not tell anything.

> 
> >>>> +			snprintf(speed_msg, msg_size,
> >>>> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
> >>>> +		else
> >>>> +			snprintf(speed_msg, msg_size, "Could be FDR10");
> >>>> +	}
> > 
> --
> 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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Hal Rosenstock]

On 12/20/2016 10:04 AM, Yuval Shaia wrote:
> On Tue, Dec 20, 2016 at 10:01:46AM -0500, Hal Rosenstock wrote:
>> On 12/20/2016 9:42 AM, Yuval Shaia wrote:
>>> On Tue, Dec 20, 2016 at 07:31:59AM -0500, Hal Rosenstock wrote:
>>>> On 12/20/2016 5:41 AM, Yuval Shaia wrote:
>>>>> On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
>>>>>>
>>>>>> Rather than printing "Could be FDR10", print
>>>>>> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
>>>>>> for this case.
>>>>>>
>>>>>> Confusion is with words "Could be" and fact that both
>>>>>> QDR and FDR10 are both 10.0 Gbps.
>>>>>>
>>>>>> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>>>> ---
>>>>>> Change since v1:
>>>>>> Preserved original "Could be FDR10" so can grep for "Could be"
>>>>>> along with adding new language.
>>>>>>
>>>>>>  src/ibdiag_common.c | 10 ++++++++--
>>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
>>>>>> index f74eed4..ad8551f 100644
>>>>>> --- a/src/ibdiag_common.c
>>>>>> +++ b/src/ibdiag_common.c
>>>>>> @@ -814,8 +814,14 @@ check_ext_speed:
>>>>>>  
>>>>>>  check_fdr10_active:
>>>>>>  	if ((mad_get_field(port->ext_info, 0,
>>>>>> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
>>>>>> -		snprintf(speed_msg, msg_size, "Could be FDR10");
>>>>>> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
>>>>>> +		/* Special case QDR to try to avoid confusion with FDR10 */
>>>>>> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
>>>
>>> Should this "4" be #define?
>>
>> I think that such defines belong in the MAD libraries/headers but
>> unfortunately are missing.
> 
> Suggesting to add it as (at least for me) 4 does not tell anything.

I'll add comment to indicate that this means Active.

Fixing the generic issue is a larger issue I don't have the time to tackle.

>>
>>>>>> +			snprintf(speed_msg, msg_size,
>>>>>> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
>>>>>> +		else
>>>>>> +			snprintf(speed_msg, msg_size, "Could be FDR10");
>>>>>> +	}
>>>
>> --
>> 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 v2 infiniband-diags] ibdiag_common.c: Disambiguate link speed when QDR and could be FDR10

[Yuval Shaia]

On Tue, Dec 20, 2016 at 07:31:59AM -0500, Hal Rosenstock wrote:
> On 12/20/2016 5:41 AM, Yuval Shaia wrote:
> > On Mon, Dec 19, 2016 at 05:25:11PM -0500, Hal Rosenstock wrote:
> >>
> >> Rather than printing "Could be FDR10", print
> >> "Could be FDR10 (Found link at QDR but expected speed is FDR10)"
> >> for this case.
> >>
> >> Confusion is with words "Could be" and fact that both
> >> QDR and FDR10 are both 10.0 Gbps.
> >>
> >> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> ---
> >> Change since v1:
> >> Preserved original "Could be FDR10" so can grep for "Could be"
> >> along with adding new language.
> >>
> >>  src/ibdiag_common.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
> >> index f74eed4..ad8551f 100644
> >> --- a/src/ibdiag_common.c
> >> +++ b/src/ibdiag_common.c
> >> @@ -814,8 +814,14 @@ check_ext_speed:
> >>  
> >>  check_fdr10_active:
> >>  	if ((mad_get_field(port->ext_info, 0,
> >> -			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0)
> >> -		snprintf(speed_msg, msg_size, "Could be FDR10");
> >> +			   IB_MLNX_EXT_PORT_LINK_SPEED_ACTIVE_F) & FDR10) == 0) {
> >> +		/* Special case QDR to try to avoid confusion with FDR10 */
> >> +		if (mad_get_field(port->info, 0, IB_PORT_LINK_SPEED_ACTIVE_F) == 4)
> >> +			snprintf(speed_msg, msg_size,
> >> +				 "Could be FDR10 (Found link at QDR but expected speed is FDR10)");
> >> +		else
> >> +			snprintf(speed_msg, msg_size, "Could be FDR10");
> >> +	}
> > 
> > Since there is no "formatting" here can you take advantage with this change
> > and consider using strncpy instead of snprintf?
> 
> 
> This patch was merely changing the messages.
> 
> Sure; strncpy rather than snprintf could be used in these cases. That
> could be done in a separate patch which would change all such
> occurrences and not just here.

Hope it'll not be forgotten :)

Still suggesting "while we are there"...

> 
> >>  }
> >>  
> >>  int vsnprint_field(char *buf, size_t n, enum MAD_FIELDS f, int spacing,
> >> --
> >> 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
--
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