freeipmi-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Freeipmi-devel] FreeIPMI Patch Submission


From: Dande, Shashi
Subject: Re: [Freeipmi-devel] FreeIPMI Patch Submission
Date: Fri, 9 May 2014 15:14:37 +0000

Hi Al

Thanks for your feedback. I will refractor the code to include your feedback 
and repost the patch. 

Thanks 
Shashi

-----Original Message-----
From: Al Chu [mailto:address@hidden 
Sent: Friday, May 09, 2014 9:54 AM
To: Dande, Shashi
Cc: address@hidden
Subject: RE: FreeIPMI Patch Submission

Hi Shashi,

The patch as a whole looks fine, but how about a few tweaks.  Comments inlined 
below.

On Wed, 2014-05-07 at 21:34 +0000, Dande, Shashi wrote:
> Hi Albert
> 
> I have attached the patch file to this e-mail per your advice.
> 
> I have also copied the content below for your reference. 
> 
> Thanks
> Shashi
> 
> 
> Index: ipmi-ssif-driver-api.c
> ===================================================================
> --- ipmi-ssif-driver-api.c    (revision 10066)
> +++ ipmi-ssif-driver-api.c    (working copy)
> @@ -30,6 +30,7 @@
>  #endif /* HAVE_UNISTD_H */
>  #include <assert.h>
>  #include <errno.h>
> +#include <time.h>

Throughout FreeIPMI you'll see code chunks like this:

#if TIME_WITH_SYS_TIME
#include <sys/time.h>
#include <time.h>
#else /* !TIME_WITH_SYS_TIME */
#if HAVE_SYS_TIME_H
#include <sys/time.h>
#else /* !HAVE_SYS_TIME_H */
#include <time.h>
#endif /* !HAVE_SYS_TIME_H */
#endif  /* !TIME_WITH_SYS_TIME */

It is more portable b/c of the weirdness/legacy of the time.h headers.

>  #include "freeipmi/driver/ipmi-ssif-driver.h"
>  #include "freeipmi/debug/ipmi-debug.h"
> @@ -319,7 +320,9 @@
>    uint8_t cmd = 0;             /* used for debugging */
>    uint8_t group_extension = 0; /* used for debugging */
>    uint64_t val;
> -
> +  struct timespec request, remain;
> +  uint8_t retry = 5;

To avoid using "magic values", could we have a #define in the code that will 
set the 5 and also the default 20000 ms below.  Something like

#define IPMI_SSIF_RETRY_DEFAULT
#define IPMI_SSIF_TIMEOUT_DEFAULT

> +  
>    assert (ctx
>         && ctx->magic == IPMI_CTX_MAGIC
>         && ctx->type == IPMI_DEVICE_SSIF
> @@ -350,9 +353,39 @@
>    if (_ssif_cmd_write (ctx, cmd, group_extension, obj_cmd_rq) < 0)
>      return (-1);
>  
> +  
> /******************************************************************************
> +    12.9 SMBus NACKs and Error Recovery:
> +    ====================================
> +    The BMC can NACK the SMBus host controller if it is not ready to accept 
> a new 
> +    transaction. Typically, this will be exhibited by the BMC NACK'ing its 
> slave 
> +    address. 
> +    
> +    If the BMC NACKs a single part transaction, software can simply retry 
> it. 
> +    If a 'middle' or 'end' transaction is NACK'd, software should not retry 
> the 
> +    particular but should restart the multi-part read or write from the 
> beginning
> +    Start transaction for the transfer.
> +  
> + ********************************************************************
> + ***********/
>    if (_ssif_cmd_read (ctx, cmd, group_extension, obj_cmd_rs) < 0)
> -    return (-1);
> +    {
> +      while (1)
> +        {
> +          request.tv_sec = 0; 
> +          request.tv_nsec = 20000000; /* 20 ms */
> +          if (nanosleep (&request, &remain) < 0 )
> +            return (-1);
>  
> +          if (_ssif_cmd_read (ctx, cmd, group_extension, obj_cmd_rs) < 0)
> +            {
> +              if (retry == 0)
> +                return (-1);
> +        
> +              retry--;        
> +            }
> +            else
> +              break;
> +        }
> +    }
> +
>    return (0);
>  }
> 
> -----Original Message-----
> From: Albert Chu [mailto:address@hidden
> Sent: Tuesday, May 06, 2014 5:29 PM
> To: Dande, Shashi
> Subject: RE: FreeIPMI Patch Submission
> 
> The trunk would be best.
> 
> Thanks,
> 
> Al
> 
> On Tue, 2014-05-06 at 22:26 +0000, Dande, Shashi wrote:
> > Thanks for a quick reply. Should I submit the patch against the trunk or a 
> > particular version of your source code. 
> > 
> > Thanks
> > Shashi
> > 
> > -----Original Message-----
> > From: Albert Chu [mailto:address@hidden
> > Sent: Tuesday, May 06, 2014 5:23 PM
> > To: Dande, Shashi
> > Subject: Re: FreeIPMI Patch Submission
> > 
> > Hi,
> > 
> > I'd be happy to work with you to get the code integrated.  Why don't you 
> > post the patch to the address@hidden mailing list and we can iterate on the 
> > patch there.
> > 
> > Al
> > 
> > On Tue, 2014-05-06 at 22:10 +0000, Shashi Dande wrote:
> > > Hi Albert
> > > 
> > > I am a software architect from Hewlett Packard and recenlty  I 
> > > have updated the FreeIPMI code to make sure that it works on our 
> > > next genaration ARM platforms. Please let me know if I can work 
> > > with you to merge these changes into the FreeIPMI project.
> > > 
> > > Thanks
> > > Shashi
> > > 
> > > _______________________________________________
> > >   Message sent via/by Savannah
> > >   http://savannah.gnu.org/
> > > 
> > --
> > Albert Chu
> > address@hidden
> > Computer Scientist
> > High Performance Systems Division
> > Lawrence Livermore National Laboratory
> > 
> > 
> --
> Albert Chu
> address@hidden
> Computer Scientist
> High Performance Systems Division
> Lawrence Livermore National Laboratory
> 
> 
--
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory




reply via email to

[Prev in Thread] Current Thread [Next in Thread]