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

[Hal Rosenstock]

On 9/4/07, Steve Welch <swelch at systemfabricworks.com> wrote:
> 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.

Perhaps the original comment should change too to be more accurate.

-- Hal

>
> >
> > > +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
>
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>