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

[Hal Rosenstock]

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.

-- Hal

>  - R.