Thanks Al Shashi
-----Original Message----- From: Al Chu [mailto:ch...@llnl.gov] Sent: Friday, May 30, 2014 10:26 AM To: Dande, Shashi Cc: freeipmi-devel@gnu.org Subject: RE: FreeIPMI Patch Submission Great. It'll be in the next release of FreeIPMI (1.4.4) Al On Thu, 2014-05-29 at 23:32 +0000, Dande, Shashi wrote: > 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:ch...@llnl.gov] > 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:ch...@llnl.gov] > > Sent: Friday, May 09, 2014 9:54 AM > > To: Dande, Shashi > > Cc: freeipmi-devel@gnu.org > > 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:ch...@llnl.gov] > > > 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:ch...@llnl.gov] > > > > 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 freeipmi-devel@gnu.org 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 > > > > ch...@llnl.gov > > > > Computer Scientist > > > > High Performance Systems Division Lawrence Livermore National > > > > Laboratory > > > > > > > > > > > -- > > > Albert Chu > > > ch...@llnl.gov > > > Computer Scientist > > > High Performance Systems Division > > > Lawrence Livermore National Laboratory > > > > > > > > -- > > Albert Chu > > ch...@llnl.gov > > Computer Scientist > > High Performance Systems Division > > Lawrence Livermore National Laboratory > > > -- > Albert Chu > ch...@llnl.gov > Computer Scientist > High Performance Systems Division > Lawrence Livermore National Laboratory > > -- Albert Chu ch...@llnl.gov Computer Scientist High Performance Systems Division Lawrence Livermore National Laboratory _______________________________________________ Freeipmi-devel mailing list Freeipmi-devel@gnu.org https://lists.gnu.org/mailman/listinfo/freeipmi-devel