[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Freeipmi-devel] FreeIPMI Patch Submission
|
From: |
Al Chu |
|
Subject: |
Re: [Freeipmi-devel] FreeIPMI Patch Submission |
|
Date: |
Fri, 30 May 2014 08:26:25 -0700 |
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: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
>
>
--
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory