Message-ID: <8f5579cb-2c4d-2cd0-e874-a44374e08417@acm.org>
Date:   Sat, 5 Aug 2017 17:23:06 -0500
From:   Corey Minyard <minyard@....org>
To:     Brendan Higgins <brendanhiggins@...gle.com>, corbet@....net,
        robh+dt@...nel.org, mark.rutland@....com, arnd@...db.de,
        gregkh@...uxfoundation.org, joel@....id.au,
        benh@...nel.crashing.org, benjaminfair@...gle.com
Cc:     linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
        openipmi-developer@...ts.sourceforge.net, openbmc@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C

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.

Powered by blists - more mailing lists