Re: [PATCH 04/24] IB/qib: add thresholds to VendorPortCounters PMA operation

[Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>]

 > >   	p->class_version = 1;
 > >   	p->cap_mask = IB_PMA_CLASS_CAP_EXT_WIDTH;
 > >   	/*
 > > -	 * Set the most significant bit of CM2 to indicate support for
 > > +	 * Set the most significant two bits of CM2 to indicate support for
 > >   	 * congestion statistics
 > >   	 */
 > 
 > Where in the IBA spec do these bits come from ? What are they ?
 > 
 > > -	p->reserved[0] = dd->psxmitwait_supported<<  7;
 > > +	p->reserved[0] = (dd->psxmitwait_supported * 3)<<  6;
 > 
 > It looks weird to me to set an IBA reserved field. Should the
 > structure be updated to contain the real field name ?

The reserved field is (I think) actually the IBA "CapabilityMask2" bits
in ClassPortInfo; I agree that it would be better to update the
structure to name this correctly.

However, both the old and the new code look very fishy to me: bit 2 in
CapabilityMask is never set, which is the generic bit that says "If 1,
the management class agent implements additional class-specific
capabilities (CapabilityMask2)" so AFAICT, nothing should ever look in
CapabilityMask2 anyway.  

It seems that the original code was trying to set
ClassPortInfo.PortCountersXmitWaitSupported (bit 12), maybe.  I can't
fathom what the new code is doing.  And I can't find anything in the
spec that suggests that CM2 in this PMA ClassPortInfo should ever be used.

Mike, can you clarify this patch?  I'll hold off on applying this for now.

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