[ofa-general] RE: [PATCH] IB/core: Enhance SMI for switch support

[Hal Rosenstock]

On Thu, 2007-03-29 at 13:16, Suresh Shelvapille wrote:
> > -----Original Message-----
> > From: Hal Rosenstock [mailto:halr at voltaire.com]
> > Sent: Thursday, March 29, 2007 2:47 PM
> > To: Roland Dreier
> > Cc: general at lists.openfabrics.org; Sean Hefty; Suresh Shelvapille
> > Subject: Re: [PATCH] IB/core: Enhance SMI for switch support
> > 
> > On Thu, 2007-03-29 at 12:12, Roland Dreier wrote:
> > >  > +		retsmi = smi_check_forward_dr_smp(&recv->mad.smp);
> > >  > +		if (!retsmi)
> > >  >  			goto local;
> > >  > -		if (!smi_handle_dr_smp_send(&recv->mad.smp,
> > >  > -					    port_priv->device->node_type,
> > >  > -					    port_priv->port_num))
> > >  > -			goto out;
> > >  > -		if (!smi_check_local_smp(&recv->mad.smp, port_priv->device))
> > >  > +
> > >  > +		if (retsmi == 1) { /* don't forward */
> > >
> > >  >  /*
> > >  >   * Return 1 if the received DR SMP should be forwarded to the send queue
> > >  >   * Return 0 if the SMP should be completed up the stack
> > >  > + * Return 2 if the SMP should be forwarded (for switches only)
> > >  >   */
> > >  >  int smi_check_forward_dr_smp(struct ib_smp *smp)
> > >
> > > I think this has now crossed the line where these magic return values
> > > should be named enums instead.
> > 
> > OK; I'll make these into enums.
> > 
> > >   Especially the "if (!retsmi)" is very
> > > hard to follow.
> > 
> > Is it hard to follow ?
> > 
> > >  > +/*
> > >  > + * Return the forwarding port number from initial_path for outgoing SMP and
> > >  > + * from return_path for returning SMP
> > >  > + */
> > >  > +static inline int smi_get_fwd_port(struct ib_smp *smp)
> > >  > +{
> > >  > +	return (!ib_get_smp_direction(smp) ? smp->initial_path[smp->hop_ptr+1] :
> > >  > +		smp->return_path[smp->hop_ptr-1]);
> > >  > +}
> > >
> > > This has exactly one caller.  I would just put this function in the .c
> > > file where it's called.
> > 
> > I'll resubmit a v2 of this patch later with this change and the enum
> > change.
> > 
> 
> the reason this was made into a function and put inside the header file was because 
> paths weren't accessed directly within mad.c.
> 
> If you are going to do what Roland is suggesting, then why have a function? Why not 
> just stick it in-line as I had before.

Because this is a SMI function and touches the DR path. Nothing in mad.c
currently directly touches the DR paths without using a SMI function.

-- Hal

> Thanks,
> Suri
>