freeipmi-devel
[Top][All Lists]
Advanced

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

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


From: Ryan Cox
Subject: Re: [Freeipmi-devel] [Fwd: Re: PATCH: ipmi-oem dell get-system-info cmc-firmware-version]
Date: Fri, 18 May 2012 12:53:50 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1

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 <address@hidden>".

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 address@hidden 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 <address@hidden>
Date:
05/18/2012 12:19 PM
To:
Ryan Cox <address@hidden>
CC:
"address@hidden" <address@hidden>

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

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

reply via email to

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