On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard <minyard@xxxxxxx> wrote: > 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: I was afraid so, alas. > > 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. Yeah, I have some work I would like to do there as well. > > 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. Right, we actually have multiple pieces of hardware that do not support NACKing correctly. The most frustrating piece is the Aspeed chip which does not provide and facility for arbitrary NACKs to be generated on the slave side. > > 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. Fair point, maybe ISIF (I2C System Interface)? I don't have strong feelings about the name. > > 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. > Yes, we do have some platforms which only have IPMI as a standard interface and we are abusing some OEM commands to do some things that we probably should not do with IPMI like doing firmware updates. Also, we have some commands which take a really long time to complete (> 1s). Admittedly, this is abusing IPMI to solve problems which should probably be solved elsewhere; nevertheless, it is a feature we are actually using. And having an option to use sequence numbers if definitely nice from our perspective. We will probably want to improve the block transfer driver at some point as well. > 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. > I think, I illustrated the use case above, but to reiterate, the desire is to have multiple messages in flight at the same time because some messages take a long time to service. > 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. > It is going to depend on the BMC of course; nevertheless, I would be willing to implement a configurable limit. > 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. > Sure, we would also like SMBus alert support, but I figured it was probably best to discuss this with you before we go too far down that path. > 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. I hope I did not send the message that we are planning on using this instead of SSIF forever and always. This is intended as an alternative to SSIF for some systems for which I2C is really the only available hardware interface, but SSIF is not well suited for said reasons. It would be great for OpenBMC if someone added support for SSIF, but as far as I know, no one is asking for it. > 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? Yes, using OpenBMC on other things is part of the point; however, all of the projects that I am currently aware of and am trying to support use Aspeed parts which also need an option. > > 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.) Coming up with a satisfactory solution to working around systems that cannot NACK is my primary goal; I don't care about alternatives that do not address this. > 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. It would not be that hard to implement something that is naive, but I think it would be difficult to implement something that is maintainable and robust. For example, if I added an OEM extension that allowed me to add a sequence number, I would have to find a place that I could put it that would allow IPMI messages that do not have the sequence number to still be sent and parsed correctly so that if either the host or BMC reset the other would still be able to send the message to return it to the desired state. I can think of ways that this could be done, but I cannot think of anyway that it could be done cleanly and would not make maintaining SSIF more complicated. > > 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 :). I totally agree, which is why I tried to do something which looks like an existing IPMI interface to all of the software outside of this. As I mentioned above, I am afraid that hacking up SSIF to achieve the desired effects would make it much harder to maintain. I figured that the stuff I was trying to do does not really look like SSIF, so I should not force it into SSIF. If we don't advertise it as SSIF, then no one will expect it to behave that way. Moreover, if we put all of the hacky stuff in its own driver then we don't have to worry about it forever polluting SSIF; whereas, if we modify SSIF to support whatever weird things we suggest, it becomes a lot harder go back and clean it up if one day we find that no one is using it, which I do indeed hope becomes the case. I guess that I would say that sticking with standards is a good thing, but if you are going to break them, you must not make anything more difficult for the people who are following them. All this being said, we do not terribly mind keeping these patches internal, at least for sometime (by then maybe we will have a better solution). The main reason I wanted to upstream these is twofold: I heard that there are some other people who ran into this problem and were interested in a solution of this type and I thought this would be a good motivating example behind a new BMC side IPMI framework that we are working on as it would help to illustrate the desire to separate the hardware layer from the common routing layer as you had done with the host side; however, as long as you are aware that there may be other implementations, just possibly not in the mainline kernel, then I suppose that is not really an issue. I will try to send out an RFC for this framework tonight. > > -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. Regardless of what we do with the "BT-I2C" stuff, I am still interested in what you think about this. >> >> 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. > > > Thanks -- 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
