Re: [Fwd: Re: PATCH: ipmi-oem dell get-system-info cmc-firmware-version]

[Ryan Cox]

Al,

Thanks.  I did indeed have some problems with email that should now be fixed.

I'll make the changes in the next few days.  You are correct that the reason for the error handling was to make a nicer error message.  The alternative was to leave it as "dell:get-system-info failed: No error message found for command DFh, network function 06h, and completion code 80h.  Please report to freeipmi-devel@...".

I'll be sure to add the response format in comments and an entry in the manpage.

Ryan

On 05/18/2012 12:41 PM, Albert Chu wrote:

This was my reply if it didn't get through your filters

Al

-- 
Albert Chu
chu11@...
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info.eml

Subject:

Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info cmc-firmware-version

From:

Albert Chu chu11@...

Date:

05/18/2012 12:19 PM

To:

Ryan Cox ryan_cox@...

CC:

freeipmi-devel@... freeipmi-devel@...

Hey Ryan,

For the most part looks fine.  I assume the final patch will also clean
up the code comments to describe the CMC version packet :-)  Just a few
notes:

+int
+ipmi_oem_dell_get_cmc_firmware_version (ipmi_oem_state_data_t
*state_data)

since this is an internal function it should be static and not have an
entry in the header file.  For naming consistency, it should probably be
_get_dell_cmc_firmware_version() or something similar to the other
functions called by ipmi_oem_get_system_info().

+  if(rs_len != 20) {
+      pstdout_fprintf (state_data->pstate,
+                       stderr,
+                       "dell:get-system-info 'cmc-firmware-version'
option not supported on this system\n");
+      goto cleanup;
+  }

I don't think this check is really necessary, it'll be done via
ipmi_oem_check_response_and_completion_code().  I suppose you added it
for a nicer error message, but with ipmi-oem, I think there is a larger
acceptance of "you gotta know what you're doing" b/c it's so
OEM/motherboard/system/product specific (vs ipmi-sensors and more
standard tools).  The manpage should document the motherboards its
confirmed against or types of motherboards its expected to work with.

Al

On Fri, 2012-05-18 at 10:34 -0700, Ryan Cox wrote:

> This patch adds "ipmi-oem dell get-system-info cmc-firmware-version".  
>  From a blade, it grabs the firmware version of the CMC for the 
> enclosure that the blade is in.  It appears to only show the active CMC 
> firmware and not show anything for the standby CMC (if there is one).  I 
> have no idea what it would show if there is a redundant CMC using 
> different firmware.  This patch has worked for every Dell blade (10G, 
> 11G, 12G) we have in an M1000e enclosure, including M1000e's with one 
> and two CMC's.
> 
> The version can be grabbed with: ipmi-raw 0 6 0x59 0 0xDF 1 0
> rcvd: 59 00 11 01 00 00 00 00 33 2E 30 33 00 00 00 00 00 00 00 00
> 
> "3.03" in this case.  I have tried this with several 3.x and 4.x 
> versions and the version numbers are all 4 characters long.
> 
> As far as I can tell, there is no easy way to grab this through existing 
> generic functions in ipmi-oem-dell.c.  Those functions appear to assume 
> set selector and block selector 0, though the set selector is different 
> in this case.  I had to decide to either reinvent the wheel somewhat or 
> modify the other functions to take selectors as parameters instead of 
> assuming 0.  I took the easy route and reinvented the wheel, though I'm 
> open to changing that if needed.  I figured I should at least get some 
> feedback before submitting a patch that changes a whole lot of code.
> 
> This patch may need some reworking, but it has been tested successfully 
> on: M600, M610, M610x, M910, and a pre-prod 12G system.  On a rackmount, 
> it returns "dell:get-system-info 'cmc-firmware-version' option not 
> supported on this system".
> 
> Let me know if there's anything that needs to be changed and if you 
> would prefer the slight duplication of code that I did versus adding set 
> and block selector parameters to several functions.
> 

-- 
Albert Chu
chu11@...
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory



_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel

_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel

[Ryan Cox]

Al,

Attached is a cleaned up patch that changes the function to static and renames it.  The message format is now included in a format that hopefully makes sense.  I tried making it look like the other documentation in ipmi-oem-dell.c.  I changed the error message to display if the completion code is non-zero.  I'm open to removing the error handler if you still think it's unnecessary.

A very simple patch to the manpage is also included.  All it does is add cmc-firmware-version to the list of get-system-info subcommands.  The system support listed for get-system-info is still valid.

Let me know if I should make any other changes.

Ryan

On 05/18/2012 12:53 PM, Ryan Cox wrote:

Al,

Thanks.  I did indeed have some problems with email that should now be fixed.

I'll make the changes in the next few days.  You are correct that the reason for the error handling was to make a nicer error message.  The alternative was to leave it as "dell:get-system-info failed: No error message found for command DFh, network function 06h, and completion code 80h.  Please report to freeipmi-devel@...".

I'll be sure to add the response format in comments and an entry in the manpage.

Ryan

On 05/18/2012 12:41 PM, Albert Chu wrote:

This was my reply if it didn't get through your filters

Al

-- 
Albert Chu
chu11@...
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory

Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info.eml

Subject:

Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info cmc-firmware-version

From:

Albert Chu chu11@...

Date:

05/18/2012 12:19 PM

To:

Ryan Cox ryan_cox@...

CC:

freeipmi-devel@... freeipmi-devel@...

Hey Ryan,

For the most part looks fine.  I assume the final patch will also clean
up the code comments to describe the CMC version packet :-)  Just a few
notes:

+int
+ipmi_oem_dell_get_cmc_firmware_version (ipmi_oem_state_data_t
*state_data)

since this is an internal function it should be static and not have an
entry in the header file.  For naming consistency, it should probably be
_get_dell_cmc_firmware_version() or something similar to the other
functions called by ipmi_oem_get_system_info().

+  if(rs_len != 20) {
+      pstdout_fprintf (state_data->pstate,
+                       stderr,
+                       "dell:get-system-info 'cmc-firmware-version'
option not supported on this system\n");
+      goto cleanup;
+  }

I don't think this check is really necessary, it'll be done via
ipmi_oem_check_response_and_completion_code().  I suppose you added it
for a nicer error message, but with ipmi-oem, I think there is a larger
acceptance of "you gotta know what you're doing" b/c it's so
OEM/motherboard/system/product specific (vs ipmi-sensors and more
standard tools).  The manpage should document the motherboards its
confirmed against or types of motherboards its expected to work with.

Al

On Fri, 2012-05-18 at 10:34 -0700, Ryan Cox wrote:

> This patch adds "ipmi-oem dell get-system-info cmc-firmware-version".  
>  From a blade, it grabs the firmware version of the CMC for the 
> enclosure that the blade is in.  It appears to only show the active CMC 
> firmware and not show anything for the standby CMC (if there is one).  I 
> have no idea what it would show if there is a redundant CMC using 
> different firmware.  This patch has worked for every Dell blade (10G, 
> 11G, 12G) we have in an M1000e enclosure, including M1000e's with one 
> and two CMC's.
> 
> The version can be grabbed with: ipmi-raw 0 6 0x59 0 0xDF 1 0
> rcvd: 59 00 11 01 00 00 00 00 33 2E 30 33 00 00 00 00 00 00 00 00
> 
> "3.03" in this case.  I have tried this with several 3.x and 4.x 
> versions and the version numbers are all 4 characters long.
> 
> As far as I can tell, there is no easy way to grab this through existing 
> generic functions in ipmi-oem-dell.c.  Those functions appear to assume 
> set selector and block selector 0, though the set selector is different 
> in this case.  I had to decide to either reinvent the wheel somewhat or 
> modify the other functions to take selectors as parameters instead of 
> assuming 0.  I took the easy route and reinvented the wheel, though I'm 
> open to changing that if needed.  I figured I should at least get some 
> feedback before submitting a patch that changes a whole lot of code.
> 
> This patch may need some reworking, but it has been tested successfully 
> on: M600, M610, M610x, M910, and a pre-prod 12G system.  On a rackmount, 
> it returns "dell:get-system-info 'cmc-firmware-version' option not 
> supported on this system".
> 
> Let me know if there's anything that needs to be changed and if you 
> would prefer the slight duplication of code that I did versus adding set 
> and block selector parameters to several functions.
> 

-- 
Albert Chu
chu11@...
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory



_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel

_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel

[ipmi-oem.8.pre.in.cmc-firmware-version.patch]

---

--- ipmi-oem.8.pre.in.orig 2012-05-21 12:04:05.403167000 -0600
+++ ipmi-oem.8.pre.in 2012-05-18 09:39:12.468223000 -0600
@@ -80,11 +80,11 @@ Valid keys are \fIguid\fR, \fIasset\-tag
 \fIboard\-revision\fR, \fIplatform\-model\-name\fR,
 \fIslot\-number\fR, \fIsystem\-revision\fR, \fIidrac\-info\fR,
 \fIidrac\-ipv4\-url\fR, \fIidrac\-gui\-webserver\-control\fR,
-\fIcmc\-ipv4\-url\fR, \fIcmc\-ipv6\-info\fR, \fIcmc\-ipv6\-url\fR,
-\fImac\-addresses\fR.  Command confirmed to work on Dell Poweredge
-2900, 2950, R610, R710, M600, M610, M610X, M910, and R905.  However,
-individual system information options may not be readable or available
-on every system.
+\fIcmc\-firmware\-version\fR, \fIcmc\-ipv4\-url\fR,
+\fIcmc\-ipv6\-info\fR, \fIcmc\-ipv6\-url\fR, \fImac\-addresses\fR.  
+Command confirmed to work on Dell Poweredge 2900, 2950, R610, R710,
+M600, M610, M610X, M910, and R905.  However, individual system
+information options may not be readable or available on every system.
 .TP
 .B get-nic-selection
 This OEM command will determine the current NIC selection for IPMI as

---

[ipmi-oem-dell.c.cmc-firmware-version.patch]

---

--- ipmi-oem-dell.c.orig 2012-05-18 09:19:48.962028000 -0600
+++ ipmi-oem-dell.c 2012-05-21 11:58:55.617101000 -0600
@@ -374,6 +374,7 @@
 #define IPMI_OEM_DELL_PORT_MAP_SLOT_MAPPING_1_2_OR_1_4        1
 #define IPMI_OEM_DELL_PORT_MAP_SLOT_MAPPING_1_8               2
 
+#define IPMI_OEM_DELL_CMC_FIRMWARE_VERSION 0xDF
 
 /* Will call ipmi_cmd_get_system_info_parameters only once, b/c field
  * requested is defined by OEM to be < 16 bytes in length
@@ -1797,6 +1798,94 @@ _output_dell_system_info_11g_or_12g_mac_
   return (rv);
 }
 
+static int
+_get_dell_cmc_firmware_version (ipmi_oem_state_data_t *state_data)
+{
+  uint8_t bytes_rq[IPMI_OEM_MAX_BYTES];
+  uint8_t bytes_rs[IPMI_OEM_MAX_BYTES];
+  uint8_t bytes_read[4];
+  int rs_len;
+  int rv = -1;
+
+  assert (state_data);
+  assert (!state_data->prog_data->args->oem_options_count);
+
+  /* Dell Poweredge OEM
+   *
+   * Ryan Cox: found by walking through addresses
+   *
+   * Get CMC Firmware Version
+   *
+   * 0x06 - OEM network function
+   * 0x59 - OEM cmd
+   * 0x00
+   * 0xDF - CMC Info?
+   * 0x01 - set selector (0x00 contains at least the CMC IPv4 addr)
+   * 0x00 - block selector
+   *
+   * Get CMC Firmware Version Response
+   *
+   * 0x59 - OEM cmd
+   * 0x?? - Completion Code
+   * 0x11 - length
+   * 0x01 - set selector
+   * 0x00 - unknown
+   * 0x00 - unknown
+   * 0x00 - unknown
+   * 0x00 - unknown
+   * 0x?? - Firmware version char 0
+   * 0x?? - Firmware version char 1
+   * 0x?? - Firmware version char 2
+   * 0x?? - Firmware version char 3
+   * trailing 8 bytes: 0x00 - unknown
+   *
+   */
+
+  bytes_rq[0] = IPMI_CMD_GET_SYSTEM_INFO_PARAMETERS;
+  bytes_rq[1] = 0x00;
+  bytes_rq[2] = IPMI_OEM_DELL_CMC_FIRMWARE_VERSION;
+  bytes_rq[3] = 0x01;
+  bytes_rq[4] = 0x00;
+
+  if ((rs_len = ipmi_cmd_raw (state_data->ipmi_ctx,
+                              0, /* lun */
+                              IPMI_NET_FN_APP_RQ, /* network function */
+                              bytes_rq, /* data */
+                              5, /* num bytes */
+                              bytes_rs,
+                              IPMI_OEM_MAX_BYTES)) < 0)
+    {
+      pstdout_fprintf (state_data->pstate,
+                       stderr,
+                       "ipmi_cmd_raw: %s\n",
+                       ipmi_ctx_errormsg (state_data->ipmi_ctx));
+      goto cleanup;
+    }
+
+  if(rs_len != 20) {
+      pstdout_fprintf (state_data->pstate,
+                       stderr,
+                       "dell:get-system-info 'cmc-firmware-version' option not supported on this system\n");
+      goto cleanup;
+  }
+
+  if (ipmi_oem_check_response_and_completion_code (state_data,
+                                                   bytes_rs,
+                                                   rs_len,
+                                                   20,
+                                                   IPMI_OEM_DELL_CMC_FIRMWARE_VERSION,
+                                                   IPMI_NET_FN_APP_RQ,
+                                                   NULL) < 0)
+    goto cleanup;
+
+    memcpy(bytes_read, &bytes_rs[8], 4);
+    pstdout_printf (state_data->pstate, "%s\n", bytes_read);
+  
+  rv = 0;
+ cleanup:
+  return (rv);
+}
+
 int
 ipmi_oem_dell_get_system_info (ipmi_oem_state_data_t *state_data)
 {
@@ -1824,6 +1913,7 @@ ipmi_oem_dell_get_system_info (ipmi_oem_
       "Option: idrac-info\n"
       "Option: idrac-ipv4-url\n"
       "Option: idrac-gui-webserver-control\n"
+      "Option: cmc-firmware-version\n"
       "Option: cmc-ipv4-url\n"
       "Option: cmc-ipv6-info\n"
       "Option: cmc-ipv6-url\n"
@@ -1857,6 +1947,7 @@ ipmi_oem_dell_get_system_info (ipmi_oem_
       && strcasecmp (state_data->prog_data->args->oem_options[0], "idrac-info")
       && strcasecmp (state_data->prog_data->args->oem_options[0], "idrac-ipv4-url")
       && strcasecmp (state_data->prog_data->args->oem_options[0], "idrac-gui-webserver-control")
+      && strcasecmp (state_data->prog_data->args->oem_options[0], "cmc-firmware-version")
       && strcasecmp (state_data->prog_data->args->oem_options[0], "cmc-ipv4-url")
       && strcasecmp (state_data->prog_data->args->oem_options[0], "cmc-ipv6-info")
       && strcasecmp (state_data->prog_data->args->oem_options[0], "cmc-ipv6-url")
@@ -2197,6 +2288,11 @@ ipmi_oem_dell_get_system_info (ipmi_oem_
       "%s\n",
       idrac_web_gui_server_control_str);
     }
+   else if (!strcasecmp (state_data->prog_data->args->oem_options[0], "cmc-firmware-version"))
+    {
+       if (_get_dell_cmc_firmware_version (state_data) < 0)
+         goto cleanup;
+    }
   else if (!strcasecmp (state_data->prog_data->args->oem_options[0], "cmc-ipv4-url"))
     {
       if (_get_dell_system_info_long_string (state_data,

---

_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel

[Albert Chu]

Hey Ryan,

Argh, I just realized something that I didn't come to me in my first
review of the patch.  Why not use the call to:

ipmi_cmd_get_system_info_parameters()

instead of building the packet yourself and calling ipmi_cmd_raw()?
That way you don't have to build up the packet like you were.

If you follow the code in _output_dell_system_info_cmc_ipv6_info(),
there should be a lot of similarities.  That would eliminate the call to
ipmi_oem_check_response_and_completion_code() too, an appropriate error
could be generated if the response is not long enough.  The comments on
the OEM packet would be adjusted to describe the system info response
instead of the actual packet payload.

A few minor tweaks:

+#define IPMI_OEM_DELL_CMC_FIRMWARE_VERSION 0xDF

lets put this in

libfreeipmi/include/freeipmi/spec/ipmi-system-info-parameters-oem-spec.h

along with the other OEM system info parameters and rename
appropriately.  Please also add a comment something like:

"Ryan Cox: reverse engineered by walking through addresses.  Is correct
name?"

you'll see similar comments from me in there.

+  assert (!state_data->prog_data->args->oem_options_count);

I think this assert should be oem_options_count == 1?  If you
run ./configure with --enable-debug, that will turn on the asserts and
we'll see if we trigger it.

BTW, if you could, put this together into just one patch.

Thanks,
Al


On Mon, 2012-05-21 at 16:03 -0700, Ryan Cox wrote:

> Al,
>
> Attached is a cleaned up patch that changes the function to static and
> renames it.  The message format is now included in a format that
> hopefully makes sense.  I tried making it look like the other
> documentation in ipmi-oem-dell.c.  I changed the error message to
> display if the completion code is non-zero.  I'm open to removing the
> error handler if you still think it's unnecessary.
>
> A very simple patch to the manpage is also included.  All it does is
> add cmc-firmware-version to the list of get-system-info subcommands.
> The system support listed for get-system-info is still valid.
>
> Let me know if I should make any other changes.
>
> Ryan
>
> On 05/18/2012 12:53 PM, Ryan Cox wrote:
> > Al,
> >
> > Thanks.  I did indeed have some problems with email that should now
> > be fixed.
> >
> > I'll make the changes in the next few days.  You are correct that
> > the reason for the error handling was to make a nicer error message.
> > The alternative was to leave it as "dell:get-system-info failed: No
> > error message found for command DFh, network function 06h, and
> > completion code 80h.  Please report to <freeipmi-devel@...>".
> >
> > I'll be sure to add the response format in comments and an entry in
> > the manpage.
> >
> > Ryan
> >
> > On 05/18/2012 12:41 PM, Albert Chu wrote:
> > > This was my reply if it didn't get through your filters
> > >
> > > Al
> > >
> > > -- Albert Chu chu11@... Computer Scientist High Performance
> > > Systems Division Lawrence Livermore National Laboratory
> > >
> > > Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info.eml
> > > Subject:
> > > Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info
> > > cmc-firmware-version
> > > From:
> > > Albert Chu <chu11@...>
> > > Date:
> > > 05/18/2012 12:19 PM
> > > To:
> > > Ryan Cox <ryan_cox@...>
> > > CC:
> > > "freeipmi-devel@..." <freeipmi-devel@...>
> > >
> > > Hey Ryan,
> > >
> > > For the most part looks fine.  I assume the final patch will also clean
> > > up the code comments to describe the CMC version packet :-)  Just a few
> > > notes:
> > >
> > > +int
> > > +ipmi_oem_dell_get_cmc_firmware_version (ipmi_oem_state_data_t
> > > *state_data)
> > >
> > > since this is an internal function it should be static and not have an
> > > entry in the header file.  For naming consistency, it should probably be
> > > _get_dell_cmc_firmware_version() or something similar to the other
> > > functions called by ipmi_oem_get_system_info().
> > >
> > > +  if(rs_len != 20) {
> > > +      pstdout_fprintf (state_data->pstate,
> > > +                       stderr,
> > > +                       "dell:get-system-info 'cmc-firmware-version'
> > > option not supported on this system\n");
> > > +      goto cleanup;
> > > +  }
> > >
> > > I don't think this check is really necessary, it'll be done via
> > > ipmi_oem_check_response_and_completion_code().  I suppose you added it
> > > for a nicer error message, but with ipmi-oem, I think there is a larger
> > > acceptance of "you gotta know what you're doing" b/c it's so
> > > OEM/motherboard/system/product specific (vs ipmi-sensors and more
> > > standard tools).  The manpage should document the motherboards its
> > > confirmed against or types of motherboards its expected to work with.
> > >
> > > Al
> > >
> > > On Fri, 2012-05-18 at 10:34 -0700, Ryan Cox wrote:
> > > > > This patch adds "ipmi-oem dell get-system-info cmc-firmware-version".  
> > > > >  From a blade, it grabs the firmware version of the CMC for the
> > > > > enclosure that the blade is in.  It appears to only show the active CMC
> > > > > firmware and not show anything for the standby CMC (if there is one).  I
> > > > > have no idea what it would show if there is a redundant CMC using
> > > > > different firmware.  This patch has worked for every Dell blade (10G,
> > > > > 11G, 12G) we have in an M1000e enclosure, including M1000e's with one
> > > > > and two CMC's.
> > > > >
> > > > > The version can be grabbed with: ipmi-raw 0 6 0x59 0 0xDF 1 0
> > > > > rcvd: 59 00 11 01 00 00 00 00 33 2E 30 33 00 00 00 00 00 00 00 00
> > > > >
> > > > > "3.03" in this case.  I have tried this with several 3.x and 4.x
> > > > > versions and the version numbers are all 4 characters long.
> > > > >
> > > > > As far as I can tell, there is no easy way to grab this through existing
> > > > > generic functions in ipmi-oem-dell.c.  Those functions appear to assume
> > > > > set selector and block selector 0, though the set selector is different
> > > > > in this case.  I had to decide to either reinvent the wheel somewhat or
> > > > > modify the other functions to take selectors as parameters instead of
> > > > > assuming 0.  I took the easy route and reinvented the wheel, though I'm
> > > > > open to changing that if needed.  I figured I should at least get some
> > > > > feedback before submitting a patch that changes a whole lot of code.
> > > > >
> > > > > This patch may need some reworking, but it has been tested successfully
> > > > > on: M600, M610, M610x, M910, and a pre-prod 12G system.  On a rackmount,
> > > > > it returns "dell:get-system-info 'cmc-firmware-version' option not
> > > > > supported on this system".
> > > > >
> > > > > Let me know if there's anything that needs to be changed and if you
> > > > > would prefer the slight duplication of code that I did versus adding set
> > > > > and block selector parameters to several functions.
> > > > >
> > > -- Albert Chu chu11@... Computer Scientist High Performance
> > > Systems Division Lawrence Livermore National Laboratory
> > > _______________________________________________ Freeipmi-devel
> > > mailing list Freeipmi-devel@...
> > > https://lists.gnu.org/mailman/listinfo/freeipmi-devel 
> >
> > --
> > Ryan Cox
> > Systems Administrator
> > Fulton Supercomputing Lab
> > Brigham Young University
> >
> >
> > _______________________________________________
> > Freeipmi-devel mailing list
> > Freeipmi-devel@...
> > https://lists.gnu.org/mailman/listinfo/freeipmi-devel
>
> --
> Ryan Cox
> Systems Administrator
> Fulton Supercomputing Lab
> Brigham Young University

--
Albert Chu
chu11@...
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory



_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel

[Ryan Cox]

Al,

I'll make those changes when I get a chance.  I assumed there was
something like this function but I couldn't locate it before. This
should be a much better approach than my original method.

Have a good Memorial Day weekend.

Ryan

On 05/21/2012 05:50 PM, Albert Chu wrote:

> Hey Ryan,
>
> Argh, I just realized something that I didn't come to me in my first
> review of the patch.  Why not use the call to:
>
> ipmi_cmd_get_system_info_parameters()
>
> instead of building the packet yourself and calling ipmi_cmd_raw()?
> That way you don't have to build up the packet like you were.
>
> If you follow the code in _output_dell_system_info_cmc_ipv6_info(),
> there should be a lot of similarities.  That would eliminate the call to
> ipmi_oem_check_response_and_completion_code() too, an appropriate error
> could be generated if the response is not long enough.  The comments on
> the OEM packet would be adjusted to describe the system info response
> instead of the actual packet payload.
>
> A few minor tweaks:
>
> +#define IPMI_OEM_DELL_CMC_FIRMWARE_VERSION 0xDF
>
> lets put this in
>
> libfreeipmi/include/freeipmi/spec/ipmi-system-info-parameters-oem-spec.h
>
> along with the other OEM system info parameters and rename
> appropriately.  Please also add a comment something like:
>
> "Ryan Cox: reverse engineered by walking through addresses.  Is correct
> name?"
>
> you'll see similar comments from me in there.
>
> +  assert (!state_data->prog_data->args->oem_options_count);
>
> I think this assert should be oem_options_count == 1?  If you
> run ./configure with --enable-debug, that will turn on the asserts and
> we'll see if we trigger it.
>
> BTW, if you could, put this together into just one patch.
>
> Thanks,
> Al
>
>
> On Mon, 2012-05-21 at 16:03 -0700, Ryan Cox wrote:
>> Al,
>>
>> Attached is a cleaned up patch that changes the function to static and
>> renames it.  The message format is now included in a format that
>> hopefully makes sense.  I tried making it look like the other
>> documentation in ipmi-oem-dell.c.  I changed the error message to
>> display if the completion code is non-zero.  I'm open to removing the
>> error handler if you still think it's unnecessary.
>>
>> A very simple patch to the manpage is also included.  All it does is
>> add cmc-firmware-version to the list of get-system-info subcommands.
>> The system support listed for get-system-info is still valid.
>>
>> Let me know if I should make any other changes.
>>
>> Ryan
>>
>> On 05/18/2012 12:53 PM, Ryan Cox wrote:
>>> Al,
>>>
>>> Thanks.  I did indeed have some problems with email that should now
>>> be fixed.
>>>
>>> I'll make the changes in the next few days.  You are correct that
>>> the reason for the error handling was to make a nicer error message.
>>> The alternative was to leave it as "dell:get-system-info failed: No
>>> error message found for command DFh, network function 06h, and
>>> completion code 80h.  Please report to<freeipmi-devel@...>".
>>>
>>> I'll be sure to add the response format in comments and an entry in
>>> the manpage.
>>>
>>> Ryan
>>>
>>> On 05/18/2012 12:41 PM, Albert Chu wrote:
>>>> This was my reply if it didn't get through your filters
>>>>
>>>> Al
>>>>
>>>> -- Albert Chu chu11@... Computer Scientist High Performance
>>>> Systems Division Lawrence Livermore National Laboratory
>>>>
>>>> Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info.eml
>>>> Subject:
>>>> Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info
>>>> cmc-firmware-version
>>>> From:
>>>> Albert Chu<chu11@...>
>>>> Date:
>>>> 05/18/2012 12:19 PM
>>>> To:
>>>> Ryan Cox<ryan_cox@...>
>>>> CC:
>>>> "freeipmi-devel@..."<freeipmi-devel@...>
>>>>
>>>> Hey Ryan,
>>>>
>>>> For the most part looks fine.  I assume the final patch will also clean
>>>> up the code comments to describe the CMC version packet :-)  Just a few
>>>> notes:
>>>>
>>>> +int
>>>> +ipmi_oem_dell_get_cmc_firmware_version (ipmi_oem_state_data_t
>>>> *state_data)
>>>>
>>>> since this is an internal function it should be static and not have an
>>>> entry in the header file.  For naming consistency, it should probably be
>>>> _get_dell_cmc_firmware_version() or something similar to the other
>>>> functions called by ipmi_oem_get_system_info().
>>>>
>>>> +  if(rs_len != 20) {
>>>> +      pstdout_fprintf (state_data->pstate,
>>>> +                       stderr,
>>>> +                       "dell:get-system-info 'cmc-firmware-version'
>>>> option not supported on this system\n");
>>>> +      goto cleanup;
>>>> +  }
>>>>
>>>> I don't think this check is really necessary, it'll be done via
>>>> ipmi_oem_check_response_and_completion_code().  I suppose you added it
>>>> for a nicer error message, but with ipmi-oem, I think there is a larger
>>>> acceptance of "you gotta know what you're doing" b/c it's so
>>>> OEM/motherboard/system/product specific (vs ipmi-sensors and more
>>>> standard tools).  The manpage should document the motherboards its
>>>> confirmed against or types of motherboards its expected to work with.
>>>>
>>>> Al
>>>>
>>>> On Fri, 2012-05-18 at 10:34 -0700, Ryan Cox wrote:
>>>>>> This patch adds "ipmi-oem dell get-system-info cmc-firmware-version".
>>>>>>   From a blade, it grabs the firmware version of the CMC for the
>>>>>> enclosure that the blade is in.  It appears to only show the active CMC
>>>>>> firmware and not show anything for the standby CMC (if there is one).  I
>>>>>> have no idea what it would show if there is a redundant CMC using
>>>>>> different firmware.  This patch has worked for every Dell blade (10G,
>>>>>> 11G, 12G) we have in an M1000e enclosure, including M1000e's with one
>>>>>> and two CMC's.
>>>>>>
>>>>>> The version can be grabbed with: ipmi-raw 0 6 0x59 0 0xDF 1 0
>>>>>> rcvd: 59 00 11 01 00 00 00 00 33 2E 30 33 00 00 00 00 00 00 00 00
>>>>>>
>>>>>> "3.03" in this case.  I have tried this with several 3.x and 4.x
>>>>>> versions and the version numbers are all 4 characters long.
>>>>>>
>>>>>> As far as I can tell, there is no easy way to grab this through existing
>>>>>> generic functions in ipmi-oem-dell.c.  Those functions appear to assume
>>>>>> set selector and block selector 0, though the set selector is different
>>>>>> in this case.  I had to decide to either reinvent the wheel somewhat or
>>>>>> modify the other functions to take selectors as parameters instead of
>>>>>> assuming 0.  I took the easy route and reinvented the wheel, though I'm
>>>>>> open to changing that if needed.  I figured I should at least get some
>>>>>> feedback before submitting a patch that changes a whole lot of code.
>>>>>>
>>>>>> This patch may need some reworking, but it has been tested successfully
>>>>>> on: M600, M610, M610x, M910, and a pre-prod 12G system.  On a rackmount,
>>>>>> it returns "dell:get-system-info 'cmc-firmware-version' option not
>>>>>> supported on this system".
>>>>>>
>>>>>> Let me know if there's anything that needs to be changed and if you
>>>>>> would prefer the slight duplication of code that I did versus adding set
>>>>>> and block selector parameters to several functions.
>>>>>>
>>>> -- Albert Chu chu11@... Computer Scientist High Performance
>>>> Systems Division Lawrence Livermore National Laboratory
>>>> _______________________________________________ Freeipmi-devel
>>>> mailing list Freeipmi-devel@...
>>>> https://lists.gnu.org/mailman/listinfo/freeipmi-devel
>>> --
>>> Ryan Cox
>>> Systems Administrator
>>> Fulton Supercomputing Lab
>>> Brigham Young University
>>>
>>>
>>> _______________________________________________
>>> Freeipmi-devel mailing list
>>> Freeipmi-devel@...
>>> https://lists.gnu.org/mailman/listinfo/freeipmi-devel
>> --
>> Ryan Cox
>> Systems Administrator
>> Fulton Supercomputing Lab
>> Brigham Young University

--
Ryan Cox
Systems Administrator
Fulton Supercomputing Lab
Brigham Young University


_______________________________________________
Freeipmi-devel mailing list
Freeipmi-devel@...
https://lists.gnu.org/mailman/listinfo/freeipmi-devel