RE: [PATCH 16/20] IB/mad: Add Intel Omni-Path Architecture defines

Weiny, Ira
2015-01-20T18:59:45Z

>
> On Mon, Jan 19, 2015 at 10:50:38PM +0000, Weiny, Ira wrote:
> > >
> > > On Mon, Jan 19, 2015 at 07:57:22PM +0000, Weiny, Ira wrote:
> > >
> > > > Perhaps you can give us an example of where the current code would
> > > > work without modifications if the IBTA were to define any new base
> > > > or smp class version?
> > >
> > > I think the point here is that every check for OPA_MIN_CLASS_VERSION
> > > must also be combined with a check if the device in question is OPA or not.
> >
> > This contradicts what you say below....
>
> How so?
>
> if (mad_reg_req->mgmt_class_version >= OPA_MIN_CLASS_VERSION
> && !port_priv->qp_info[qpn].supports_jumbo_mads) {
>

Sorry, I got a bit confused because this is not the latest version of the patch.

> Should be:
>
> if (port_is_opa && mad_reg_req->mgmt_class_version >=
> OPA_MIN_CLASS_VERSION
> && !port_priv->qp_info[qpn].supports_jumbo_mads) {
>
> And since port_is_opa = true implies
> port_priv->qp_info[qpn].supports_jumbo_mads = true the above if can never
> evaluate to true and can just be removed.

Right there is no need to check for supports_jumbo_mads. The latest version was:

+ if (mad_reg_req->mgmt_class_version >= OPA_MIN_CLASS_VERSION &&
+ !(port_priv->device->attributes.device_cap_flags2 & IB_DEVICE_OPA_MAD_SUPPORT)) {

The "bug" in the patch is that any class version should be allowed on IB devices. This check prevented class versions >= 0x80 for IB devices.

NOTE however that the current code already limits users to a class version < 8. (due to the implementation.)

-#define MAX_MGMT_VERSION 8
+#define MAX_MGMT_VERSION 0x83
...
if (mad_reg_req->mgmt_class_version >= MAX_MGMT_VERSION) {
dev_notice(&device->dev,
"ib_register_mad_agent: invalid Class Version %u\n",
mad_reg_req->mgmt_class_version);
goto error1;
}

Therefore I did not see this as limiting the IB implementation we already have.

I will remove the patch as I agree with Jason and Hal it is technically more correct.

Ira