[ofa-general] ***SPAM*** Re: [PATCHv3] libibmad/rpc.c: Handle redirection status

[Sasha Khapyorsky]

On 06:30 Wed 11 Mar     , Hal Rosenstock wrote:
> 
> Also, in mad_rpc, status should be based on management class
> 
> Signed-off-by: Hal Rosenstock <hal.rosenstock at gmail.com>
> ---
> Changes since v2:
> Made ERRS into macro
> 
> Changes since v1:
> Always get 16 bits of MAD status and mask when DR SMP
> Remove some extraneous braces
> 
> diff --git a/libibmad/src/rpc.c b/libibmad/src/rpc.c
> index 8c68cf9..e9a1c06 100644
> --- a/libibmad/src/rpc.c
> +++ b/libibmad/src/rpc.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright (c) 2004-2006 Voltaire Inc.  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
> @@ -59,7 +60,10 @@ static int save_mad_len = 256;
>  
>  #undef DEBUG
>  #define DEBUG	if (ibdebug)	IBWARN
> -#define ERRS	if (iberrs || ibdebug)	IBWARN
> +#define ERRS(fmt, ...) do {	\
> +	if (iberrs || ibdebug)	\
> +		IBWARN(fmt, ## __VA_ARGS__); \
> +} while (0)
>  
>  #define MAD_TID(mad)	(*((uint64_t *)((char *)(mad) + 8)))
>  
> @@ -128,9 +132,8 @@ _do_madrpc(int port_id, void *sndbuf, void *rcvbuf, int agentid, int len,
>  	    (uint32_t) mad_get_field64(umad_get_mad(sndbuf), 0, IB_MAD_TRID_F);
>  
>  	for (retries = 0; retries < madrpc_retries; retries++) {
> -		if (retries) {
> +		if (retries)
>  			ERRS("retry %d (timeout %d ms)", retries, timeout);
> -		}
>  
>  		length = len;
>  		if (umad_send(port_id, agentid, sndbuf, length, timeout, 0) < 0) {

I'm taking this macro wrapping code as separate patch - this seems
unrelated to the rest.

> @@ -170,7 +173,7 @@ void *mad_rpc(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t * dport
>  	      void *payload, void *rcvdata)
>  {
>  	int status, len;
> -	uint8_t sndbuf[1024], rcvbuf[1024], *mad;
> +	uint8_t sndbuf[1024], rcvbuf[1024], *mad, mgmtclass;
>  
>  	len = 0;
>  	memset(sndbuf, 0, umad_size() + IB_MAD_SIZE);
> @@ -187,7 +190,18 @@ void *mad_rpc(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t * dport
>  
>  	mad = umad_get_mad(rcvbuf);
>  
> -	if ((status = mad_get_field(mad, 0, IB_DRSMP_STATUS_F)) != 0) {

Do you know any example where this will not work properly? I don't. This
code assumed as fast execution path, so adding not practically needed
flows doesn't seem like a good idea for me.

> +	status = mad_get_field(mad, 0, IB_MAD_STATUS_F);
> +	mgmtclass = mad_get_field(mad, 0, IB_MAD_MGMTCLASS_F);
> +	if (mgmtclass == IB_SMI_DIRECT_CLASS)
> +		status &= 0x7fff;
> +	else if (mgmtclass != IB_SMI_CLASS) {
> +		if (status & 2) {
> +			ERRS("MAD redirection not supported; dport (%s)",
> +			     portid2str(dport));
> +			return 0;
> +		}
> +	}
> +	if (status) {
>  		ERRS("MAD completed with error status 0x%x; dport (%s)",
>  		     status, portid2str(dport));
>  		return 0;
> @@ -227,8 +241,12 @@ void *mad_rpc_rmpp(const struct ibmad_port *port, ib_rpc_t * rpc, ib_portid_t *
>  	mad = umad_get_mad(rcvbuf);
>  
>  	if ((status = mad_get_field(mad, 0, IB_MAD_STATUS_F)) != 0) {
> -		ERRS("MAD completed with error status 0x%x; dport (%s)",
> -		     status, portid2str(dport));
> +		if (status & 2)
> +			ERRS("MAD redirection not supported; dport (%s)",
> +			     portid2str(dport));
> +		else
> +			ERRS("MAD completed with error status 0x%x; dport (%s)",
> +			     status, portid2str(dport));

The error status is printed originally, isn't it not sufficient? If not
what about adding generic function which prints know error codes as
string instead of handling some values separately?

Sasha