freeipmi-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Albert Chu
Subject: Re: [Freeipmi-devel] PATCH: ipmi-oem dell get-system-info cmc-firmware-version
Date: Fri, 18 May 2012 11:19:41 -0700

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
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory





reply via email to

[Prev in Thread] Current Thread [Next in Thread]