[ofa-general] Re: [PATCH] libibmad: added support for handling of BM (Baseboard management) MADs

[Sasha Khapyorsky]

Hi Itai,

On 16:22 Tue 17 Mar     , Itai Baz wrote:
> diff --git a/libibmad/include/infiniband/mad.h b/libibmad/include/infiniband/mad.h
> index b8290a7..efc887a 100644
> --- a/libibmad/include/infiniband/mad.h
> +++ b/libibmad/include/infiniband/mad.h

<snip...>

> @@ -876,6 +922,9 @@ MAD_EXPORT uint8_t *performance_reset_via(void *rcvbuf, ib_portid_t * dest,
>  					  unsigned timeout, unsigned id,
>  					  const struct ibmad_port *srcport);
>  
> +/* bm.c */
> +uint8_t * bm_rpc_call_via(void *data, ib_portid_t *portid, ib_bm_call_t *call, struct ibmad_port *srcport);
> +

I think that MAD_EXPORT is needed here in order to not break WinOF
compatibility.

>  /* dump.c */
>  MAD_EXPORT ib_mad_dump_fn
>      mad_dump_int, mad_dump_uint, mad_dump_hex, mad_dump_rhex,
> diff --git a/libibmad/src/bm.c b/libibmad/src/bm.c
> new file mode 100644
> index 0000000..a095b9a
> --- /dev/null
> +++ b/libibmad/src/bm.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (c) 2004-2007 Voltaire Inc.  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
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + *     Redistribution and use in source and binary forms, with or
> + *     without modification, are permitted provided that the following
> + *     conditions are met:
> + *
> + *      - Redistributions of source code must retain the above
> + *        copyright notice, this list of conditions and the following
> + *        disclaimer.
> + *
> + *      - Redistributions in binary form must reproduce the above
> + *        copyright notice, this list of conditions and the following
> + *        disclaimer in the documentation and/or other materials
> + *        provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + */
> +
> +#if HAVE_CONFIG_H
> +#  include <config.h>
> +#endif /* HAVE_CONFIG_H */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +#include <string.h>
> +#include <sys/time.h>
> +
> +#include <infiniband/mad.h>
> +
> +#undef DEBUG
> +#define DEBUG 	if (ibdebug)	IBWARN
> +
> +static inline int
> +response_expected(int method)
> +{
> +	return method == IB_MAD_METHOD_GET ||
> +		method == IB_MAD_METHOD_SET ||
> +		method == IB_MAD_METHOD_TRAP;
> +}
> +
> +uint8_t *
> +bm_rpc_call_via(void *data, ib_portid_t *portid, ib_bm_call_t *call, struct ibmad_port *srcport)

This function uses rpc or regular sends. Maybe slightly better name
would be just 'bm_call_via'?

> +{
> +	ib_rpc_t rpc = {0};
> +	int resp_expected;
> +	char data_with_bkey[IB_BM_BKEY_AND_DATA_SZ] = {0};
> +
> +	DEBUG("route %s data %p", portid2str(portid), data);
> +	if (portid->lid <= 0) {
> +		IBWARN("only lid routes are supported");
> +		return 0;
> +	}
> +
> +	resp_expected = response_expected(call->method);
> +
> +	rpc.mgtclass = IB_BOARD_MGMT_CLASS;
> +
> +	rpc.method = call->method;
> +	rpc.attr.id = call->attrid;
> +	rpc.attr.mod = call->mod;
> +	rpc.timeout = resp_expected ? call->timeout : 0;
> +	// send data and bkey
> +	rpc.datasz = IB_BM_BKEY_AND_DATA_SZ;
> +	rpc.dataoffs = IB_BM_BKEY_OFFS;
> +
> +	// copy data to a buffer which also includes the bkey
> +	*((uint64_t *) data_with_bkey) = call->bkey;

Actually this means that B_Key should be provided in call->bkey in
network byte order. It is not like most others "call" helpers work.
Wouldn't it be better to keep compatibility here?

> +	memcpy(data_with_bkey + IB_BM_DATA_OFFS - IB_BM_BKEY_OFFS, data, IB_BM_DATA_SZ);
> +
> +	DEBUG("method 0x%x attr 0x%x mod 0x%x datasz %d off %d res_ex %d bkey 0x%08x%08x",
> +		rpc.method, rpc.attr.id, rpc.attr.mod,
> +		rpc.datasz, rpc.dataoffs, resp_expected,
> +		(int) (call->bkey >> 32), (int) call->bkey);
> +
> +	portid->qp = 1;
> +	if (!portid->qkey)
> +		portid->qkey = IB_DEFAULT_QP1_QKEY;
> +
> +	if (resp_expected)
> +		return mad_rpc_rmpp(srcport, &rpc, portid, 0, data_with_bkey);		/* FIXME: no RMPP for now */

How is the response data returned to the caller? data_with_bkey is a
local buffer, right?

> +
> +	return mad_send_via(&rpc, portid, 0, data_with_bkey, srcport) < 0 ? 0 : data_with_bkey;		/* FIXME: no RMPP for now */
> +}

Sasha