Re: [Openipmi-developer] [RFC v1 0/4] ipmi_bmc: framework for IPMI on BMCs

2017-08-14 Thread Brendan Higgins via Openipmi-developer 
On Mon, Aug 14, 2017 at 7:03 PM, Patrick Williams  wrote:
> On Mon, Aug 07, 2017 at 08:52:57PM -0700, Brendan Higgins wrote:
>> Currently, OpenBMC handles all IPMI message routing and handling in userland;
>> the existing drivers simply provide a file interface for the hardware on the
>> device. In this patchset, we propose a common file interface to be shared by 
>> all
>> IPMI hardware interfaces, but also a framework for implementing handlers at 
>> the
>> kernel level, similar to how the existing OpenIPMI framework supports both
>> kernel users, as well as misc device file interface.
>
> Brendan,
>
> Can you expand on why this is a good thing from an OpenBMC perspective?

Sure, so in addition to the individual handlers; this does introduce a
common file
system interface for BMC side IPMI hardware interfaces. I think that is pretty
straightforward.

Corey and I are still exploring the handlers. My original intention
was not to replace
any of the handlers implemented in userspace. My motivating use case is for some
OEM commands that would be easier to implement inside of the kernel.

I was hoping to send out an overview of that, but the internet in my
hotel sucks,
so I will do it the next time I get decent internet access. :-P

In any case, Corey raised some interesting points on the subject; the
most recent
round I have not responded to yet.

> We have a pretty significant set of IPMI providers that run in the
> userspace daemon(s) and I can't picture more than a very small subset
> even being possible to run in kernel space without userspace assistance.

Like I said, I have an example of some OEM commands. Also, as I have said,
my intention is not to replace any of the userland stuff. That being said, I am
not sure the approach we have taken so far is the best when it comes to some
of the new protocols we are looking at like IPMB and MCTP. Having some
consistency of where we draw these interface boundaries would be nice; so
maybe that means rethinking some of that. I don't know, but it sounds like
Corey has already tried some of this stuff out on his own BMC side
implementation.

Regardless, I think there is a lot of interesting conversation to be had.

> We also already have an implementation of a RMCP+ daemon that can, and
> does, share most of its providers with the host-side daemon.

That's great. Like I said, my original intention was not to rewrite any of that.

>
> --
> Patrick Williams

By the way, Corey suggested that we have a BoF session at the Linux Plumbers
Conference, so I set one up:
https://linuxplumbersconf.org/2017/ocw/proposals/4723
I highly encourage anyone who is interested in this discussion to attend.

Thanks!

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

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

2017-08-09 Thread Brendan Higgins via Openipmi-developer 
On Wed, Aug 9, 2017 at 7:26 PM, Corey Minyard  wrote:
> 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.

I took a look at the patch, it seems reasonable. If I was maintaining
SSIF, I probably
would not want that kind of clutter for my admittedly weird use case,
but if you're
okay with it, then so am I.

>
>
>>> 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.

No, I don't expect much code sharing between them. I just thought it would be a
reasonable place to put IPMB, sort of like how we have a bunch of "character"
device drivers in drivers/char, but I suppose that might be somewhat of an
anti-pattern ;-)

>
> 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:
>>

Re: [Openipmi-developer] [RFC v1 1/4] ipmi_bmc: framework for BT IPMI on BMCs

2017-08-10 Thread Brendan Higgins via Openipmi-developer 
On Thu, Aug 10, 2017 at 6:58 AM, Corey Minyard  wrote:
> On 08/07/2017 10:52 PM, Brendan Higgins wrote:
>>
>> From: Benjamin Fair 
>>
>> This patch introduces a framework for writing IPMI drivers which run on
>> a Board Management Controller. It is similar in function to OpenIPMI.
>> The framework handles registering devices and routing messages.
>
>
> Ok, I think I understand this.  I have a few nitpicky comments inline.  The
> RCU usage
> looks correct, and the basic design seems sound.

Sweet

>
> This piece of code takes a communication interface, called a bus, and
> muxes/demuxes
> messages on that bus to various users, called devices.  The name "devices"
> confused
> me for a bit, because I was thinking they were physical devices, what Linux
> would
> call a device.  I don't have a good suggestion for another name, though.

We could maybe do "*_interface" instead of "*_bus" and "*_handler" instead
of "*_device"; admittedly, it is not the best name ever: handler has some
connotations.

>
>
>
> I assume you would create one of these per bus for handling multiple busses,
> which
> you will obviously need to do in the future when IPMB comes.

Yep, that is the intention. I was planning on adding support to use the device
infrastructure so that multiple busses could be declared using device tree, etc.
I do not have that now, but I thought that was a lot of work and I would want
to get general buy-in before doing that. We just did enough to make code
that achieves feature parity to what we have now and the core of the new
proposed features.

>
> I can see two big problems with the way the "match_request" is done:
> * If multiple users want to handle the same request, only one of them will
> get it
>   and they will not know they have conflicted.
> * Doing this for userland interfaces will be hard.
> The other way to do this is for each user to register for each request type
> and
> manage it all in this code, which is kind of messy to use, but avoids the
> above problems.

Right, we considered this; however, I thought this is best because we also
want to handle routing of OEM messages; I suppose we could register a
type for OEM messages and then have a secondary set of handlers there;
this would have some drawbacks though, the default handler interface would
become a lot more complicated.

On the other hand, now that I am thinking about it; having a common kernel
interface for OEM messages could be really useful; this has definitely
caused us some grief.

>
> In thinking about the bigger picture here, you will need something like this
> for every
> communication interface the BMC supports: the system interface, LAN, IPMB,
> ICMB
> (let's hope not), and serial/modem interfaces (let's hope not, too, but
> people really
> use these in the field).  Maybe ICMB and serial aren't on your radar, but
> I'd expect
> LAN is pretty important, and you have already mentioned IPMB.

Right, we are thinking about IPMB right now; I agree that the other stuff should
be considered, but we don't really have a need for it now.

My hope would be to make a similar interface for IPMB.

>
> If you are thinking you will have a separate one of these for LAN in
> userspace, I
> would say just do it all in userspace.  For LAN you will have something that
> has
> to mux/demux all the messages from the LAN interface to the various users,
> the
> same code could sit on top of the current BT interface (and IPMB, etc.).

So right now we do have handlers for a lot of basic commands in user space;
this is why I have the default device file interface. However, it is incomplete
and I am starting to look at commands that make more sense being
implemented in the kernel.

I have kind of danced around your point so far, for LAN, as far as I know we
only support a REST interface right now; we should have the option of the
LAN interface, but I don't think anyone has really tackled that yet.

Basically, the way I see this now is sort of a mirror of what is done
on the host
side, we will have a kernel and a userland interface and then we will evolve it
as necessary.

In any case, I think there is probably a lot of room for additional discussion
here.

>
> I guess I'm trying to figure out how you expect all this work out in the
> end.  What
> you have now is a message mux/demux that can only have on thing underneath
> it and one thing above it, which obviously isn't very useful.  Are you
> thinking you
> can have other in-kernel things that can handle specific messages? I'm
> having
> a hard time imagining that's the case.  Or are you thinking that you will

Yes, I as I mentioned above, having handlers in the kernel is a prime motivator
for this. I have been working on an interface for doing flash updates over IPMI.
IPMI is not really a great way to do this in terms of performance or
the interface
it provides; however, IPMI is great in the sense that all platforms
have it, so I
want to have alternative backends

[Openipmi-developer] [PATCH v2 2/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C host side

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
The IPMI definition of the Block Transfer protocol defines the hardware
registers and behavior in addition to the message format and messaging
semantics. This implements a new protocol that uses IPMI Block Transfer
messages and semantics on top of a standard I2C interface.

Signed-off-by: Brendan Higgins 
---
Changes for v2:
  - None
---
 drivers/char/ipmi/Kconfig   |   4 +
 drivers/char/ipmi/Makefile  |   1 +
 drivers/char/ipmi/ipmi_bt_i2c.c | 452 
 3 files changed, 457 insertions(+)
 create mode 100644 drivers/char/ipmi/ipmi_bt_i2c.c

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index f6fa056a52fc..a8734a369cb0 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -79,6 +79,10 @@ config IPMI_POWEROFF
  This enables a function to power off the system with IPMI if
 the IPMI management controller is capable of this.
 
+config IPMI_BT_I2C
+   select I2C
+   tristate 'BT IPMI bmc driver over I2c'
+
 endif # IPMI_HANDLER
 
 config ASPEED_BT_IPMI_BMC
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index eefb0b301e83..323de0b0b8b5 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -12,4 +12,5 @@ obj-$(CONFIG_IPMI_SSIF) += ipmi_ssif.o
 obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
 obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
 obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
+obj-$(CONFIG_IPMI_BT_I2C) += ipmi_bt_i2c.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
diff --git a/drivers/char/ipmi/ipmi_bt_i2c.c b/drivers/char/ipmi/ipmi_bt_i2c.c
new file mode 100644
index ..94b5c11d23cd
--- /dev/null
+++ b/drivers/char/ipmi/ipmi_bt_i2c.c
@@ -0,0 +1,452 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt)"ipmi-bt-i2c: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define IPMI_BT_I2C_TIMEOUT (msecs_to_jiffies(1000))
+
+/* If we don't have netfn_lun, seq, and cmd, we might as well have nothing. */
+#define IPMI_BT_I2C_LEN_MIN 3
+/* We need at least netfn_lun, seq, cmd, and completion. */
+#define IPMI_BT_I2C_RESPONSE_LEN_MIN 4
+#define IPMI_BT_I2C_MSG_MAX_PAYLOAD_SIZE 252
+
+struct ipmi_bt_i2c_msg {
+   u8 len;
+   u8 netfn_lun;
+   u8 seq;
+   u8 cmd;
+   u8 payload[IPMI_BT_I2C_MSG_MAX_PAYLOAD_SIZE];
+} __packed;
+
+#define IPMI_BT_I2C_MAX_SMI_SIZE 254 /* Need extra byte for seq. */
+#define IPMI_BT_I2C_SMI_MSG_HEADER_SIZE 2
+
+struct ipmi_bt_i2c_smi_msg {
+   u8 netfn_lun;
+   u8 cmd;
+   u8 payload[IPMI_MAX_MSG_LENGTH - 2];
+} __packed;
+
+static inline u32 bt_msg_len(struct ipmi_bt_i2c_msg *bt_request)
+{
+   return bt_request->len + 1;
+}
+
+#define IPMI_BT_I2C_SEQ_MAX 256
+
+struct ipmi_bt_i2c_seq_entry {
+   struct ipmi_smi_msg *msg;
+   unsigned long   send_time;
+};
+
+struct ipmi_bt_i2c_master {
+   struct ipmi_device_id   ipmi_id;
+   struct i2c_client   *client;
+   ipmi_smi_t  intf;
+   spinlock_t  lock;
+   struct ipmi_bt_i2c_seq_entryseq_msg_map[IPMI_BT_I2C_SEQ_MAX];
+   struct work_struct  ipmi_bt_i2c_recv_work;
+   struct work_struct  ipmi_bt_i2c_send_work;
+   struct ipmi_smi_msg *msg_to_send;
+};
+
+static const unsigned long write_timeout = 25;
+
+static int ipmi_bt_i2c_send_request(struct ipmi_bt_i2c_master *master,
+   struct ipmi_bt_i2c_msg *request)
+{
+   struct i2c_client *client = master->client;
+   unsigned long timeout, read_time;
+   u8 *buf = (u8 *) request;
+   int ret;
+
+   timeout = jiffies + msecs_to_jiffies(write_timeout);
+   do {
+   read_time = jiffies;
+   ret = i2c_master_send(client, buf, bt_msg_len(request));
+   if (ret >= 0)
+   return 0;
+   usleep_range(1000, 1500);
+   } while (time_before(read_time, timeout));
+   return ret;
+}
+
+static int ipmi_bt_i2c_receive_response(struct ipmi_bt_i2c_master *master,
+   struct ipmi_bt_i2c_msg *response)
+{
+   struct i2c_client *client = master->client;
+   unsigned long timeout, read_time;
+   u8 *buf = (u8 *) response;
+   u8 len = 0;
+   int ret;
+
+   /*
+* Slave may not NACK when

[Openipmi-developer] [PATCH v2 3/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C BMC side

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
The IPMI definition of the Block Transfer protocol defines the hardware
registers and behavior in addition to the message format and messaging
semantics. This implements a new protocol that uses IPMI Block Transfer
messages and semantics on top of a standard I2C interface. This protocol
has the same BMC side file system interface as "ipmi-bt-host".

Signed-off-by: Brendan Higgins 
---
Changes for v2:
  - None
---
 drivers/char/Kconfig|   1 +
 drivers/char/Makefile   |   1 +
 drivers/char/ipmi_bmc/Kconfig   |  22 ++
 drivers/char/ipmi_bmc/Makefile  |   5 +
 drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c | 346 
 include/linux/ipmi_bmc.h|  76 +++
 6 files changed, 451 insertions(+)
 create mode 100644 drivers/char/ipmi_bmc/Kconfig
 create mode 100644 drivers/char/ipmi_bmc/Makefile
 create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
 create mode 100644 include/linux/ipmi_bmc.h

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index ccd239ab879f..2a6ca2325a45 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -195,6 +195,7 @@ config POWERNV_OP_PANEL
  If unsure, say M here to build it as a module called powernv-op-panel.
 
 source "drivers/char/ipmi/Kconfig"
+source "drivers/char/ipmi_bmc/Kconfig"
 
 config DS1620
tristate "NetWinder thermometer support"
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 53e33720818c..9e143186fa30 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -58,4 +58,5 @@ js-rtc-y = rtc.o
 
 obj-$(CONFIG_TILE_SROM)+= tile-srom.o
 obj-$(CONFIG_XILLYBUS) += xillybus/
+obj-$(CONFIG_IPMI_BMC) += ipmi_bmc/
 obj-$(CONFIG_POWERNV_OP_PANEL) += powernv-op-panel.o
diff --git a/drivers/char/ipmi_bmc/Kconfig b/drivers/char/ipmi_bmc/Kconfig
new file mode 100644
index ..26c8e0cb765c
--- /dev/null
+++ b/drivers/char/ipmi_bmc/Kconfig
@@ -0,0 +1,22 @@
+#
+# IPMI BMC configuration
+#
+
+menuconfig IPMI_BMC
+   tristate 'IPMI BMC core'
+   help
+ This enables the BMC-side IPMI drivers.
+
+ If unsure, say N.
+
+if IPMI_BMC
+
+config IPMI_BMC_BT_I2C
+   depends on I2C
+   select I2C_SLAVE
+   tristate 'Generic I2C BT IPMI BMC driver'
+   help
+ Provides a driver that uses IPMI Block Transfer messages and
+ semantics on top of plain old I2C.
+
+endif # IPMI_BMC
diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
new file mode 100644
index ..dfe5128f8158
--- /dev/null
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the ipmi bmc drivers.
+#
+
+obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
new file mode 100644
index ..686b83fa42a4
--- /dev/null
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
@@ -0,0 +1,346 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define PFX "IPMI BMC BT-I2C: "
+
+/*
+ * TODO: This is "bt-host" to match the bt-host driver; however, I think this 
is
+ * unclear in the context of a CPU side driver. Should probably name this
+ * and the DEVICE_NAME in bt-host to something like "bt-bmc" or "bt-slave".
+ */
+#define DEVICE_NAME"ipmi-bt-host"
+
+static const unsigned long request_queue_max_len = 256;
+
+struct bt_request_elem {
+   struct list_headlist;
+   struct bt_msg   request;
+};
+
+struct bt_i2c_slave {
+   struct i2c_client   *client;
+   struct miscdevice   miscdev;
+   struct bt_msg   request;
+   struct list_headrequest_queue;
+   atomic_trequest_queue_len;
+   struct bt_msg   response;
+   boolresponse_in_progress;
+   size_t  msg_idx;
+   spinlock_t  lock;
+   wait_queue_head_t   wait_queue;
+   struct mutexfile_mutex;
+};
+
+static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool non_blocking,
+ struct bt_msg *bt_request)
+{
+   int res;
+   unsigned long flags;
+   struct bt_request_elem *queue_elem;
+
+   if (!non_blocking) {
+try_again:
+   res = wait_event_interruptible(
+   bt_slave->wait_queue,

[Openipmi-developer] [PATCH v1 1/3] ipmi: bt-i2c: added documentation for bt-i2c drivers

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
Added device tree binding documentation for ipmi-bt-i2c (host) and
ipmi-bmc-bt-i2c (BMC) and documentation for the Block Transfer over I2C
(bt-i2c) protocol.

Signed-off-by: Brendan Higgins 
---
 Documentation/bt-i2c.txt   | 121 +
 .../devicetree/bindings/ipmi/ipmi-bt-i2c.txt   |  21 
 .../bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt  |  21 
 3 files changed, 163 insertions(+)
 create mode 100644 Documentation/bt-i2c.txt
 create mode 100644 Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
 create mode 100644 
Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt

diff --git a/Documentation/bt-i2c.txt b/Documentation/bt-i2c.txt
new file mode 100644
index ..1b375359c519
--- /dev/null
+++ b/Documentation/bt-i2c.txt
@@ -0,0 +1,121 @@
+Linux Block Transfer over I2C (bt-i2c) interface description
+
+
+by Brendan Higgins  in 2016
+
+Introduction
+
+
+IPMI defines an interface for communication between a CPU, a BMC (Baseboard
+Management Controller), and sensors and various other peripherals. For a more
+complete description of IPMI please see:
+http://www.intel.com/content/www/us/en/servers/ipmi/ipmi-second-gen-interface-spec-v2-rev1-1.html
+
+IPMI defines a *common* message format, as in a set of fields that are common
+across all IPMI messages; they could be viewed as part of the framing
+information for an IPMI message. They include:
+
+   - netfn
+   - lun
+   - cmd
+
+netfn and cmd together define the type of the message; netfn can be viewed as a
+message class and cmd is a subtype of sorts. lun (logical unit number) is used
+for routing between messages between different interfaces. After the last field
+there is usually a variable length payload. Despite these common fields, the
+remainder of the framing varies widely between the IPMI defined hardware
+interfaces; some specify a length as part of the framing which is never more
+than a byte in length; others use a special signal to denote the end of 
message.
+Some IPMI hardware interfaces, the Block Transfer interface in particular,
+support a sequence number that aids in support of multiple in-flight IPMI
+messages.
+
+IPMI defines SSIF (SMBus System Interface) as the IPMI hardware interface for
+SMBus/I2C. It supports a maximum total message length of 255 bytes that is
+broken up across several SMBus block operations. It does not define a sequence
+field in the IPMI framing making it very difficult to support multiple in 
flight
+messages (it is also intentionally left out of the specification). SSIF also
+requires the slave device to NACK until it is ready to accept data (technically
+it only specifies that it may NACK until it is ready, but must NACK on 
attempted
+reads if it does not support SMBus Alert; however, this is an effective
+requirements since a slave device is supposed to start with SMBus Alert
+disabled); this again makes SSIF very difficult to support for some slave
+devices which may not support NACKing arbitrary messages; indeed, at the time 
of
+writing, the Linux I2C slave driver framework did not have support for sending
+NACKs.
+
+Block Transfer over I2C defines a new IPMI compatible interface that uses Block
+Transfer messages and semantics on top of plain old I2C; it does not assume 
that
+the I2C slave is capable of NACKing arbitrary messages; however, it is designed
+such that it could take advantage of SMBus Alert so that the master does not
+have to poll (the Linux I2C core slave mode does not currently support SMBus
+Alert, but a patch adding this support is currently on the way).
+
+Protocol Definition
+---
+
+Block Transfer over I2C uses the IPMI defined Block Transfer message format; it
+supports variable length messages with a maximum length of 255 bytes (limited 
by
+the IPMI Block Transfer length byte).
+
+A Block Transfer over I2C Request is structured as follows:
+
+--
+| I2C start | slave address / RW bit unset | Block Transfer message | ... 
(another message or stop ) |
+--
+
+Multiple requests can be sent before any responses are received. Sequence
+numbers are to be handled by the users of the drivers; thus, no semantics are
+prescribed to their usage; however, the slave driver is required to buffer at
+least 256 requests before dropping requests; this can be used in conjunction
+with sequence numbers to prevent messages from being dropped by the slave.
+
+A Block Transfer over I2C Response is structured as follows:
+
+
+| I2C start | slave address / RW bit set | Block Transfer

[Openipmi-developer] [PATCH v1 3/3] ipmi: bt-i2c: added IPMI Block Transfer over I2C BMC side

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
The IPMI definition of the Block Transfer protocol defines the hardware
registers and behavior in addition to the message format and messaging
semantics. This implements a new protocol that uses IPMI Block Transfer
messages and semantics on top of a standard I2C interface. This protocol
has the same BMC side file system interface as "ipmi-bt-host".

Signed-off-by: Brendan Higgins 
---
 drivers/char/Kconfig|   1 +
 drivers/char/Makefile   |   1 +
 drivers/char/ipmi_bmc/Kconfig   |  22 ++
 drivers/char/ipmi_bmc/Makefile  |   5 +
 drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c | 346 
 include/linux/ipmi_bmc.h|  76 +++
 6 files changed, 451 insertions(+)
 create mode 100644 drivers/char/ipmi_bmc/Kconfig
 create mode 100644 drivers/char/ipmi_bmc/Makefile
 create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
 create mode 100644 include/linux/ipmi_bmc.h

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index ccd239ab879f..2a6ca2325a45 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -195,6 +195,7 @@ config POWERNV_OP_PANEL
  If unsure, say M here to build it as a module called powernv-op-panel.
 
 source "drivers/char/ipmi/Kconfig"
+source "drivers/char/ipmi_bmc/Kconfig"
 
 config DS1620
tristate "NetWinder thermometer support"
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 53e33720818c..9e143186fa30 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -58,4 +58,5 @@ js-rtc-y = rtc.o
 
 obj-$(CONFIG_TILE_SROM)+= tile-srom.o
 obj-$(CONFIG_XILLYBUS) += xillybus/
+obj-$(CONFIG_IPMI_BMC) += ipmi_bmc/
 obj-$(CONFIG_POWERNV_OP_PANEL) += powernv-op-panel.o
diff --git a/drivers/char/ipmi_bmc/Kconfig b/drivers/char/ipmi_bmc/Kconfig
new file mode 100644
index ..26c8e0cb765c
--- /dev/null
+++ b/drivers/char/ipmi_bmc/Kconfig
@@ -0,0 +1,22 @@
+#
+# IPMI BMC configuration
+#
+
+menuconfig IPMI_BMC
+   tristate 'IPMI BMC core'
+   help
+ This enables the BMC-side IPMI drivers.
+
+ If unsure, say N.
+
+if IPMI_BMC
+
+config IPMI_BMC_BT_I2C
+   depends on I2C
+   select I2C_SLAVE
+   tristate 'Generic I2C BT IPMI BMC driver'
+   help
+ Provides a driver that uses IPMI Block Transfer messages and
+ semantics on top of plain old I2C.
+
+endif # IPMI_BMC
diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
new file mode 100644
index ..dfe5128f8158
--- /dev/null
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the ipmi bmc drivers.
+#
+
+obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
new file mode 100644
index ..686b83fa42a4
--- /dev/null
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
@@ -0,0 +1,346 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define PFX "IPMI BMC BT-I2C: "
+
+/*
+ * TODO: This is "bt-host" to match the bt-host driver; however, I think this 
is
+ * unclear in the context of a CPU side driver. Should probably name this
+ * and the DEVICE_NAME in bt-host to something like "bt-bmc" or "bt-slave".
+ */
+#define DEVICE_NAME"ipmi-bt-host"
+
+static const unsigned long request_queue_max_len = 256;
+
+struct bt_request_elem {
+   struct list_headlist;
+   struct bt_msg   request;
+};
+
+struct bt_i2c_slave {
+   struct i2c_client   *client;
+   struct miscdevice   miscdev;
+   struct bt_msg   request;
+   struct list_headrequest_queue;
+   atomic_trequest_queue_len;
+   struct bt_msg   response;
+   boolresponse_in_progress;
+   size_t  msg_idx;
+   spinlock_t  lock;
+   wait_queue_head_t   wait_queue;
+   struct mutexfile_mutex;
+};
+
+static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool non_blocking,
+ struct bt_msg *bt_request)
+{
+   int res;
+   unsigned long flags;
+   struct bt_request_elem *queue_elem;
+
+   if (!non_blocking) {
+try_again:
+   res = wait_event_interruptible(
+   bt_slave->wait_queue,
+

[Openipmi-developer] [PATCH v2 4/4] ipmi: bt-bmc: move Aspeed IPMI BMC driver to ipmi_bmc

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
From: Benjamin Fair 

The ipmi_bmc folder contains drivers for a BMC to communicate using
IPMI. The ipmi folder is only for drivers on the host side using the
OpenIPMI framework.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
Added in v2:
---
 drivers/char/ipmi/Kconfig | 9 -
 drivers/char/ipmi/Makefile| 1 -
 drivers/char/ipmi_bmc/Kconfig | 9 +
 drivers/char/ipmi_bmc/Makefile| 1 +
 drivers/char/{ipmi/bt-bmc.c => ipmi_bmc/ipmi_bmc_bt_aspeed.c} | 0
 5 files changed, 10 insertions(+), 10 deletions(-)
 rename drivers/char/{ipmi/bt-bmc.c => ipmi_bmc/ipmi_bmc_bt_aspeed.c} (100%)

diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
index a8734a369cb0..09ce9f64abf8 100644
--- a/drivers/char/ipmi/Kconfig
+++ b/drivers/char/ipmi/Kconfig
@@ -84,12 +84,3 @@ config IPMI_BT_I2C
tristate 'BT IPMI bmc driver over I2c'
 
 endif # IPMI_HANDLER
-
-config ASPEED_BT_IPMI_BMC
-   depends on ARCH_ASPEED || COMPILE_TEST
-   depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
-   tristate "BT IPMI bmc driver"
-   help
- Provides a driver for the BT (Block Transfer) IPMI interface
- found on Aspeed SOCs (AST2400 and AST2500). The driver
- implements the BMC side of the BT interface.
diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
index 323de0b0b8b5..f0b5672cdca9 100644
--- a/drivers/char/ipmi/Makefile
+++ b/drivers/char/ipmi/Makefile
@@ -13,4 +13,3 @@ obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o
 obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o
 obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o
 obj-$(CONFIG_IPMI_BT_I2C) += ipmi_bt_i2c.o
-obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
diff --git a/drivers/char/ipmi_bmc/Kconfig b/drivers/char/ipmi_bmc/Kconfig
index 26c8e0cb765c..b6af38455702 100644
--- a/drivers/char/ipmi_bmc/Kconfig
+++ b/drivers/char/ipmi_bmc/Kconfig
@@ -19,4 +19,13 @@ config IPMI_BMC_BT_I2C
  Provides a driver that uses IPMI Block Transfer messages and
  semantics on top of plain old I2C.
 
+config ASPEED_BT_IPMI_BMC
+   depends on ARCH_ASPEED || COMPILE_TEST
+   depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
+   tristate "BT IPMI bmc driver"
+   help
+ Provides a driver for the BT (Block Transfer) IPMI interface
+ found on Aspeed SOCs (AST2400 and AST2500). The driver
+ implements the BMC side of the BT interface.
+
 endif # IPMI_BMC
diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
index dfe5128f8158..8bff32b55c24 100644
--- a/drivers/char/ipmi_bmc/Makefile
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
+obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
diff --git a/drivers/char/ipmi/bt-bmc.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
similarity index 100%
rename from drivers/char/ipmi/bt-bmc.c
rename to drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
-- 
2.14.0.rc1.383.gd1ce394fe2-goog


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

[Openipmi-developer] [PATCH v2 1/4] ipmi: bt-i2c: added documentation for bt-i2c drivers

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
Added device tree binding documentation for ipmi-bt-i2c (host) and
ipmi-bmc-bt-i2c (BMC) and documentation for the Block Transfer over I2C
(bt-i2c) protocol.

Signed-off-by: Brendan Higgins 
---
Changes for v2:
  - Fixed a typo
  - Reworded a sentence to make it clear that I was talking about IBM's BT-BMC
  - Deleted an unnecessary discussion of the host side interface (unnecessary
since the OpenIPMI stuff already has its own documentation).
---
 Documentation/bt-i2c.txt   | 109 +
 .../devicetree/bindings/ipmi/ipmi-bt-i2c.txt   |  21 
 .../bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt  |  21 
 3 files changed, 151 insertions(+)
 create mode 100644 Documentation/bt-i2c.txt
 create mode 100644 Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
 create mode 100644 
Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt

diff --git a/Documentation/bt-i2c.txt b/Documentation/bt-i2c.txt
new file mode 100644
index ..499931b02e6c
--- /dev/null
+++ b/Documentation/bt-i2c.txt
@@ -0,0 +1,109 @@
+Linux Block Transfer over I2C (bt-i2c) interface description
+
+
+by Brendan Higgins  in 2016
+
+Introduction
+
+
+IPMI defines an interface for communication between a CPU, a BMC (Baseboard
+Management Controller), and sensors and various other peripherals. For a more
+complete description of IPMI please see:
+http://www.intel.com/content/www/us/en/servers/ipmi/ipmi-second-gen-interface-spec-v2-rev1-1.html
+
+IPMI defines a *common* message format, as in a set of fields that are common
+across all IPMI messages; they could be viewed as part of the framing
+information for an IPMI message. They include:
+
+   - netfn
+   - lun
+   - cmd
+
+netfn and cmd together define the type of the message; netfn can be viewed as a
+message class and cmd is a subtype of sorts. lun (logical unit number) is used
+for routing between messages between different interfaces. After the last field
+there is usually a variable length payload. Despite these common fields, the
+remainder of the framing varies widely between the IPMI defined hardware
+interfaces; some specify a length as part of the framing which is never more
+than a byte in length; others use a special signal to denote the end of 
message.
+Some IPMI hardware interfaces, the Block Transfer interface in particular,
+support a sequence number that aids in support of multiple in-flight IPMI
+messages.
+
+IPMI defines SSIF (SMBus System Interface) as the IPMI hardware interface for
+SMBus/I2C. It supports a maximum total message length of 255 bytes that is
+broken up across several SMBus block operations. It does not define a sequence
+field in the IPMI framing making it very difficult to support multiple in 
flight
+messages (it is also intentionally left out of the specification). SSIF also
+requires the slave device to NACK until it is ready to accept data (technically
+it only specifies that it may NACK until it is ready, but must NACK on 
attempted
+reads if it does not support SMBus Alert; however, this is an effective
+requirements since a slave device is supposed to start with SMBus Alert
+disabled); this again makes SSIF very difficult to support for some slave
+devices which may not support NACKing arbitrary messages; indeed, at the time 
of
+writing, the Linux I2C slave driver framework did not have support for sending
+NACKs.
+
+Block Transfer over I2C defines a new IPMI compatible interface that uses Block
+Transfer messages and semantics on top of plain old I2C; it does not assume 
that
+the I2C slave is capable of NACKing arbitrary messages; however, it is designed
+such that it could take advantage of SMBus Alert so that the master does not
+have to poll (the Linux I2C core slave mode does not currently support SMBus
+Alert, but a patch adding this support is currently on the way).
+
+Protocol Definition
+---
+
+Block Transfer over I2C uses the IPMI defined Block Transfer message format; it
+supports variable length messages with a maximum length of 255 bytes (limited 
by
+the IPMI Block Transfer length byte).
+
+A Block Transfer over I2C Request is structured as follows:
+
+--
+| I2C start | slave address / RW bit unset | Block Transfer message | ... 
(another message or stop ) |
+--
+
+Multiple requests can be sent before any responses are received. Sequence
+numbers are to be handled by the users of the drivers; thus, no semantics are
+prescribed to their usage; however, the slave driver is required to buffer at
+least 256 requests before dropping requests; this can be used in conjunction
+with sequence numbers to prevent messages from

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

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
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.

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.

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [Openipmi-developer] [PATCH v1 0/3] ipmi: bt-i2c: added IPMI Block Transfer over I2C

2017-08-04 Thread Brendan Higgins via Openipmi-developer 
On Fri, Aug 4, 2017 at 4: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.
>
> 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.
>
> 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.

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

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

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

2017-08-07 Thread Brendan Higgins via Openipmi-developer 
On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard  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

[Openipmi-developer] [RFC v1 1/4] ipmi_bmc: framework for BT IPMI on BMCs

2017-08-07 Thread Brendan Higgins via Openipmi-developer 
From: Benjamin Fair 

This patch introduces a framework for writing IPMI drivers which run on
a Board Management Controller. It is similar in function to OpenIPMI.
The framework handles registering devices and routing messages.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
 drivers/char/ipmi_bmc/Makefile   |   1 +
 drivers/char/ipmi_bmc/ipmi_bmc.c | 294 +++
 include/linux/ipmi_bmc.h | 184 
 3 files changed, 479 insertions(+)
 create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc.c

diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
index 8bff32b55c24..9c7cd48d899f 100644
--- a/drivers/char/ipmi_bmc/Makefile
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -2,5 +2,6 @@
 # Makefile for the ipmi bmc drivers.
 #
 
+obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
 obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
diff --git a/drivers/char/ipmi_bmc/ipmi_bmc.c b/drivers/char/ipmi_bmc/ipmi_bmc.c
new file mode 100644
index ..c1324ac9a83c
--- /dev/null
+++ b/drivers/char/ipmi_bmc/ipmi_bmc.c
@@ -0,0 +1,294 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define PFX "IPMI BMC core: "
+
+struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx()
+{
+   static struct ipmi_bmc_ctx global_ctx;
+
+   return _ctx;
+}
+
+int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
+  struct bt_msg *bt_response)
+{
+   struct ipmi_bmc_bus *bus;
+   int ret = -ENODEV;
+
+   rcu_read_lock();
+   bus = rcu_dereference(ctx->bus);
+
+   if (bus)
+   ret = bus->send_response(bus, bt_response);
+
+   rcu_read_unlock();
+   return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_send_response);
+
+bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx)
+{
+   struct ipmi_bmc_bus *bus;
+   bool ret = false;
+
+   rcu_read_lock();
+   bus = rcu_dereference(ctx->bus);
+
+   if (bus)
+   ret = bus->is_response_open(bus);
+
+   rcu_read_unlock();
+   return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_is_response_open);
+
+int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
+struct ipmi_bmc_device *device_in)
+{
+   struct ipmi_bmc_device *device;
+
+   mutex_lock(>drivers_mutex);
+   /* Make sure it hasn't already been registered. */
+   list_for_each_entry(device, >devices, link) {
+   if (device == device_in) {
+   mutex_unlock(>drivers_mutex);
+   return -EINVAL;
+   }
+   }
+
+   list_add_rcu(_in->link, >devices);
+   mutex_unlock(>drivers_mutex);
+
+   return 0;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_device);
+
+int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
+  struct ipmi_bmc_device *device_in)
+{
+   struct ipmi_bmc_device *device;
+   bool found = false;
+
+   mutex_lock(>drivers_mutex);
+   /* Make sure it is currently registered. */
+   list_for_each_entry(device, >devices, link) {
+   if (device == device_in) {
+   found = true;
+   break;
+   }
+   }
+   if (!found) {
+   mutex_unlock(>drivers_mutex);
+   return -ENXIO;
+   }
+
+   list_del_rcu(_in->link);
+   mutex_unlock(>drivers_mutex);
+   synchronize_rcu();
+
+   return 0;
+}
+EXPORT_SYMBOL(ipmi_bmc_unregister_device);
+
+int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
+struct ipmi_bmc_device *device)
+{
+   int ret;
+
+   mutex_lock(>drivers_mutex);
+   if (!ctx->default_device) {
+   ctx->default_device = device;
+   ret = 0;
+   } else {
+   ret = -EBUSY;
+   }
+   mutex_unlock(>drivers_mutex);
+
+   return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_default_device);
+
+int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
+  struct ipmi_bmc_device *device)
+{
+   int ret;
+
+   mutex_lock(>drivers_mutex);
+   if (ctx->default_device == device) {
+   ctx->default_device = NULL;
+   ret = 0;
+   } else {
+   ret = -ENXIO;
+   }
+

[Openipmi-developer] [RFC v1 3/4] ipmi_bmc: bt-i2c: port driver to IPMI BMC framework

2017-08-07 Thread Brendan Higgins via Openipmi-developer 
From: Benjamin Fair 

Instead of handling interaction with userspace and providing a file
interface, rely on the IPMI BMC framework to do this. This simplifies
the logic and eliminates duplicate code.

Signed-off-by: Benjamin Fair 
Signed-off-by: Brendan Higgins 
---
 drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c | 202 +---
 1 file changed, 28 insertions(+), 174 deletions(-)

diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c 
b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
index 686b83fa42a4..6665aa9d4300 100644
--- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
@@ -14,102 +14,51 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
-#include 
 #include 
 #include 
-#include 
 
 #define PFX "IPMI BMC BT-I2C: "
 
-/*
- * TODO: This is "bt-host" to match the bt-host driver; however, I think this 
is
- * unclear in the context of a CPU side driver. Should probably name this
- * and the DEVICE_NAME in bt-host to something like "bt-bmc" or "bt-slave".
- */
-#define DEVICE_NAME"ipmi-bt-host"
-
-static const unsigned long request_queue_max_len = 256;
-
-struct bt_request_elem {
-   struct list_headlist;
-   struct bt_msg   request;
-};
-
 struct bt_i2c_slave {
+   struct ipmi_bmc_bus bus;
struct i2c_client   *client;
-   struct miscdevice   miscdev;
+   struct ipmi_bmc_ctx *bmc_ctx;
struct bt_msg   request;
-   struct list_headrequest_queue;
-   atomic_trequest_queue_len;
struct bt_msg   response;
boolresponse_in_progress;
size_t  msg_idx;
spinlock_t  lock;
-   wait_queue_head_t   wait_queue;
-   struct mutexfile_mutex;
 };
 
-static int receive_bt_request(struct bt_i2c_slave *bt_slave, bool non_blocking,
- struct bt_msg *bt_request)
+static bool bt_i2c_is_response_open(struct ipmi_bmc_bus *bus)
 {
-   int res;
+   struct bt_i2c_slave *bt_slave;
+   bool response_in_progress;
unsigned long flags;
-   struct bt_request_elem *queue_elem;
-
-   if (!non_blocking) {
-try_again:
-   res = wait_event_interruptible(
-   bt_slave->wait_queue,
-   atomic_read(_slave->request_queue_len));
-   if (res)
-   return res;
-   }
 
-   spin_lock_irqsave(_slave->lock, flags);
-   if (!atomic_read(_slave->request_queue_len)) {
-   spin_unlock_irqrestore(_slave->lock, flags);
-   if (non_blocking)
-   return -EAGAIN;
-   goto try_again;
-   }
+   bt_slave = container_of(bus, struct bt_i2c_slave, bus);
 
-   if (list_empty(_slave->request_queue)) {
-   pr_err(PFX "request_queue was empty despite nonzero 
request_queue_len\n");
-   return -EIO;
-   }
-   queue_elem = list_first_entry(_slave->request_queue,
- struct bt_request_elem, list);
-   memcpy(bt_request, _elem->request, sizeof(*bt_request));
-   list_del(_elem->list);
-   kfree(queue_elem);
-   atomic_dec(_slave->request_queue_len);
+   spin_lock_irqsave(_slave->lock, flags);
+   response_in_progress = bt_slave->response_in_progress;
spin_unlock_irqrestore(_slave->lock, flags);
-   return 0;
+
+   return !response_in_progress;
 }
 
-static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
-   struct bt_msg *bt_response)
+static int bt_i2c_send_response(struct ipmi_bmc_bus *bus,
+   struct bt_msg *bt_response)
 {
-   int res;
+   struct bt_i2c_slave *bt_slave;
unsigned long flags;
 
-   if (!non_blocking) {
-try_again:
-   res = wait_event_interruptible(bt_slave->wait_queue,
-  !bt_slave->response_in_progress);
-   if (res)
-   return res;
-   }
+   bt_slave = container_of(bus, struct bt_i2c_slave, bus);
 
spin_lock_irqsave(_slave->lock, flags);
if (bt_slave->response_in_progress) {
spin_unlock_irqrestore(_slave->lock, flags);
-   if (non_blocking)
-   return -EAGAIN;
-   goto try_again;
+   return -EAGAIN;
}
 
memcpy(_slave->response, bt_response, sizeof(*bt_response));
@@ -118,106 +67,13 @@ static int send_bt_response(struct bt_i2c_slave 
*bt_slave, bool non_blocking,
return 0;
 }
 
-static inline struct bt_i2c_slave *to_bt_i2c_slave(struct file *file)
-{
-   return container_of(file->private_data, struct bt_i2c_slave, miscdev);
-}
-
-static ssize_t bt_read(struct file

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

2017-08-09 Thread Brendan Higgins via Openipmi-developer 
> 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.

>
> 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/.

>
> -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.

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

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [Openipmi-developer] [PATCH v1 0/3] ipmi: bt-i2c: added IPMI Block Transfer over I2C

2017-08-09 Thread Brendan Higgins via Openipmi-developer 
On Wed, Aug 9, 2017 at 3:56 AM, Anton D. Kachalov  wrote:
> Hello,
>
> I would like to mention one of the our related work for IPMI and I2C.
>
> We use OpenIPMI stack to connect to the computing nodes through the I2C
> using IPMB (BT is not supported by nodes):
>
> https://github.com/ya-mouse/meta-openbmc-yandex/blob/master/meta-yandex/meta-openrack/meta-shaosi/recipes-kernel/linux/linux-obmc/ipmi_i2c.c
>
> It lacks complete slave support (slave part is only for receiving known
> packets with query results due to OpenIPMI implementation in kernel) and use
> one local slave to communicate with a number of target systems on the same
> bus (currently supported only 1-to-1 schema).
>
> With this stuff we able to use ipmitool across different /dev/ipmiX devices
> to communicate with nodes.

Cool, I met someone else who had a similar use case which is part of why
I decided to share this (not sure if should say who).

So it sounds like we are probably not going to go with the approach I proposed;
if you indeed find this useful, I would suggest that we put this in our OpenBMC
repository and switch it out with the suggested method at some point.

Let me know what you think

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [Openipmi-developer] [RFC v1 1/4] ipmi_bmc: framework for BT IPMI on BMCs

2017-08-23 Thread Brendan Higgins via Openipmi-developer 
Sorry for the delayed response.

>>> This piece of code takes a communication interface, called a bus, and
>>> muxes/demuxes
>>> messages on that bus to various users, called devices.  The name
>>> "devices"
>>> confused
>>> me for a bit, because I was thinking they were physical devices, what
>>> Linux
>>> would
>>> call a device.  I don't have a good suggestion for another name, though.
>>
>> We could maybe do "*_interface" instead of "*_bus" and "*_handler" instead
>> of "*_device"; admittedly, it is not the best name ever: handler has some
>> connotations.
>>
>
> I think the "_bus" name is ok, that's what I2C uses, and "communication bus"
> makes
> sense, at least to me.  _handler is probably better than _device, but not
> that much.
> The IPMI host driver uses _user, but that's not great, either. Maybe
> _service?  Naming
> is such a pain.

Actually, I like _service. The way I am doing it here it is not
necessarily broken
up by command so service makes sense.

>
>
...
>>
>> As far as this being a complete design; I do not consider what I have
>> presented as being complete. I mentioned some things above that I would
>> like
>> to add and some people have already chimed in asking for some changes.
>> I just wanted to get some feedback before I went *too* far.
>
>
> I'm not sure this is well know, but the OpenIPMI library has a fully
> functional BMC
> that I wrote, originally for testing the library, but it has been deployed
> in a system
> and people use it with QEMU.  So I have some experience here.

Is this the openipmi/lanserv?

>
> The biggest frustration* writing that BMC was that IPMI does not lend itself
> to a
> nice modular design.  Some parts are fairly modular (sensors, SDR, FRU data)
> but some are not so clean (channel management, firmware firewall, user
> management).  I would try to design things in nice independent pieces, and
> end up having to put ugly hooks in places.

Yeah, I had started to notice this when I started working on our userland IPMI
stack.

>
> Firmware firewall, for instance, makes sense to implement in a single place
> that
> handles all incoming messages.  However, it also deals with subcommands
> (like making firewalls for each individual LAN parameter), so you either
> have
> to put knowledge of the individual command structure in the main firewall,
> or you have to embed pieces of the firewall in each command that has
> subcommands.  But if you put it in the commands, then the commands
> have to have knowledge of the communication channels.
>
> I ended up running into this all over the place.  In the end I just hacked
> in
> what I needed because what I designed was monolithic.  It seemed that
> designing it in a way that was modular was so complex that it wasn't worth
> the effort.  I've seen a few BMC designs, none were modular.
>
> In the design you are working on here, firmware firewall will be a bit of a
> challenge.
>
> Also, if you implement a LAN interface, you have to deal with a shared
> channel and privilege levels.  You will either have to have a context per
> LAN connection, with user and privilege attached to the context, or you
> will need a way to have user and privilege information in each message
> so that the message router can handle rejecting messages it doesn't
> have privilege to do, and the responses coming back will go to the
> right connection.
>
> There is also a strange situation where commands can come from a LAN
> (or other) interface, be routed to a system interface where the host
> picks up the command, handles it, send the response back to the
> BMC which routes it back to the LAN interface.  People actually use this.

That sounds like fun :-P

>
> Another problem I see with this design is that most services don't care
> about the interface the message comes from.  They won't want to have
> to discover and make individual connections to each connection, they
> will just want to say "Hook me up to everything, I don't care."
>
> Some services will care, though.  Event queues and interface flag handling
> will only want that on individual interfaces.  For these types of services,
> it would be easier if they could discover and identify the interfaces.  If
> interfaces are added dynamically (or each LAN connection appears as
> a context) it needs a way to know when interfaces come and go.
>
> If you end up implementing all this, you will have a fairly complex
> piece of software in your message routing.  If you look at the message
> handler in the host driver, it's fairly complex, but it makes the user's
> job simple, and it makes the interfaces job simple(r).  IMHO that's a
> fair trade-off.  If you have to have complexity, keep it in one place.

I think that is a reasonable point. My initial goal was not to move the
routing that we do in user land to kernel space and only provide basic
facilities that are enough for my use case, but it sounds like there might
be some wisdom in handling message routing and message filtering to

Re: [Openipmi-developer] [RFC v1 0/4] ipmi_bmc: framework for IPMI on BMCs

2017-08-23 Thread Brendan Higgins via Openipmi-developer 
On Mon, Aug 14, 2017 at 3:28 PM, Brendan Higgins
 wrote:
> On Mon, Aug 14, 2017 at 7:03 PM, Patrick Williams  wrote:
>> On Mon, Aug 07, 2017 at 08:52:57PM -0700, Brendan Higgins wrote:
>>> Currently, OpenBMC handles all IPMI message routing and handling in 
>>> userland;
>>> the existing drivers simply provide a file interface for the hardware on the
>>> device. In this patchset, we propose a common file interface to be shared 
>>> by all
>>> IPMI hardware interfaces, but also a framework for implementing handlers at 
>>> the
>>> kernel level, similar to how the existing OpenIPMI framework supports both
>>> kernel users, as well as misc device file interface.
>>
>> Brendan,
>>
>> Can you expand on why this is a good thing from an OpenBMC perspective?
>
> Sure, so in addition to the individual handlers; this does introduce a
> common file
> system interface for BMC side IPMI hardware interfaces. I think that is pretty
> straightforward.
>
> Corey and I are still exploring the handlers. My original intention
> was not to replace
> any of the handlers implemented in userspace. My motivating use case is for 
> some
> OEM commands that would be easier to implement inside of the kernel.
>
> I was hoping to send out an overview of that, but the internet in my
> hotel sucks,
> so I will do it the next time I get decent internet access. :-P

I was able to get this out on Monday on the OpenBMC mailing lists:
https://lists.ozlabs.org/pipermail/openbmc/2017-August/008861.html

>
> In any case, Corey raised some interesting points on the subject; the
> most recent
> round I have not responded to yet.
>
>> We have a pretty significant set of IPMI providers that run in the
>> userspace daemon(s) and I can't picture more than a very small subset
>> even being possible to run in kernel space without userspace assistance.
>
> Like I said, I have an example of some OEM commands. Also, as I have said,
> my intention is not to replace any of the userland stuff. That being said, I 
> am
> not sure the approach we have taken so far is the best when it comes to some
> of the new protocols we are looking at like IPMB and MCTP. Having some
> consistency of where we draw these interface boundaries would be nice; so
> maybe that means rethinking some of that. I don't know, but it sounds like
> Corey has already tried some of this stuff out on his own BMC side
> implementation.
>
> Regardless, I think there is a lot of interesting conversation to be had.
>
>> We also already have an implementation of a RMCP+ daemon that can, and
>> does, share most of its providers with the host-side daemon.
>
> That's great. Like I said, my original intention was not to rewrite any of 
> that.

Corey had a good point about this in my thread with him. I made a proposal
of what to do there.

>
>>
>> --
>> Patrick Williams
>
> By the way, Corey suggested that we have a BoF session at the Linux Plumbers
> Conference, so I set one up:
> https://linuxplumbersconf.org/2017/ocw/proposals/4723
> I highly encourage anyone who is interested in this discussion to attend.
>
> Thanks!

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Re: [Openipmi-developer] [RFC v1 1/4] ipmi_bmc: framework for BT IPMI on BMCs

2017-09-06 Thread Brendan Higgins via Openipmi-developer 
On Thu, Aug 24, 2017 at 6:01 AM, Corey Minyard  wrote:
> On 08/23/2017 01:03 AM, Brendan Higgins wrote:
>
> 
>>
>> ...

 As far as this being a complete design; I do not consider what I have
 presented as being complete. I mentioned some things above that I would
 like
 to add and some people have already chimed in asking for some changes.
 I just wanted to get some feedback before I went *too* far.
>>>
>>>
>>> I'm not sure this is well know, but the OpenIPMI library has a fully
>>> functional BMC
>>> that I wrote, originally for testing the library, but it has been
>>> deployed
>>> in a system
>>> and people use it with QEMU.  So I have some experience here.
>>
>> Is this the openipmi/lanserv?
>
>
> Yes.  That name is terrible, but it started out as a server that provided a
> LAN
> interface to an existing BMC over the local interface, primarily for my
> testing.
>
>
>>
>>> The biggest frustration* writing that BMC was that IPMI does not lend
>>> itself
>>> to a
>>> nice modular design.  Some parts are fairly modular (sensors, SDR, FRU
>>> data)
>>> but some are not so clean (channel management, firmware firewall, user
>>> management).  I would try to design things in nice independent pieces,
>>> and
>>> end up having to put ugly hooks in places.
>>
>> Yeah, I had started to notice this when I started working on our userland
>> IPMI
>> stack.
>>
>>> Firmware firewall, for instance, makes sense to implement in a single
>>> place
>>> that
>>> handles all incoming messages.  However, it also deals with subcommands
>>> (like making firewalls for each individual LAN parameter), so you either
>>> have
>>> to put knowledge of the individual command structure in the main
>>> firewall,
>>> or you have to embed pieces of the firewall in each command that has
>>> subcommands.  But if you put it in the commands, then the commands
>>> have to have knowledge of the communication channels.
>>>
>>> I ended up running into this all over the place.  In the end I just
>>> hacked
>>> in
>>> what I needed because what I designed was monolithic.  It seemed that
>>> designing it in a way that was modular was so complex that it wasn't
>>> worth
>>> the effort.  I've seen a few BMC designs, none were modular.
>>>
>>> In the design you are working on here, firmware firewall will be a bit of
>>> a
>>> challenge.
>>>
>>> Also, if you implement a LAN interface, you have to deal with a shared
>>> channel and privilege levels.  You will either have to have a context per
>>> LAN connection, with user and privilege attached to the context, or you
>>> will need a way to have user and privilege information in each message
>>> so that the message router can handle rejecting messages it doesn't
>>> have privilege to do, and the responses coming back will go to the
>>> right connection.
>>>
>>> There is also a strange situation where commands can come from a LAN
>>> (or other) interface, be routed to a system interface where the host
>>> picks up the command, handles it, send the response back to the
>>> BMC which routes it back to the LAN interface.  People actually use this.
>>
>> That sounds like fun :-P
>>
>>> Another problem I see with this design is that most services don't care
>>> about the interface the message comes from.  They won't want to have
>>> to discover and make individual connections to each connection, they
>>> will just want to say "Hook me up to everything, I don't care."
>>>
>>> Some services will care, though.  Event queues and interface flag
>>> handling
>>> will only want that on individual interfaces.  For these types of
>>> services,
>>> it would be easier if they could discover and identify the interfaces.
>>> If
>>> interfaces are added dynamically (or each LAN connection appears as
>>> a context) it needs a way to know when interfaces come and go.
>>>
>>> If you end up implementing all this, you will have a fairly complex
>>> piece of software in your message routing.  If you look at the message
>>> handler in the host driver, it's fairly complex, but it makes the user's
>>> job simple, and it makes the interfaces job simple(r).  IMHO that's a
>>> fair trade-off.  If you have to have complexity, keep it in one place.
>>
>> I think that is a reasonable point. My initial goal was not to move the
>> routing that we do in user land to kernel space and only provide basic
>> facilities that are enough for my use case, but it sounds like there might
>> be some wisdom in handling message routing and message filtering to
>> kernel space. This might also make the framework more platform agnostic,
>> and less tightly coupled to OpenBMC.
>>
>> Nevertheless, that substantially broadens the scope of what I am trying
>> to do.
>>
>> I think a good place to start is still to create a common interface for
>> hardware interfaces (BT, KCS, SSIF, and their varying implementations)
>> to implement, as I have done, and while we are working on the rest of the
>> stack on top of it, we have