[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