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

[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<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

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