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

[Hal Rosenstock]

On Tue, Apr 14, 2009 at 9:56 AM, Sasha Khapyorsky <sashak at voltaire.com> wrote:
> 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.

Not currently but it's a spec compliance issue.

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

IMO it's not sufficient as redirection is not an error.

> If not
> what about adding generic function which prints know error codes as
> string instead of handling some values separately?

That's doable but a separate proposition IMO.

-- Hal

> Sasha
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>