[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
ipmi-ssif.patch
Description: ipmi-ssif.patch