Re: [PATCH infiniband-diags] ibstat.c: If port is 1x SDR, rate is 2.5 rather than 2 Gbps

["ira.weiny" <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Wed, Feb 08, 2017 at 03:47:47PM -0500, Hal Rosenstock wrote:
> On 2/8/2017 3:25 PM, ira.weiny wrote:
> > On Mon, Feb 06, 2017 at 08:03:12AM -0500, Hal Rosenstock wrote:
> >> From: Oded Nissan <odedni-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >>
> >> Signed-off-by: Oded Nissan <odedni-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Hal Rosenstock <hal-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > 
> > Shouldn't this be fixed at a lower level?
> 
> That requires changing the libibumad ABI as rate is an int rather than
> float in umad_port and I didn't think it was worth it for this trivial
> issue. sysfs uses a * 10 approach and then rate / 10, rate % 10 ? ".5" :
> "" but that can't be used here without changing many more things.
> 
> > I think the problem is that libibumad is masking the information.  AFAICT the
> > kernel reports "2.5" in sysfs.
> > 
> > I'm concerned that there are probably lots of other places in the software
> > stack which have this bug because libibumad is masking the rate.
> 
> I don't think there are any places in open sources.
> 
> > If the value of "2" was at least documented to be special and meant "2.5" I
> > think this could be an ok patch...
> > 
> > Something like 
> > 
> > else if (port->rate == UMAD_2_5_RATE)
> > 	printf("%sRate: 2.5\n", pre);
> > else
> > 	printf("%sRate: %d\n", pre, port->rate);
> 
> Is it worth it to tie infiniband-diags to some version of libibumad
> where UMAD_2_5_RATE is defined like this ?

Fair enough...  And who uses SDR anymore anyway...  ;-)

But if we ever do get a fractional rate again I'd like to see this handled
better.

So I've taken the patch as we are unlikely to get a fractional rate at this
point.

Ira

> 
> -- Hal
> 
> > Ira
> > 
> >> ---
> >> diff --git a/src/ibstat.c b/src/ibstat.c
> >> index 37f2361..2d980c4 100644
> >> --- a/src/ibstat.c
> >> +++ b/src/ibstat.c
> >> @@ -182,8 +182,10 @@ static int port_dump(umad_port_t * port, int alone)
> >>  	       7 ? port_phy_state_str[port->phys_state] : "???");
> >>  	if (is_fdr10(port))
> >>  		printf("%sRate: %d (FDR10)\n", pre, port->rate);
> >> -	else
> >> +	else if (port->rate != 2)	/* 1x SDR */
> >>  		printf("%sRate: %d\n", pre, port->rate);
> >> +	else
> >> +		printf("%sRate: 2.5\n", pre);
> >>  	printf("%sBase lid: %d\n", pre, port->base_lid);
> >>  	printf("%sLMC: %d\n", pre, port->lmc);
> >>  	printf("%sSM lid: %d\n", pre, port->sm_lid);
> > 
--
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