Re: [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 08/04/2017 08:18 PM, Brendan Higgins wrote:
This patchset introduces IPMI Block Transfer over I2C (BT-I2C), which has the
same semantics as IPMI Block Transfer except it done over I2C.

For the OpenBMC people, this is based on an RFC:
https://lists.ozlabs.org/pipermail/openbmc/2016-September/004505.html

The documentation discusses the reason for this in greater detail, suffice it to
say SSIF cannot be correctly implemented on some naive I2C devices. There are
some additional reasons why we don't like SSIF, but those are again covered in
the documentation for all those who are interested.

I'm not terribly excited about this.  A few notes:

SMBus alerts are fairly broken in Linux right now. I have a patch to fix this at:
https://github.com/cminyard/linux-ipmi/commit/48136176ce1890f99857c73e0ace5bd8dfb61fbf
I haven't been able to get much traction getting anyone to care.

The lack of a NACK could be worked around fairly easily in the current driver. It looks like you are just returning a message too short to be valid. That's easy. I think it's a rather major deficiency in the hardware to not be able to NACK something, but that is what it is.

What you have is not really BT over I2C. You have just added a sequence number to the IPMI messages and dropped the SMBus command. Other interfaces have sequence numbers,
too.  Calling it BT is a little over the top.

Do you really need the performance required by having multiple outstanding messages? That adds a lot of complexity, if it's unnecessary it's just a waste. The IPMI work on top of interfaces does not really require that much performance, it's just reading sensors,
FRU data, and such.  Perhaps you have a reason, but I fail to see
why it's really that big a deal. The BT interface has this ability, but the driver does not
take advantage of it and nobody has complained.

And I don't understand the part about OpenBMC making use of sequence numbers. Why does that matter for this interface? It's the host side that would care about that, the host would stick the numbers in and the slave would return it. If you are using sequence numbers in OpenBMC, which sounds quite reasonable, I would think it would be a bad idea to to trust that the host would give you good sequence
numbers.

Plus, with multiple outstanding messages, you really need to limit it. A particular BMC may not be able to handle it the full 256, and the ability to have that many messages
outstanding is probably not a good thing.

If you really need multiple outstanding messages, the host side IPMI message handler needs to change to allow that. It's doable, and I know how, I just haven't seen the
need.

I would agree that the multi-part messages in SSIF is a big pain and and a lot of unnecessary complexity. I believe it is there to accommodate host hardware that is limited. But SMBus can have 255 byte messages and there's no arbitrary limit on
I2C.  It is the way of IPMI to support the least common denominator.

Your interface will only work on Linux. Other OSes (unless they choose to implement this driver) will be unable to use your BMC. Of course there's the NACK issue, but that's a big difference, and it would probably still work with existing drivers on other OSes. Plus people may choose to use OpenBMC on things besides the aspeed devices that
could do a NACK.  That's kind of the point of OpenBMC, isn't it?

My suggestion would be to implement a BMC that supports standard SSIF single part messages by default. That way any existing driver will work with it. (I don't know about the NACK thing, but that's a much smaller issue than a whole new protocol.) Then use OEM extensions to query things like if it can do messages larger than 32
bytes or supports sequence numbers, and how many outstanding messages it
can have, and extend the current SSIF driver.  It wouldn't be very hard.

In my experience, sticking with standards is a good thing, and designing your own protocols is fraught with peril. I'm not trying to be difficult here, but I am against
the proliferation of things that do not need to proliferate :).

-corey


In addition, since I am adding both host side and BMC side support, I figured
that now is a good time to resolve the problem of where to put BMC side IPMI
drivers; right now we have it (there is only one) in drivers/char/ipmi/ with the
rest of the host side IPMI drivers, but I think it makes sense to put all of the
host side IPMI drivers in one directory and all of the BMC side drivers in
another, preferably in a way that does not effect all of the current OpenIPMI
users. I have not created a MAINTAINERS entry for the new directory yet, as I
figured there might be some discussion to be had about it.

I have tested this patchset on the Aspeed 2500 EVB.

Changes since previous update:
   - Cleaned up some documentation.
   - Added patch which moves the Aspeed BT-BMC driver to the new ipmi_bmc
     directory.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux