[ofa-general] [PATCH] infiniband/core: Enable loopback of DR SMP responses from userspace

[Steve Welch]

Hi Sean,

> -----Original Message-----
> From: Sean Hefty [mailto:mshefty at ichips.intel.com]
> Sent: Tuesday, September 04, 2007 7:55 PM
> To: swelch at systemfabricworks.com
> Cc: general at lists.openfabrics.org; sean.hefty at intel.com
> Subject: Re: [ofa-general] [PATCH] infiniband/core: Enable loopback of DR
> SMP responses from userspace
> 
> > @@ -754,6 +755,7 @@ static int handle_outgoing_dr_smp(struct
> ib_mad_agent_private *mad_agent_priv,
> >  		if (port_priv) {
> >  			mad_priv->mad.mad.mad_hdr.tid =
> >  				((struct ib_mad *)smp)->mad_hdr.tid;
> > +			memcpy(&mad_priv->mad.mad, smp, sizeof(struct
ib_mad));
> 
> I'm having a hard time understanding the impact of this change.  If I'm
> reading the code correctly, mad_priv->mad should contain the response
> from the device process_mad() routine.  This changes that response.  Can
> you provide more details describing the effect this change has on the
> existing behavior?

The new code is executed when the device specific process_mad function
returns only IB_MAD_RESULT_SUCCESS status in the status bitmask.  Since
IB_MAD_RESULT_REPLY is not also set; the device is indicating it did
not create a response and mad_priv->mad should be as it was before the
process_mad call (i.e. not initialized with a response).  Since the
IB_MAD_RESULT_CONSUMED status was not set in the status bitmask, the
original MAD is still needing delivery and by definition goes to
the local node. 

Prior to this patch and the addition of the of the
smi_check_local_resp_smp() test, the only DR SMP that could have made
it to the device's process_mad call would have been a DR SMP Request
that was targeted to the local SMA.  It is possible that the device's
SMA would not handle the DR SMP Request and that it would return only
IB_MAD_RESULT_SUCCESS; however in that case the find_mad_agent() call
would still access the un-initialized mad_priv->mad.mad.  For
this reason I do not believe this code path was previously executed,
and I believe there will be no effect on the existing behavior.

Running with these changes, the IB utilities built on top of DR SMP's
continue to operate on the host, going both to the local SMA and out
on the fabric to an SMA.

> 
> Also, I think we can eliminate setting the tid, since the memcpy will
> set that as well.
Yes, I agree.

> 
> >  			recv_mad_agent = find_mad_agent(port_priv,
> >  						        &mad_priv->mad.mad);
> >  		}
> > diff --git a/drivers/infiniband/core/smi.h
> b/drivers/infiniband/core/smi.h
> > index 1cfc298..d96fc8e 100644
> > --- a/drivers/infiniband/core/smi.h
> > +++ b/drivers/infiniband/core/smi.h
> > @@ -71,4 +71,18 @@ static inline enum smi_action
> smi_check_local_smp(struct ib_smp *smp,
> >  		(smp->hop_ptr == smp->hop_cnt + 1)) ?
> >  		IB_SMI_HANDLE : IB_SMI_DISCARD);
> >  }
> > +
> > +/*
> > + * Return 1 if the SMP response should be handled by the local
> management stack
> > + */
> 
> The comment is off here - return IB_SMI_HANDLE.  (It's off for
> smi_check_local_smp() as well.)
Yes, I agree.  It appears I was a little over zealous in my header
cut and paste of the existing DR SMP request local check function.

> 
> > +static inline enum smi_action smi_check_local_resp_smp(struct ib_smp
> *smp,
> > +						       struct ib_device
*device)
> > +{
> > +	/* C14-13:3 -- We're at the end of the DR segment of path */
> > +	/* C14-13:4 -- Hop Pointer == 0 -> give to SM */
> > +	return ((device->process_mad &&
> > +		ib_get_smp_direction(smp) &&
> > +		!smp->hop_ptr) ? IB_SMI_HANDLE : IB_SMI_DISCARD);
> > +}
> 
> - Sean
Thanks,
Steve