[ofa-general] Re: [PATCH] opensm/osm_trap_rcv.c: Validate trap is 144 before checking for NodeDescription changed

[Sasha Khapyorsky]

On 08:47 Tue 04 Aug     , Hal Rosenstock wrote:
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
> ---
> diff --git a/opensm/opensm/osm_trap_rcv.c b/opensm/opensm/osm_trap_rcv.c
> index bf39926..925cb27 100644
> --- a/opensm/opensm/osm_trap_rcv.c
> +++ b/opensm/opensm/osm_trap_rcv.c
> @@ -2,6 +2,7 @@
>   * Copyright (c) 2004-2008 Voltaire, Inc. All rights reserved.
>   * Copyright (c) 2002-2006 Mellanox Technologies LTD. All rights reserved.
>   * Copyright (c) 1996-2003 Intel Corporation. All rights reserved.
> + * Copyright (c) 2009 HNR Consulting. All rights reserved.
>   *
>   * This software is available to you under a choice of one of two
>   * licenses.  You may choose to be licensed under the terms of the GNU
> @@ -546,42 +547,47 @@ trap_rcv_process_request(IN osm_sm_t * sm,
>  		}
>  	}
>  
> -	/* Check for node description update. IB Spec v1.2.1 pg 823 */
> -	if (p_ntci->data_details.ntc_144.local_changes & TRAP_144_MASK_OTHER_LOCAL_CHANGES &&
> -	    p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_NODE_DESCRIPTION_CHANGE) {
> -		OSM_LOG(sm->p_log, OSM_LOG_INFO, "Trap 144 Node description update\n");
> -
> -		if (p_physp) {
> -			CL_PLOCK_ACQUIRE(sm->p_lock);
> -			osm_req_get_node_desc(sm, p_physp);
> -			CL_PLOCK_RELEASE(sm->p_lock);
> -		} else {
> -			OSM_LOG(sm->p_log, OSM_LOG_ERROR,
> -				"ERR 3812: No physical port found for "
> -				"trap 144: \"node description update\"\n");
> +	if (ib_notice_is_generic(p_ntci)) {
> +		/* Check for node description update. IB Spec v1.2.1 pg 823 */
> +		if (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 144) {
> +			if (p_ntci->data_details.ntc_144.local_changes & TRAP_144_MASK_OTHER_LOCAL_CHANGES &&
> +			    p_ntci->data_details.ntc_144.change_flgs & TRAP_144_MASK_NODE_DESCRIPTION_CHANGE) {
> +				OSM_LOG(sm->p_log, OSM_LOG_INFO,
> +					"Trap 144 Node description update\n");
> +
> +				if (p_physp) {
> +					CL_PLOCK_ACQUIRE(sm->p_lock);
> +					osm_req_get_node_desc(sm, p_physp);
> +					CL_PLOCK_RELEASE(sm->p_lock);
> +				} else
> +					OSM_LOG(sm->p_log, OSM_LOG_ERROR,
> +						"ERR 3812: No physical port found for "
> +						"trap 144: \"node description update\"\n");
> +			}
>  		}
> -	}
>  
> -	/* do a sweep if we received a trap */
> -	if (sm->p_subn->opt.sweep_on_trap) {
> -		/* if this is trap number 128 or run_heavy_sweep is TRUE -
> -		   update the force_heavy_sweep flag of the subnet.
> -		   Sweep also on traps 144/145 - these traps signal a change of
> -		   certain port capabilities/system image guid.
> -		   TODO: In the future this can be changed to just getting
> -		   PortInfo on this port instead of sweeping the entire subnet. */
> -		if (ib_notice_is_generic(p_ntci) &&
> -		    (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 128 ||
> -		     cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 144 ||
> -		     cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 145 ||
> -		     run_heavy_sweep)) {
> -			OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> -				"Forcing heavy sweep. Received trap:%u\n",
> -				cl_ntoh16(p_ntci->g_or_v.generic.trap_num));
> +		/* do a sweep if we received a trap */
> +		if (sm->p_subn->opt.sweep_on_trap) {
> +			/* if this is trap number 128 or run_heavy_sweep is
> +			   TRUE - update the force_heavy_sweep flag of the
> +			   subnet. Also, sweep also on traps 144/145 -
> +			   these traps signal a change of certain port
> +			   capabilities/system image guid.
> +			   TODO: In the future this can be changed to just
> +			   getting PortInfo on this port instead of sweeping
> +			   the entire subnet. */
> +			if (cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 128 ||
> +			    cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 144 ||
> +			    cl_ntoh16(p_ntci->g_or_v.generic.trap_num) == 145 ||
> +			    run_heavy_sweep) {
> +				OSM_LOG(sm->p_log, OSM_LOG_VERBOSE,
> +					"Forcing heavy sweep. Received trap:%u\n",
> +					cl_ntoh16(p_ntci->g_or_v.generic.trap_num));
>  
> -			sm->p_subn->force_heavy_sweep = TRUE;
> +				sm->p_subn->force_heavy_sweep = TRUE;
> +			}
> +			osm_sm_signal(sm, OSM_SIGNAL_SWEEP);
>  		}
> -		osm_sm_signal(sm, OSM_SIGNAL_SWEEP);

Actually this disables sweep (light) on non generic traps. Was it desired
change? Could you see any potential issues with it?

Sasha

>  	}
>  
>  	/* If we reached here due to trap 129/130/131 - do not need to do
>