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

[Suresh Shelvapille]

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

Thanks,
Suri