| Message ID | 7230ef46-74ff-76cd-4123-f34a75a6d436@dev.mellanox.co.il (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Ira Weiny |
| Headers | show |
On Mon, Feb 06, 2017 at 08:03:12AM -0500, Hal Rosenstock wrote: > From: Oded Nissan <odedni@mellanox.com> > > Signed-off-by: Oded Nissan <odedni@mellanox.com> > Signed-off-by: Hal Rosenstock <hal@mellanox.com> Shouldn't this be fixed at a lower level? 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. 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); 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@mellanox.com> >> >> Signed-off-by: Oded Nissan <odedni@mellanox.com> >> Signed-off-by: Hal Rosenstock <hal@mellanox.com> > > 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 ? -- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@mellanox.com> > >> > >> Signed-off-by: Oded Nissan <odedni@mellanox.com> > >> Signed-off-by: Hal Rosenstock <hal@mellanox.com> > > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);