Dmitry,
1.pps-bugfixes.diff
Looks good
2.pps-oem.diff
ipmi_oem.c patch failed to apply to TOB lib/ipmi_oem.c
The other changes look ok to me
3. pps-sol-instance.diff
I prefer accepting the patch that Corey Minyard proposed on 9/12/2012 instead
of this patch as Corey's patch implements the specification of the instance
number
within the sol activate and deactivate commands instead of a global
ipmitool switch. Since instance is specific to the sol commands, it seems more
appropriate that the instance specification be done within these commands.
I reviewed Corey's patch on 9/21, but never heard back from Corey on
my comments. The only real question I was waiting on from Corey before
accepting the patch was the question as to why there is a need for the
ability to specify the instance number other than the specification of one
that is done in the existing code.
4.pps-long-packets.diff
The lanplus.c patch failed to apply to TOB src/plugins/lanplus/lanplus.c
In ipmi_fru.c write_fru_area (~line 506), the fru_data_rqst_size is set to the
return value from ipmi_intf_get_max_request_data_size(intf) - 3 whereas
before it was the channel_buf_size - 5. I don't know exactly what the
overhead is nor why there is now less overhead needed than there was
before, can you explain a little as to the change? Also, if the
fru_data_rqst_size that gets calculated is greater than 255, you set it to
255; why doesn't the overhead need to be subtracted out of this?
You modified a compiled out version of write_fru_area on line 621 in ipmi_fru.c,
I think it would be better just to remove this function.
In ipmi_fru.c read_fru_area (line 728), why is the overhead here now one instead
of 9? Why doesn't the overhead need to be subtracted out of the 255? Pretty
much the same questions I had about write_fru_area above.
ipmi_hpmfwupg.c I don't know about the correctness of the functional changes
you are making, but I did see that you are doing a malloc on line 2294 and you
do
not check success/failure of the allocation.
ipmi_main.c
There needs to be man page documentation for all the switches you have added as
well as an explanation of the new serial interfaces and arguments required to
connect
using the serial interfaces.
It looks like you have to specific -D ttydev:baud to use the serial interfaces
and that the -X
switch is specific to these interfaces as well. How does one know if they
need to use the
-X switch? Might it be possible to package the serial driver separately from
the other
changes?
What platform did you test these serial interfaces on? I would like to see
the -r switch
functionality implemented in the fru command since it is specific to only fru
commands.
Why did you make the change to do the interface open even when the
target_address is
not specified? Could this cause any problems?
ipmi_oem.c changes are ok
ipmi_sdr.c Please use GET_SDR_ENTIRE_RECORD instead of the constant 0xFF on
line 2794 and
could you explain why you are setting sdr_max_read_len to 0xFE instead of
GET_SDR_ENTIRE_RECORD
on line 2975. sdr_max_read_len is initialized statically to
GET_SDR_ENTIRE_RECORD so isn't that
the correct maximum?
Why does serial_basic.c copyright mention Sun Microsystems in the text, but not
as a Copyright and
why does serial_terminal.c have a 2003 Sun Copyright? Where these files
derived from Sun source
code in some way? Neither hpm2.c nor hpm2.h references Sun Copyrights.
Thanks for the contributions,
Jim
-- Jim Mankovich |jm...@hp.com (MST) --
On 10/1/2012 10:24 PM, Dmitry Bazhenov wrote:
> Hello, Jim,
>
> Here are the updated patches which have your comments addressed. We decided
> not to push the changes into ipmi_sdr.c which regard to bridging commands to
> sensors since there is a major conflict between PICMG and generic IPMI
> systems in this matter.
>
> The patches are not dependent on each other (we merged patches 4 and 5
> together since serial drivers have strong dependence on large packet support
> implementation).
>
> The description for patches:
> 1. pps-bugfixes.diff:
> o lib/ipmi_sel.c
> - zero data before making request to avoid reading of uninitialized
> data
> o lib/ipmi_mc.c
> - do not print AUX info, if IPMC return 12-byte Get Device ID
> response
> o lib/ipmi_picmg.c
> - add support for hexadecimal and octal formats for the command-line
> parameters
> o configure.in
> - fixed build for the newer autotool releases
> - fixed cross-compilation
> 2. pps-oem.diff
> o Add support for PPS H8 evaluation board BMC which contains Intel
> 82751 GbE MAC. RMCP+ support for this MAC controller has several
> deviations.
> 3. pps-sol-instance.diff
> o Add support for SOL instances other than 1.
> 4. pps-long-packets.diff
> o Add support for HPM.2 compliant long packets. The implementation is
> made against the HPM.2 0.9.2 draft specification, and requires
> update for the HPM.2 1.0 release version. Pigeon Point Systems is
> going to provide the update later.
> o HPM.1 upgrade and FRU Inventory commands were modified to reuse the
> long packet support.
> o Access to FRU inventory devices with the length restrictions is
> made as a command-line option.
> o Kontron OEM large packet support is now optional and enabled by the
> corresponding OEM command-line parameter.
> o Add support for Terminal Mode and Basic Mode direct serial drivers.
>
> We are looking for your further comments.
>
> Regards,
> Dmitry
>
> 13.09.2012 16:14, Dmitry Bazhenov пишет:
>> Hello, Jim,
>>
>> Since both the "Get Channel Info" command and MC Device Locator record
>> are optional for generic IPMC, I think it is not worth trying to use
>> them to identify the cases when sensor command bridging should be
>> inhibited.
>>
>> Instead, I've started thinking of an explicit command-line parameter
>> which would signal that the command bridging (which is enabled by
>> default for backward compatibility) shall be inhibited.
>>
>> At this moment there are two options for this approach. The first is
>> using a global command-line option (assign a unused letter). The second
>> is using a command-specific flag for sensor-related commands ("sdr",
>> "sensor", maybe there are others).
>>
>> I would like to know which approach seems more attractive from the
>> maintainers point of view.
>>
>> Thanks in advance,
>> Dmitry
>>
>> 13.09.2012 14:39, Dmitry Bazhenov пишет:
>>> Hello, Jim,
>>>
>>> MC Device Locator is a mandatory record for PICMG systems. And this is
>>> where the slave address of the controller is got for comparison.
>>>
>>> For systems that do not provide MC Locators, the default IPMITool
>>> behavior (as it is currently implemented) could be retained.
>>>
>>> Regards,
>>> Dmitry
>>>
>>> 12.09.2012 20:00, Jim Mankovich пишет:
>>>> Hi Dmitry,
>>>>
>>>> Where does the slave address of the target IPMC on a PICMG system come
>>>> from that
>>>> you will be using to compare against the sensor owner to determine
>>>> bridging is not necessary?
>>>>
>>>> FYI: Not all platforms contain an MC Device Locator and some platforms
>>>> contain more than one.
>>>>
>>>> Thanks in Advance,
>>>> Jim
>>>>
>>>> -- Jim Mankovich | jm...@hp.com (MST) --
>>>>
>>>> On 9/10/2012 1:48 AM, Dmitry Bazhenov wrote:
>>>>> Hello, Jim,
>>>>>
>>>>> Thanks for you comments.
>>>>>
>>>>> The thing is that when doing "sdr" and "sensor" command against PICMG
>>>>> systems, IPMITool tries to bridge sensor commands to another channel
>>>>> (primary IPMB) instead of just sending them directly to IPMC.
>>>>>
>>>>> This is because PICMG IPMCs have slave addresses different from 20h on
>>>>> IPMB-0, but still have 20h for LAN access.
>>>>>
>>>>> Nevertheless, I agree that the proposed workaround might break the
>>>>> sensor reading for some useful cases.
>>>>>
>>>>> After some thinking I have come to a conclusion, that IPMITool shall
>>>>> read the Management Controller Device Locator record to get the IPMB-0
>>>>> slave address of the controller before sending sensor commands.
>>>>> If the slave address of the target IPMC is the same as the sensor
>>>>> owner, than bridging shall not be used, and vice versa.
>>>>>
>>>>> There should be also a restriction for the case when the target IPMC
>>>>> is already accessed via double bridging. In this case, if sensors
>>>>> reside on another IPMC and the access channel is not the same as to
>>>>> access target IPMC, they can be inaccessible since triple bridging is
>>>>> impossible. IPMITool shall also send Get Channel Info command to the
>>>>> target IPMC to check this.
>>>>>
>>>>> I'll come up with the updates soon.
>>>>>
>>>>> Regards,
>>>>> Dmitry
>>>>>
>>>>> 07.09.2012 22:29, Jim Mankovich пишет:
>>>>>> Dimitry,
>>>>>>
>>>>>> I took at look at your bugfix patch and found that it has a dependency
>>>>>> on your 4th patch. This needs to be resolved. Each of these
>>>>>> patches
>>>>>> should be able to be be applied independently, even if a later patch
>>>>>> depends
>>>>>> on a prior patch already having been applied.
>>>>>>
>>>>>> I would like to understand why you want to inhibit bridging sensor
>>>>>> requests
>>>>>> more than one level. You mention in your comment that it is a
>>>>>> workaround for
>>>>>> PICMG systems, but the change is not specific to PICMG systems. Won't
>>>>>> this
>>>>>> cause problems on systems that support more than a single level of
>>>>>> bridging?
>>>>>> I no expert on how IPMI bridging works, but the code as written
>>>>>> (before your
>>>>>> change) would support multiple levels of bridging.
>>>>>>
>>>>>> Also, I don't believe the save/restore logic you are using will work
>>>>>> correctly if you
>>>>>> are actually bridging to a different target address. Your logic
>>>>>> is as
>>>>>> follow:
>>>>>>
>>>>>> if (intf->target_addr == intf->my_addr) {
>>>>>> save_addr = intf->target_addr;
>>>>>> intf->target_addr = target;
>>>>>> save_channel = intf->target_channel;
>>>>>> intf->target_channel = channel;
>>>>>> }
>>>>>> ..... (sendrecv) ...
>>>>>>
>>>>>> if (intf->target_addr == intf->my_addr) {
>>>>>> intf->target_addr = save_addr;
>>>>>> intf->target_channel = save_channel;
>>>>>> }
>>>>>>
>>>>>> If the intf->target_addr is changed to a value different than
>>>>>> intf->my_addr in the
>>>>>> first if block, then the restore will never happen in the second if
>>>>>> block. This is because
>>>>>> when you get to the second if, intf->target_addr won't equal
>>>>>> intf->my_addr anymore so
>>>>>> the intf restore of target_addr/target_channel won't happen.
>>>>>>
>>>>>> -- Jim Mankovich |jm...@hp.com (MST) --
>>>>>>
>>>>>> On 9/7/2012 6:31 AM, Dmitry Bazhenov wrote:
>>>>>>> Hello, IPMITool maintainers,
>>>>>>>
>>>>>>> On behalf of Pigeon Point Systems company,
>>>>>>> I would like to contribute the following patches which contains
>>>>>>> IPMITool bug-fixes and some new functionality.
>>>>>>>
>>>>>>> Below is the description for the attached patches:
>>>>>>>
>>>>>>> 1. pps-bugfixes.diff:
>>>>>>> o lib/ipmi_sel.c
>>>>>>> - zero data before making request to avoid reading of
>>>>>>> uninitialized
>>>>>>> data
>>>>>>> o lib/ipmi_mc.c
>>>>>>> - do not print AUX info, if IPMC return 12-byte Get Device ID
>>>>>>> response
>>>>>>> o lib/ipmi_sdr.c
>>>>>>> - do not bridge sensor requests when interface bridging is
>>>>>>> already
>>>>>>> used. Is workaround for PICMG systems (ATCA/MicroTCA).
>>>>>>> o lib/ipmi_picmg.c
>>>>>>> - add support for hexadecimal and octal formats for the
>>>>>>> command-line
>>>>>>> parameters
>>>>>>> o configure.in
>>>>>>> - fixed build for the newer autotool releases
>>>>>>> - fixed cross-compilation
>>>>>>> 2. pps-oem.diff
>>>>>>> o Add support for PPS H8 evaluation board BMC which contains
>>>>>>> Intel
>>>>>>> 82751 GbE MAC. RMCP+ support for this MAC controller has
>>>>>>> several
>>>>>>> deviations.
>>>>>>> 3. pps-sol-instance.diff
>>>>>>> o Add support for SOL instances other than 1.
>>>>>>> 4. pps-long-packets.diff
>>>>>>> o Add support for HPM.2 compliant long packets. The
>>>>>>> implementation is
>>>>>>> made against the HPM.2 0.9.2 draft specification, and requires
>>>>>>> update for the HPM.2 1.0 release version. Pigeon Point
>>>>>>> Systems is
>>>>>>> going to provide the update later.
>>>>>>> o HPM.1 upgrade and FRU Inventory commands were modified to reuse
>>>>>>> the
>>>>>>> long packet support.
>>>>>>> o Access to FRU inventory devices with the length restrictions is
>>>>>>> made as a command-line option.
>>>>>>> o Kontron OEM large packet support is now optional and enabled by
>>>>>>> the
>>>>>>> corresponding OEM command-line parameter.
>>>>>>> 5. pps-serial-drivers.diff
>>>>>>> o Add support for Terminal Mode and Basic Mode direct serial
>>>>>>> drivers.
>>>>>>>
>>>>>>> The patches with greater numbers depend on patches with lesser
>>>>>>> numbers
>>>>>>> and must be applied in the ascending order.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ------------------------------------------------------------------------------
>>>>>>>
>>>>>>>
>>>>>>> Live Security Virtual Conference
>>>>>>> Exclusive live event will cover all the ways today's security and
>>>>>>> threat landscape has changed and how IT managers can respond.
>>>>>>> Discussions
>>>>>>> will include endpoint security, mobile security and the latest in
>>>>>>> malware
>>>>>>> threats.http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Ipmitool-devel mailing list
>>>>>>> Ipmitool-devel@lists.sourceforge.net
>>>>>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>>>>>
>>>>>
>>>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> Live Security Virtual Conference
>>> Exclusive live event will cover all the ways today's security and
>>> threat landscape has changed and how IT managers can respond. Discussions
>>> will include endpoint security, mobile security and the latest in malware
>>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
>>> _______________________________________________
>>> Ipmitool-devel mailing list
>>> Ipmitool-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
>>>
------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
