Corey Minyard wrote: >This is broken hardware/firmware, and this general workaround is not a >good idea. The trouble is that some systems don't have any IPMB bus, >and you need to correctly identify these systems. > > But the existing code assumes an IPMB on channel 0 when the device does not support the channel info command. If those systems don't have an IPMB, it apparently isn't that big a deal. Given that the standard says channel 0 is an IPMB, I didn't think it would be an issue.
>My first recommendation is to try to get the hardware vendor to fix the >problem, since it is their bug. You seem to have the BMC code, so the >fix should be there and a firmware upgrade should be able to fix the >problem. Barring that, this would need some type of blacklist type >thing based upon the manufacturer and product id. > > Well actually, I/We are the hardware vendor. And it's going to take time to find and fix this if it even is fixable. >But you should really fix the firmware, because other higher-level code >sends this command to know what channels are available. So it will >break other things besides the driver. > > That's just it. You see, the channel info command isn't broken and doesn't always fail. It's just that the BMC frequently replies with status 0xff which the standard says is "unspecified error" but if you retry the command several times or wait a bit, then it will succeed. But the current code sees the 0xff response, marks the channel unavailable and moves on. > linux-0w10:/usr/src/linux-2.6.16.11-7.ipmi # ipmitool channel info 0 > Channel 0x0 info: > Channel Medium Type : IPMB (I2C) > Channel Protocol Type : IPMB-1.0 > Session Support : session-less > Active Session Count : 0 > Protocol Vendor ID : 7154 I considered adding some kind of loop to retry on 0xff status, but I didn't want to retry infinitely and I couldn't come up with a method of retrying and limiting retries that I was happy with. My thought was to add two more variables to the intf structure to track the number of "channel info retries" and the maximum number of retries. I can work up a patch that implements this if you think that would be a better solution. >A few comments on the patch: > > * I don't like the change to send_channel_info_cmd(). That makes > the function useless for other purposes, and doesn't really save > anything. > > I don't see that it's useless for other purposes. You set the channel in the intf structure rather than as an argument to the function. You're going to have to set the "null_user_handler" structure member as well otherwise you won't get your reply. Why not set curr_channel at the same time? How do I tell what fields of the intf structure are specific to the interface and which are "semi-private" state holding variables? Is there any reason not to add additional fields to that structure (for the retry algorithm above)? > * Skipping channels 8-14 (or 8-15, really) is probably a good idea, > but scanning them is somewhat harmless. I'll think about that one. > > 14 in particular is an alias for "this channel" so it always answers with the information for what ever interface the request came in on. So that clearly is never a valid channel to "scan". You can choose to scan or not the others, but given the special behavior of channel 14, I thought it wiser to not scan reserved channels in case they had other "magical" properties in future standards. Thanks for the feedback, :v) -- Philip Pokorny, Director of Engineering Tel: 415-370-0835 Fax: 415-954-2899 Toll Free: 888-PENGUIN PENGUIN COMPUTING, INC. www.penguincomputing.com _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
