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

[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 <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
> > 
> > 
> > _______________________________________________
> > Freeipmi-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/freeipmi-devel
> 
> -- 
> Ryan Cox
> Systems Administrator
> Fulton Supercomputing Lab
> Brigham Young University
-- 
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory