Message-ID: <9c0aef7c-6dcc-2328-f712-d35ef81ebc41@gmail.com>
Date:   Wed, 9 Aug 2017 21:26:40 -0500
From:   Corey Minyard <tcminyard@...il.com>
To:     Brendan Higgins <brendanhiggins@...gle.com>,
        Corey Minyard <minyard@....org>
Cc:     Jonathan Corbet <corbet@....net>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Joel Stanley <joel@....id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Benjamin Fair <benjaminfair@...gle.com>,
        linux-doc@...r.kernel.org, devicetree <devicetree@...r.kernel.org>,
        openipmi-developer@...ts.sourceforge.net,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C

On 08/09/2017 08:04 PM, Brendan Higgins wrote:
>> Perhaps that is some level of abuse, but it's pretty common.  I'm not
>> against it.
>>
>> There is standard IPMI firmware NetFN (though no commands defined) that if
>> you use
>> the driver automatically goes into "Maintenance mode" and modified the
>> timeouts
>> and handling to some extent to help with this.
> That is a really good point, I missed that.
> ...
>>
>> There are ways to accomplish this that aren't that complex.  You can create
>> an OEM
>> command that can query the maximum message size and the ability to do
>> sequence
>> numbers in the messages.
>>
>> If messages larger than 32-bytes are supported, and the host I2C/SMBus
>> driver
>> supports it, you could use the standard SSIF SMBus commands to do this, they
>> have an 8-bit length field.
>>
>> If sequence numbers are supported, The SSIF could use different SMBus
>> commands
>> to do the write and read requests.  Since this is only if you get an OEM
>> command,
>> and if you put the sequence numbers at the end where they are easy to add on
>> the send side, this is a small change to the driver.
> What if we just had an OEM command that changed the message structure from
> that point on? We could abuse the "maintenance mode" NetFN to get back into
> normal SSIF if necessary.

Actually, I wouldn't have a separate "openbmc mode".  I would have 
OpenBMC always
work with standard SSIF, and have separate SMBus commands for messages with
the sequence number and messages larger than 32 bytes.

I've attached a patch with what I would expect the changes to be to the 
host driver.
It doesn't handle multiple outstanding messages, but it shows what 
detection and a
separate SMBus command would look like.

>> So I think the changes would be small and contained.  I'm actually ok with a
>> different driver, but I think it would be more valuable to the OpenBMC
>> project
>> to have a standardized interface that would work (in a not quite as
>> efficient
>> mode) with software that does not use the Linux IPMI driver.
> I guess I see the all of my asks as hacky things which we can hopefully remove
> at some point. Hopefully, most OpenBMC users won't want or need these things.
> ...
>>> Regardless of what we do with the "BT-I2C" stuff, I am still interested in
>>> what
>>> you think about this.
>>
>> I think you are right, it probably belongs some place else.  The way that
>> makes the most
>> sense to me would be to have an "ipmi" directory with a "host" and "slave"
>> side, and since
>> ipmi is not really a char driver, to move it to the main driver directory.
>> That might be
>> fairly disruptive, though.
> That was my thinking exactly.
>
>> The other option that makes sense to me would be to add a
>> drivers/char/ipmi_slave directory,
>> or something like that, and put the slave code there.  That would be less
>> disruptive.
> Right that is the approach I took, except I called it drivers/char/ipmi_bmc.
>
> I originally thought doing the less disruptive thing is best; however, I know
> there are also some OpenBMC people who are interested in implementing
> IPMB. So maybe now is the time to bite the bullet and create an ipmi
> directory under drivers/.

I'm not sure IPMB would make much difference, there's no host side 
change as it's
already supported.  I don't think there would be any significant code 
sharing
between the two.

If there end up being a significant amount of common code, then it would
definitely be worth the effort to move it.

>> -corey
> In summary, I think I can live with making it a mangled form of SSIF, but
> I would prefer to put it in its own driver.

You can look at the patch and consider it, and consider that you would 
need to
implement flag and event handling.  On an x86 host there would be SMBIOS
and ACPI stuff to deal with somehow for discovery.  There's probably few 
other
things to deal with.

> In any case, I think I would rather focus on the the BMC side IPMI framework
> now, since it is a bigger change and would also reduce the work of
> implementing a BMC side SSIF driver.
>
> Here is what I propose: we focus on the BMC side IPMI framework RFC that
> I sent out the other day:
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1463473.html
> I will add a change to the BMC side IPMI framework patchset to move all the
> IPMI stuff to the new drivers/ipmi directory as discussed and then drop the
> patch in that patchset that depends on this patchset.
>
> Let me know what you think

Let's hold off on the new directory, there's probably some convincing of 
the "powers
that be" for that.

I'll look at the patch set tomorrow, unless something critical comes up.

Thanks,

-corey


View attachment "ssif-openbmc.patch" of type "text/x-patch" (4843 bytes)

Powered by blists - more mailing lists