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 _______________________________________________ Freeipmi-devel mailing list Freeipmi-devel@gnu.org https://lists.gnu.org/mailman/listinfo/freeipmi-devel