Re: [Freeipmi-devel] [RFC][PATCH] freeipmi: Special calxeda commands ported from ipmitools

[Thomas Renninger]

On Thursday, August 08, 2013 03:54:13 PM Albert Chu wrote:
> Hi Thomas,
> 
> Thanks for the patch.  I saw this patch fly by on the ipmitool mailing
> list.  I looked at it briefly but realized it'd be quite a bit of work
> to get ported over to FreeIPMI.  Also, since I don't have docs for
> Calxeda OEM extensions, it'd be harder for me todo.  Thanks for taking
> the first steps.
> 
> > - What still has to be done to get this accepted mainline?
> 
> I think the main goal is to "FreeIPMI-ize" the code so it's more
> consistent with the rest of FreeIPMI code.
Disadvantage is that the code diverges.
Maybe a way in the middle could be found.

> We'll probably have to
> iterate a number of times and I'll have to slowly learn some of this
> code too.
Sounds sane.

> I didn't look at this patch super-close.  But here are some
> immediate/obvious things that I noticed:
> 
> 1) Appropriate manpage documentation should be written up for all the
>    OEM extensions offered.
The syntax is the same as in ipmitools, but I couldn't find any docs there
for the cxoem command as well.
This would more be something for someone who knows the bits in detail,
I simply ported the code...
 
> 2) lprintf should be replaced w/ calls to pstdout_printf or
>    pstdout_fprintf as appropriate (most notably pstdout_printf for
>    good output, pstdout_fprintf to stderr for errors).
I can do that.
 
> 3) I don't believe the "help" option is needed, as ipmi-oem already
>    has something like this constructed into its architecture.  For
>    example, what options are there for Dell.
Yep.
 
> 4) Call ipmi_cmd_raw directly, no need to have ipmitool wrappers.  It
>    seems this was already done in cx_fw_download(), perhaps as a
>    test??  Then of course call appropriate error checks like
> ipmi_oem_check_response_and_completion_code().
Hm, it may be a good idea to keep the wrapper for easier diffing and
lower maintenance overhead with ipmitools code

ipmi_oem_check_response_and_completion_code()
would still need to be added to the wrapper.

Would that be acceptable?

> 5)
> 
> +struct cx_fw_info_rs {
> +     unsigned char ver;      /* param version */
> +     unsigned char count;    /* number of bytes */
> +     img_info_t img_info;
> +} __attribute__ ((packed));
> 
> I'd prefer the use of FreeIPMI "fiid" templates or just using buffers,
> as "__attribute__ ((packed))" isn't portable across all many
> compilers.  We can talk about this in more detail if you feel that the
> structs are a deal breaker.
I have to look deeper into this.
 
> 6)
> 
> A lot of the macros in freeipmi-1.2.9/ipmi-oem/_ipmi-oem-calxeda.h
> should be placed in their appropriate header files in
> libfreeipmi/include/freeipmi/spec.  For example OEM commands in
> ipmi-cmd-oem-spec.h and net fn's in ipmi-netfn-oem-spec.h.  For many
> random macros that are needed by OEM commands, they should go in
> ipmi-oem-spec.h.
Ok.
 
> 7)
> 
> If you look at most of the ipmi-oem code, you'll see comments like
> this throughout:
> 
>   /* Supermicro OEM
>    *
>    * Request
>    *
>    * 0x30 - OEM network function
>    * 0x70 - OEM cmd
>    * 0xF0 - Sub-command
>    * 0x?? - action
>    *      - 0x00 - disable
>    *      - 0x01 - enable
>    *      - 0x02 - status
>    *
>    * Response
>    *
>    * 0x70 - OEM cmd
>    * 0x?? - Completion Code
>    * 0x?? - if action == status
>    *      - 0x00 - disabled
>    *      - 0x01 - enabled
>    */
> 
> I think it's important to document each of the OEM extensions in as
> much detail as possible to make things easier for myself and future
> maintainers.  Unlike the IPMI spec, we don't want to reach the point
> where the code is the only documentation, b/c alot of it can be
> confusing.  For example:
> 
> +       req.msg.netfn = IPMI_NETFN_OEM_SS;
> +       req.msg.cmd = IPMI_CMD_OEM_FW_DOWNLOAD;
> +       msg_data[0] = type;
> +       msg_data[1] = partition;
> +       msg_data[2] = CXOEM_FW_UPLOAD;
> +       msg_data[3] = 0;
> +       msg_data[4] = 0;
> +       msg_data[5] = 6;        // ipv4 addresses by default (for now)
> 
> Why are msg_data[3-4] set to 0?  Why is msg_data[5] set to 6??  I have
> no idea why.
Nothing for me. Hopefully Calxeda guys can add something.
Sounds worth for both ipmitools and freeipmi and shouldn't be a big deal
to push it for both (once the stuff is in a mainline branch?).
 
> There were some nits I found along the way, but they can be discussed
> later.
> 
> Not sure how you all want to proceed.  Perhaps your side wants to try
> 1-2 clean ups as a first attempt, then I can apply to a FreeIPMI branch
> and we can iterate on that?

Sounds like a good idea.
Be aware that I will not maintain this code.
I can help a bit getting it cleaner, but either calxeda guys have to help
or community picks up, once some bugs/issues are found.

Thanks a lot for the quick answer,

        Thomas

 
> Al
> 
> On Thu, 2013-08-08 at 15:33 +0200, Thomas Renninger wrote:
> > this is more or less a one to one copy from:
> > http://sources.calxeda.com/gitweb/?p=ipmitool.git;a=summary
> > file: lib/ipmi_cxoem.c
> > implementing calxeda specific IPMI commands.
> > 
> > The entry points have been adjusted to freeipmi style and ipmitool
> > specific
> > coding style (struct ipmi_intf, intf->sendrecv(..)) have been put into
> > small wrapper functions.
> > Goal was to not have a perfectly correct coding style, but to show a prove
> > of concept, that it's not that hard to get this functionality into
> > freeipmi.
> > 
> > I wonder:
> > - What still has to be done to get this accepted mainline?
> > - Could Calxeda guys take over some testing? I have no idea about most
> > 
> >   of the commands.
> > 
> > - Could Calxeda provide some maintenance? If a bug in ipmitool is
> > discovered,> 
> >   the fix should probably also be pushed into freeipmi (or vice versa), as
> >   the code is more or less the same.
> > 
> > Currently all commands are added:
> > ipmi-oem calxeda
> > Calxeda Command: help
> > Calxeda Command: fw
> > Calxeda Command: fabric
> > Calxeda Command: mac
> > Calxeda Command: log
> > Calxeda Command: data
> > Calxeda Command: info
> > Calxeda Command: feature
> > 
> > but only some are implemented (the others would need some more adjusting
> > of
> > ipmitool specific commands used, but I doubt it would be that hard):
> > 
> > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda fw activate 10
> > Partition         : 10
> > activate: write flags <fffdfff8>
> > 
> > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda data mem read 4 100000
> > xint Length   : 4
> > Addr     : 00100000
> > Value    : 0xdd0000ea
> > 
> > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda feature status mansen
> > 
> >    mansen is enabled
> > 
> > ./ipmi-oem -h 10.121.39.1 -u admin -p admin calxeda fabric get macaddr
> > node 0 interface 1 fc:2f:40:0d:9f:11
> > 
> > ./ipmi-oem calxeda log
> > ./ipmi-oem calxeda mac
> > ./ipmi-oem calxeda info
> > log command not implemented
> > 
> > Comments?
> > 
> > Thanks,
> > 
> >       Thomas
> >