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: Thu, 29 May 2014 23:32:30 +0000

Hi Al

Here is the updated patch per our conversation today. 

Thanks
Shashi

Index: ipmi-ssif-driver-api.c
===================================================================
--- ipmi-ssif-driver-api.c      (revision 10066)
+++ ipmi-ssif-driver-api.c      (working copy)
@@ -319,7 +319,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 = IPMI_SSIF_RETRY_DEFAULT;
+  
   assert (ctx
          && ctx->magic == IPMI_CTX_MAGIC
          && ctx->type == IPMI_DEVICE_SSIF
@@ -350,9 +352,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 = IPMI_SSIF_TIMEOUT_DEFAULT;
+          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);
 }
 
Index: ipmi-ssif-driver-api.h
===================================================================
--- ipmi-ssif-driver-api.h      (revision 10066)
+++ ipmi-ssif-driver-api.h      (working copy)
@@ -23,6 +23,9 @@
 #include <freeipmi/api/ipmi-api.h>
 #include <freeipmi/fiid/fiid.h>
 
+#define IPMI_SSIF_RETRY_DEFAULT       5
+#define IPMI_SSIF_TIMEOUT_DEFAULT     20000000 /* 20 ms */
+
 int api_ssif_cmd (ipmi_ctx_t ctx,
                  fiid_obj_t obj_cmd_rq,
                  fiid_obj_t obj_cmd_rs);

-----Original Message-----
From: Albert Chu [mailto:address@hidden 
Sent: Thursday, May 29, 2014 2:16 PM
To: Dande, Shashi
Subject: RE: FreeIPMI Patch Submission

Hi Shashi,

Just re-pinging in case you forgot about this thread.

Thanks,

Al

On Fri, 2014-05-09 at 15:14 +0000, Dande, Shashi wrote:
> 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
> 
--
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory


Attachment: ipmi-ssif.patch
Description: ipmi-ssif.patch


reply via email to

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