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

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

[Brendan Higgins]

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.

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

[Brendan Higgins]

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 <brendanhiggins@google.com>
---
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 000000000000..499931b02e6c
--- /dev/null
+++ b/Documentation/bt-i2c.txt
@@ -0,0 +1,109 @@
+Linux Block Transfer over I2C (bt-i2c) interface description
+============================================================
+
+by Brendan Higgins <brendanhiggins@google.com> 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 message | ... (another message or stop ) |
+----------------------------------------------------------------------------------------------------
+
+Until a response is ready to send, the slave will respond only with zero bytes.
+If the slave receives a start or a stop before it was done sending a response,
+it will resend the entire response the next time a read is requested; in this
+way, an I2C master may poll for responses by reading a single byte until it is
+non-zero and then perform the read transaction shown above.
+
+In the future, it is planned that the slave will support using SMBus Alert to
+notify the master of an available response, but will never be required.
+
+Driver Interface
+----------------
+
+The device file interface for the slave side is modeled after bt-bmc, a Block
+Transfer over LPC driver for the Aspeed 24xx/255xx.
+
+A read (request) should be large enough to fit a Block Transfer message
+(including the length byte) of 256 bytes; if the provided buffer is smaller, the
+message will be truncated.
+
+A write (response) must be no greater than the maximum valid length of a Block
+Transfer message (including the length byte), 256 bytes; if the provided buffer
+is larger, the write will fail with EINVAL. The driver also enforces a valid
+length byte which must contain the total length of the message not including the
+length byte; thus, the write will fail with EINVAL if the buffer length is less
+than the length field plus one. The driver will also only send length field plus
+one bytes.
+
+The slave device file interface also supports the poll system call; it will
+report when a request is available to read or it is ready to accept a response.
+
+The master side conforms to the OpenIPMI kernel framework.
diff --git a/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt b/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
new file mode 100644
index 000000000000..bd956f2805e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
@@ -0,0 +1,21 @@
+Device tree configuration for the ipmi-bt-i2c driver.
+
+Required Properties:
+- compatible 	: should be "ipmi-bt-i2c"
+- reg		: the I2C slave address of the ipmi-bmc-bt-i2c
+
+i2c0: i2c-bus@40 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x40 0x40>;
+	compatible = "aspeed,ast2400-i2c-bus";
+	clock-frequency = <100000>;
+	status = "disabled";
+	interrupts = <0>;
+	interrupt-parent = <&i2c>;
+
+	bt-i2c-master@41 {
+		compatible = "ipmi-bt-i2c";
+		reg = <0x41>;
+	};
+};
diff --git a/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt b/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt
new file mode 100644
index 000000000000..6e82693ee50e
--- /dev/null
+++ b/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt
@@ -0,0 +1,21 @@
+Device tree configuration for the ipmi-bmc-bt-i2c driver.
+
+Required Properties:
+- compatible 	: should be "ipmi-bmc-bt-i2c"
+- reg		: the I2C slave address of this driver.
+
+i2c0: i2c-bus@40 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x40 0x40>;
+	compatible = "aspeed,ast2400-i2c-bus";
+	clock-frequency = <100000>;
+	status = "disabled";
+	interrupts = <0>;
+	interrupt-parent = <&i2c>;
+
+	bt-i2c-slave@41 {
+		compatible = "ipmi-bmc-bt-i2c";
+		reg = <0x41>;
+	};
+};
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

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

[Rob Herring]

On Fri, Aug 04, 2017 at 06:18:52PM -0700, Brendan Higgins wrote:
> 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.

Please split the bindings to a separate patch.

> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> 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/devicetree/bindings/ipmi/ipmi-bt-i2c.txt b/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
> new file mode 100644
> index 000000000000..bd956f2805e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
> @@ -0,0 +1,21 @@
> +Device tree configuration for the ipmi-bt-i2c driver.
> +
> +Required Properties:
> +- compatible 	: should be "ipmi-bt-i2c"
> +- reg		: the I2C slave address of the ipmi-bmc-bt-i2c
> +
> +i2c0: i2c-bus@40 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x40 0x40>;
> +	compatible = "aspeed,ast2400-i2c-bus";

This is the host side, right? So this should be something besides an 
ASpeed I2C controller.

> +	clock-frequency = <100000>;
> +	status = "disabled";

Don't should status in examples.

> +	interrupts = <0>;
> +	interrupt-parent = <&i2c>;
> +
> +	bt-i2c-master@41 {

But it's the slave device you are describing here, right? The host is 
the master. Just "ipmi-bt" is probably enough.

> +		compatible = "ipmi-bt-i2c";
> +		reg = <0x41>;
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt b/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt
> new file mode 100644
> index 000000000000..6e82693ee50e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt
> @@ -0,0 +1,21 @@
> +Device tree configuration for the ipmi-bmc-bt-i2c driver.
> +
> +Required Properties:
> +- compatible 	: should be "ipmi-bmc-bt-i2c"
> +- reg		: the I2C slave address of this driver.
> +
> +i2c0: i2c-bus@40 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x40 0x40>;
> +	compatible = "aspeed,ast2400-i2c-bus";
> +	clock-frequency = <100000>;
> +	status = "disabled";
> +	interrupts = <0>;
> +	interrupt-parent = <&i2c>;
> +
> +	bt-i2c-slave@41 {
> +		compatible = "ipmi-bmc-bt-i2c";
> +		reg = <0x41>;

This doesn't follow the slave binding which sets bit 30 for slave 
devices.

> +	};
> +};
> -- 
> 2.14.0.rc1.383.gd1ce394fe2-goog
> 

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

[Rob Herring]

On Fri, Aug 04, 2017 at 06:18:52PM -0700, Brendan Higgins wrote:
> 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.

Please split the bindings to a separate patch.

> 
> Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> 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/devicetree/bindings/ipmi/ipmi-bt-i2c.txt b/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
> new file mode 100644
> index 000000000000..bd956f2805e4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ipmi/ipmi-bt-i2c.txt
> @@ -0,0 +1,21 @@
> +Device tree configuration for the ipmi-bt-i2c driver.
> +
> +Required Properties:
> +- compatible 	: should be "ipmi-bt-i2c"
> +- reg		: the I2C slave address of the ipmi-bmc-bt-i2c
> +
> +i2c0: i2c-bus@40 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x40 0x40>;
> +	compatible = "aspeed,ast2400-i2c-bus";

This is the host side, right? So this should be something besides an 
ASpeed I2C controller.

> +	clock-frequency = <100000>;
> +	status = "disabled";

Don't should status in examples.

> +	interrupts = <0>;
> +	interrupt-parent = <&i2c>;
> +
> +	bt-i2c-master@41 {

But it's the slave device you are describing here, right? The host is 
the master. Just "ipmi-bt" is probably enough.

> +		compatible = "ipmi-bt-i2c";
> +		reg = <0x41>;
> +	};
> +};
> diff --git a/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt b/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt
> new file mode 100644
> index 000000000000..6e82693ee50e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ipmi_bmc/ipmi-bmc-bt-i2c.txt
> @@ -0,0 +1,21 @@
> +Device tree configuration for the ipmi-bmc-bt-i2c driver.
> +
> +Required Properties:
> +- compatible 	: should be "ipmi-bmc-bt-i2c"
> +- reg		: the I2C slave address of this driver.
> +
> +i2c0: i2c-bus@40 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x40 0x40>;
> +	compatible = "aspeed,ast2400-i2c-bus";
> +	clock-frequency = <100000>;
> +	status = "disabled";
> +	interrupts = <0>;
> +	interrupt-parent = <&i2c>;
> +
> +	bt-i2c-slave@41 {
> +		compatible = "ipmi-bmc-bt-i2c";
> +		reg = <0x41>;

This doesn't follow the slave binding which sets bit 30 for slave 
devices.

> +	};
> +};
> -- 
> 2.14.0.rc1.383.gd1ce394fe2-goog
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Brendan Higgins]

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 <brendanhiggins@google.com>
---
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 000000000000..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 <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/ipmi_smi.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/types.h>
+
+#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_entry	seq_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 not ready, so we peek at the first byte to
+	 * see if it is a valid length.
+	 */
+	ret = i2c_master_recv(client, &len, 1);
+	while (ret != 1 || len == 0) {
+		if (ret < 0)
+			return ret;
+
+		usleep_range(1000, 1500);
+
+		/* Signal received: quit syscall. */
+		if (signal_pending(current))
+			return -ERESTARTSYS;
+
+		ret = i2c_master_recv(client, &len, 1);
+	}
+
+	timeout = jiffies + msecs_to_jiffies(write_timeout);
+	do {
+		read_time = jiffies;
+		ret = i2c_master_recv(client, buf, len + 1);
+		if (ret >= 0)
+			return 0;
+		usleep_range(1000, 1500);
+	} while (time_before(read_time, timeout));
+	return ret;
+}
+
+static int ipmi_bt_i2c_start_processing(void *data, ipmi_smi_t intf)
+{
+	struct ipmi_bt_i2c_master *master = data;
+
+	master->intf = intf;
+
+	return 0;
+}
+
+static void __ipmi_bt_i2c_error_reply(struct ipmi_bt_i2c_master *master,
+				      struct ipmi_smi_msg *msg,
+				      u8 completion_code)
+{
+	struct ipmi_bt_i2c_smi_msg *response;
+	struct ipmi_bt_i2c_smi_msg *request;
+
+	response = (struct ipmi_bt_i2c_smi_msg *) msg->rsp;
+	request = (struct ipmi_bt_i2c_smi_msg *) msg->data;
+
+	response->netfn_lun = request->netfn_lun | 0x4;
+	response->cmd = request->cmd;
+	response->payload[0] = completion_code;
+	msg->rsp_size = 3;
+	ipmi_smi_msg_received(master->intf, msg);
+}
+
+static void ipmi_bt_i2c_error_reply(struct ipmi_bt_i2c_master *master,
+				    struct ipmi_smi_msg *msg,
+				    u8 completion_code)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->lock, flags);
+	__ipmi_bt_i2c_error_reply(master, msg, completion_code);
+	spin_unlock_irqrestore(&master->lock, flags);
+}
+
+/*
+ * ipmi_bt_i2c_smi_msg contains a payload and 2 header fields, each 1 byte:
+ * netfn_lun and cmd. They're passed to OpenIPMI within an ipmi_smi_msg struct
+ * along with their length.
+ *
+ * ipmi_bt_i2c_msg contains a payload and 4 header fields: the two above in
+ * addition to seq and len. However, len is not included in the length count so
+ * this message encapsulation is considered 1 byte longer than the other.
+ */
+static u8 ipmi_bt_i2c_smi_to_bt_len(u8 smi_msg_len)
+{
+	/* Only field that BT adds to the header is seq. */
+	return smi_msg_len + 1;
+}
+
+static u8 ipmi_bt_i2c_bt_to_smi_len(struct ipmi_bt_i2c_msg *bt_msg)
+{
+	/* Subtract one byte for seq (opposite of above) */
+	return bt_msg->len - 1;
+}
+
+static size_t ipmi_bt_i2c_payload_len(struct ipmi_bt_i2c_msg *bt_msg)
+{
+	/* Subtract one byte for each: netfn_lun, seq, cmd. */
+	return bt_msg->len - 3;
+}
+
+static bool ipmi_bt_i2c_assign_seq(struct ipmi_bt_i2c_master *master,
+				   struct ipmi_smi_msg *msg, u8 *ret_seq)
+{
+	struct ipmi_bt_i2c_seq_entry *entry;
+	bool did_cleanup = false;
+	unsigned long flags;
+	u8 seq;
+
+	spin_lock_irqsave(&master->lock, flags);
+retry:
+	for (seq = 0; seq < IPMI_BT_I2C_SEQ_MAX; seq++) {
+		if (!master->seq_msg_map[seq].msg) {
+			master->seq_msg_map[seq].msg = msg;
+			master->seq_msg_map[seq].send_time = jiffies;
+			spin_unlock_irqrestore(&master->lock, flags);
+			*ret_seq = seq;
+			return true;
+		}
+	}
+
+	if (did_cleanup) {
+		spin_unlock_irqrestore(&master->lock, flags);
+		return false;
+	}
+
+	/*
+	 * TODO: we should do cleanup at times other than only when we run out
+	 * of sequence numbers.
+	 */
+	for (seq = 0; seq < IPMI_BT_I2C_SEQ_MAX; seq++) {
+		entry = &master->seq_msg_map[seq];
+		if (entry->msg &&
+		    time_after(entry->send_time + IPMI_BT_I2C_TIMEOUT,
+			       jiffies)) {
+			__ipmi_bt_i2c_error_reply(master, entry->msg,
+						  IPMI_TIMEOUT_ERR);
+			entry->msg = NULL;
+		}
+	}
+	did_cleanup = true;
+	goto retry;
+}
+
+static struct ipmi_smi_msg *ipmi_bt_i2c_find_msg(
+		struct ipmi_bt_i2c_master *master, u8 seq)
+{
+	struct ipmi_smi_msg *msg;
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->lock, flags);
+	msg = master->seq_msg_map[seq].msg;
+	spin_unlock_irqrestore(&master->lock, flags);
+	return msg;
+}
+
+static void ipmi_bt_i2c_free_seq(struct ipmi_bt_i2c_master *master, u8 seq)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->lock, flags);
+	master->seq_msg_map[seq].msg = NULL;
+	spin_unlock_irqrestore(&master->lock, flags);
+}
+
+static void ipmi_bt_i2c_send_workfn(struct work_struct *work)
+{
+	struct ipmi_bt_i2c_smi_msg *smi_msg;
+	struct ipmi_bt_i2c_master *master;
+	struct ipmi_bt_i2c_msg bt_msg;
+	struct ipmi_smi_msg *msg;
+	size_t smi_msg_size;
+	unsigned long flags;
+
+	master = container_of(work, struct ipmi_bt_i2c_master,
+			      ipmi_bt_i2c_send_work);
+
+	msg = master->msg_to_send;
+	smi_msg_size = msg->data_size;
+	smi_msg = (struct ipmi_bt_i2c_smi_msg *) msg->data;
+
+	if (smi_msg_size > IPMI_BT_I2C_MAX_SMI_SIZE) {
+		ipmi_bt_i2c_error_reply(master, msg, IPMI_REQ_LEN_EXCEEDED_ERR);
+		return;
+	}
+
+	if (smi_msg_size < IPMI_BT_I2C_SMI_MSG_HEADER_SIZE) {
+		ipmi_bt_i2c_error_reply(master, msg, IPMI_REQ_LEN_INVALID_ERR);
+		return;
+	}
+
+	if (!ipmi_bt_i2c_assign_seq(master, msg, &bt_msg.seq)) {
+		ipmi_bt_i2c_error_reply(master, msg, IPMI_NODE_BUSY_ERR);
+		return;
+	}
+
+	bt_msg.len = ipmi_bt_i2c_smi_to_bt_len(smi_msg_size);
+	bt_msg.netfn_lun = smi_msg->netfn_lun;
+	bt_msg.cmd = smi_msg->cmd;
+	memcpy(bt_msg.payload, smi_msg->payload,
+	       ipmi_bt_i2c_payload_len(&bt_msg));
+
+	if (ipmi_bt_i2c_send_request(master, &bt_msg) < 0) {
+		ipmi_bt_i2c_free_seq(master, bt_msg.seq);
+		ipmi_bt_i2c_error_reply(master, msg, IPMI_BUS_ERR);
+	}
+
+	spin_lock_irqsave(&master->lock, flags);
+	master->msg_to_send = NULL;
+	spin_unlock_irqrestore(&master->lock, flags);
+}
+
+void ipmi_bt_i2c_recv_workfn(struct work_struct *work)
+{
+	struct ipmi_bt_i2c_smi_msg *smi_msg;
+	struct ipmi_bt_i2c_master *master;
+	struct ipmi_bt_i2c_msg bt_msg;
+	struct ipmi_smi_msg *msg;
+
+	master = container_of(work, struct ipmi_bt_i2c_master,
+			      ipmi_bt_i2c_recv_work);
+
+	if (ipmi_bt_i2c_receive_response(master, &bt_msg) < 0)
+		return;
+
+	if (bt_msg.len < IPMI_BT_I2C_LEN_MIN)
+		return;
+
+	msg = ipmi_bt_i2c_find_msg(master, bt_msg.seq);
+	if (!msg)
+		return;
+
+	ipmi_bt_i2c_free_seq(master, bt_msg.seq);
+
+	if (bt_msg.len < IPMI_BT_I2C_RESPONSE_LEN_MIN)
+		ipmi_bt_i2c_error_reply(master, msg, IPMI_ERR_MSG_TRUNCATED);
+
+	msg->rsp_size = ipmi_bt_i2c_bt_to_smi_len(&bt_msg);
+	smi_msg = (struct ipmi_bt_i2c_smi_msg *) msg->rsp;
+	smi_msg->netfn_lun = bt_msg.netfn_lun;
+	smi_msg->cmd = bt_msg.cmd;
+	memcpy(smi_msg->payload, bt_msg.payload,
+	       ipmi_bt_i2c_payload_len(&bt_msg));
+	ipmi_smi_msg_received(master->intf, msg);
+}
+
+static void ipmi_bt_i2c_sender(void *data, struct ipmi_smi_msg *msg)
+{
+	struct ipmi_bt_i2c_master *master = data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&master->lock, flags);
+	if (master->msg_to_send) {
+		/*
+		 * TODO(benjaminfair): Queue messages to send instead of only
+		 * keeping one.
+		 */
+		__ipmi_bt_i2c_error_reply(master, msg, IPMI_NODE_BUSY_ERR);
+	} else {
+		master->msg_to_send = msg;
+		schedule_work(&master->ipmi_bt_i2c_send_work);
+	}
+	spin_unlock_irqrestore(&master->lock, flags);
+}
+
+static void ipmi_bt_i2c_request_events(void *data)
+{
+	struct ipmi_bt_i2c_master *master = data;
+
+	schedule_work(&master->ipmi_bt_i2c_recv_work);
+}
+
+static void ipmi_bt_i2c_set_run_to_completion(void *data,
+					      bool run_to_completion)
+{
+}
+
+static void ipmi_bt_i2c_poll(void *data)
+{
+	struct ipmi_bt_i2c_master *master = data;
+
+	schedule_work(&master->ipmi_bt_i2c_recv_work);
+}
+
+static struct ipmi_smi_handlers ipmi_bt_i2c_smi_handlers = {
+	.owner			= THIS_MODULE,
+	.start_processing	= ipmi_bt_i2c_start_processing,
+	.sender			= ipmi_bt_i2c_sender,
+	.request_events		= ipmi_bt_i2c_request_events,
+	.set_run_to_completion	= ipmi_bt_i2c_set_run_to_completion,
+	.poll			= ipmi_bt_i2c_poll,
+};
+
+static int ipmi_bt_i2c_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct ipmi_bt_i2c_master *master;
+	int ret;
+
+	master = devm_kzalloc(&client->dev, sizeof(struct ipmi_bt_i2c_master),
+			      GFP_KERNEL);
+	if (!master)
+		return -ENOMEM;
+
+	spin_lock_init(&master->lock);
+	INIT_WORK(&master->ipmi_bt_i2c_recv_work, ipmi_bt_i2c_recv_workfn);
+	INIT_WORK(&master->ipmi_bt_i2c_send_work, ipmi_bt_i2c_send_workfn);
+	master->client = client;
+	i2c_set_clientdata(client, master);
+
+	/*
+	 * TODO(benjaminfair): read ipmi_device_id from BMC to determine version
+	 * information and be able to tell multiple BMCs apart
+	 */
+	ret = ipmi_register_smi(&ipmi_bt_i2c_smi_handlers, master,
+				&master->ipmi_id, &client->dev, 0);
+
+	return ret;
+}
+
+static int ipmi_bt_i2c_remove(struct i2c_client *client)
+{
+	struct ipmi_bt_i2c_master *master;
+
+	master = i2c_get_clientdata(client);
+	return ipmi_unregister_smi(master->intf);
+}
+
+static const struct acpi_device_id ipmi_bt_i2c_acpi_id[] = {
+	{"BTMA0001", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, ipmi_bt_i2c_acpi_id);
+
+static const struct i2c_device_id ipmi_bt_i2c_i2c_id[] = {
+	{"ipmi-bt-i2c", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ipmi_bt_i2c_i2c_id);
+
+static struct i2c_driver ipmi_bt_i2c_driver = {
+	.driver = {
+		.name = "ipmi-bt-i2c",
+		.acpi_match_table = ipmi_bt_i2c_acpi_id,
+	},
+	.id_table = ipmi_bt_i2c_i2c_id,
+	.probe = ipmi_bt_i2c_probe,
+	.remove = ipmi_bt_i2c_remove,
+};
+module_i2c_driver(ipmi_bt_i2c_driver);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("IPMI Block Transfer over I2C.");
+MODULE_LICENSE("GPL v2");
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

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

[Brendan Higgins]

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 <brendanhiggins@google.com>
---
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 000000000000..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 000000000000..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 000000000000..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 <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/ipmi_bmc.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/poll.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/wait.h>
+
+#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_head	list;
+	struct bt_msg		request;
+};
+
+struct bt_i2c_slave {
+	struct i2c_client	*client;
+	struct miscdevice	miscdev;
+	struct bt_msg		request;
+	struct list_head	request_queue;
+	atomic_t		request_queue_len;
+	struct bt_msg		response;
+	bool			response_in_progress;
+	size_t			msg_idx;
+	spinlock_t		lock;
+	wait_queue_head_t	wait_queue;
+	struct mutex		file_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,
+				atomic_read(&bt_slave->request_queue_len));
+		if (res)
+			return res;
+	}
+
+	spin_lock_irqsave(&bt_slave->lock, flags);
+	if (!atomic_read(&bt_slave->request_queue_len)) {
+		spin_unlock_irqrestore(&bt_slave->lock, flags);
+		if (non_blocking)
+			return -EAGAIN;
+		goto try_again;
+	}
+
+	if (list_empty(&bt_slave->request_queue)) {
+		pr_err(PFX "request_queue was empty despite nonzero request_queue_len\n");
+		return -EIO;
+	}
+	queue_elem = list_first_entry(&bt_slave->request_queue,
+				      struct bt_request_elem, list);
+	memcpy(bt_request, &queue_elem->request, sizeof(*bt_request));
+	list_del(&queue_elem->list);
+	kfree(queue_elem);
+	atomic_dec(&bt_slave->request_queue_len);
+	spin_unlock_irqrestore(&bt_slave->lock, flags);
+	return 0;
+}
+
+static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
+			    struct bt_msg *bt_response)
+{
+	int res;
+	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;
+	}
+
+	spin_lock_irqsave(&bt_slave->lock, flags);
+	if (bt_slave->response_in_progress) {
+		spin_unlock_irqrestore(&bt_slave->lock, flags);
+		if (non_blocking)
+			return -EAGAIN;
+		goto try_again;
+	}
+
+	memcpy(&bt_slave->response, bt_response, sizeof(*bt_response));
+	bt_slave->response_in_progress = true;
+	spin_unlock_irqrestore(&bt_slave->lock, flags);
+	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 *file, char __user *buf, size_t count,
+		       loff_t *ppos)
+{
+	struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
+	struct bt_msg msg;
+	ssize_t ret;
+
+	mutex_lock(&bt_slave->file_mutex);
+	ret = receive_bt_request(bt_slave, file->f_flags & O_NONBLOCK, &msg);
+	if (ret < 0)
+		goto out;
+	count = min_t(size_t, count, bt_msg_len(&msg));
+	if (copy_to_user(buf, &msg, count)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+out:
+	mutex_unlock(&bt_slave->file_mutex);
+	if (ret < 0)
+		return ret;
+	else
+		return count;
+}
+
+static ssize_t bt_write(struct file *file, const char __user *buf, size_t count,
+			loff_t *ppos)
+{
+	struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
+	struct bt_msg msg;
+	ssize_t ret;
+
+	if (count > sizeof(msg))
+		return -EINVAL;
+
+	if (copy_from_user(&msg, buf, count) || count < bt_msg_len(&msg))
+		return -EINVAL;
+
+	mutex_lock(&bt_slave->file_mutex);
+	ret = send_bt_response(bt_slave, file->f_flags & O_NONBLOCK, &msg);
+	mutex_unlock(&bt_slave->file_mutex);
+
+	if (ret < 0)
+		return ret;
+	else
+		return count;
+}
+
+static unsigned int bt_poll(struct file *file, poll_table *wait)
+{
+	struct bt_i2c_slave *bt_slave = to_bt_i2c_slave(file);
+	unsigned int mask = 0;
+
+	mutex_lock(&bt_slave->file_mutex);
+	poll_wait(file, &bt_slave->wait_queue, wait);
+
+	if (atomic_read(&bt_slave->request_queue_len))
+		mask |= POLLIN;
+	if (!bt_slave->response_in_progress)
+		mask |= POLLOUT;
+	mutex_unlock(&bt_slave->file_mutex);
+	return mask;
+}
+
+static const struct file_operations bt_fops = {
+	.owner		= THIS_MODULE,
+	.read		= bt_read,
+	.write		= bt_write,
+	.poll		= bt_poll,
+};
+
+/* Called with bt_slave->lock held. */
+static int handle_request(struct bt_i2c_slave *bt_slave)
+{
+	struct bt_request_elem *queue_elem;
+
+	if (atomic_read(&bt_slave->request_queue_len) >= request_queue_max_len)
+		return -EFAULT;
+	queue_elem = kmalloc(sizeof(*queue_elem), GFP_KERNEL);
+	if (!queue_elem)
+		return -ENOMEM;
+	memcpy(&queue_elem->request, &bt_slave->request, sizeof(struct bt_msg));
+	list_add(&queue_elem->list, &bt_slave->request_queue);
+	atomic_inc(&bt_slave->request_queue_len);
+	wake_up_all(&bt_slave->wait_queue);
+	return 0;
+}
+
+/* Called with bt_slave->lock held. */
+static int complete_response(struct bt_i2c_slave *bt_slave)
+{
+	/* Invalidate response in buffer to denote it having been sent. */
+	bt_slave->response.len = 0;
+	bt_slave->response_in_progress = false;
+	wake_up_all(&bt_slave->wait_queue);
+	return 0;
+}
+
+static int bt_i2c_slave_cb(struct i2c_client *client,
+			   enum i2c_slave_event event, u8 *val)
+{
+	struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
+	u8 *buf;
+
+	spin_lock(&bt_slave->lock);
+	switch (event) {
+	case I2C_SLAVE_WRITE_REQUESTED:
+		bt_slave->msg_idx = 0;
+		break;
+
+	case I2C_SLAVE_WRITE_RECEIVED:
+		buf = (u8 *) &bt_slave->request;
+		if (bt_slave->msg_idx >= sizeof(struct bt_msg))
+			break;
+
+		buf[bt_slave->msg_idx++] = *val;
+		if (bt_slave->msg_idx >= bt_msg_len(&bt_slave->request))
+			handle_request(bt_slave);
+		break;
+
+	case I2C_SLAVE_READ_REQUESTED:
+		buf = (u8 *) &bt_slave->response;
+		bt_slave->msg_idx = 0;
+		*val = buf[bt_slave->msg_idx];
+		/*
+		 * Do not increment buffer_idx here, because we don't know if
+		 * this byte will be actually used. Read Linux I2C slave docs
+		 * for details.
+		 */
+		break;
+
+	case I2C_SLAVE_READ_PROCESSED:
+		buf = (u8 *) &bt_slave->response;
+		if (bt_slave->response.len &&
+		    bt_slave->msg_idx < bt_msg_len(&bt_slave->response)) {
+			*val = buf[++bt_slave->msg_idx];
+		} else {
+			*val = 0;
+		}
+		if (bt_slave->msg_idx + 1 >= bt_msg_len(&bt_slave->response))
+			complete_response(bt_slave);
+		break;
+
+	case I2C_SLAVE_STOP:
+		bt_slave->msg_idx = 0;
+		break;
+
+	default:
+		break;
+	}
+	spin_unlock(&bt_slave->lock);
+
+	return 0;
+}
+
+static int bt_i2c_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct bt_i2c_slave *bt_slave;
+	int ret;
+
+	bt_slave = devm_kzalloc(&client->dev, sizeof(*bt_slave),
+				GFP_KERNEL);
+	if (!bt_slave)
+		return -ENOMEM;
+
+	spin_lock_init(&bt_slave->lock);
+	init_waitqueue_head(&bt_slave->wait_queue);
+	atomic_set(&bt_slave->request_queue_len, 0);
+	bt_slave->response_in_progress = false;
+	INIT_LIST_HEAD(&bt_slave->request_queue);
+
+	mutex_init(&bt_slave->file_mutex);
+
+	bt_slave->miscdev.minor = MISC_DYNAMIC_MINOR;
+	bt_slave->miscdev.name = DEVICE_NAME;
+	bt_slave->miscdev.fops = &bt_fops;
+	bt_slave->miscdev.parent = &client->dev;
+	ret = misc_register(&bt_slave->miscdev);
+	if (ret)
+		return ret;
+
+	bt_slave->client = client;
+	i2c_set_clientdata(client, bt_slave);
+	ret = i2c_slave_register(client, bt_i2c_slave_cb);
+	if (ret) {
+		misc_deregister(&bt_slave->miscdev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bt_i2c_remove(struct i2c_client *client)
+{
+	struct bt_i2c_slave *bt_slave = i2c_get_clientdata(client);
+
+	i2c_slave_unregister(client);
+	misc_deregister(&bt_slave->miscdev);
+	return 0;
+}
+
+static const struct i2c_device_id bt_i2c_id[] = {
+	{"ipmi-bmc-bt-i2c", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, bt_i2c_id);
+
+static struct i2c_driver bt_i2c_driver = {
+	.driver = {
+		.name		= "ipmi-bmc-bt-i2c",
+	},
+	.probe		= bt_i2c_probe,
+	.remove		= bt_i2c_remove,
+	.id_table	= bt_i2c_id,
+};
+module_i2c_driver(bt_i2c_driver);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("BMC-side IPMI Block Transfer over I2C.");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/ipmi_bmc.h b/include/linux/ipmi_bmc.h
new file mode 100644
index 000000000000..d0885c0bf190
--- /dev/null
+++ b/include/linux/ipmi_bmc.h
@@ -0,0 +1,76 @@
+/*
+ * 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.
+ */
+
+#ifndef __LINUX_IPMI_BMC_H
+#define __LINUX_IPMI_BMC_H
+
+#include <linux/bug.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#define BT_MSG_PAYLOAD_LEN_MAX 252
+
+/**
+ * struct bt_msg - Block Transfer IPMI message.
+ * @len: Length of the message, not including this field.
+ * @netfn_lun: 6-bit netfn field definining the category of message and 2-bit
+ *             lun field used for routing.
+ * @seq: Sequence number used to associate requests with responses.
+ * @cmd: Command within a netfn category.
+ * @payload: Variable length field. May have specific requirements based on
+ *           netfn/cmd pair.
+ *
+ * Use bt_msg_len() to determine the total length of a message (including
+ * the @len field) rather than reading it directly.
+ */
+struct bt_msg {
+	u8 len;
+	u8 netfn_lun;
+	u8 seq;
+	u8 cmd;
+	u8 payload[BT_MSG_PAYLOAD_LEN_MAX];
+} __packed;
+
+/**
+ * bt_msg_len() - Determine the total length of a Block Transfer message.
+ * @bt_msg: Pointer to the message.
+ *
+ * This function calculates the length of an IPMI Block Transfer message
+ * including the length field itself.
+ *
+ * Return: Length of @bt_msg.
+ */
+static inline u32 bt_msg_len(struct bt_msg *bt_msg)
+{
+	return bt_msg->len + 1;
+}
+
+/**
+ * bt_msg_payload_to_len() - Calculate the len field of a Block Transfer message
+ *                           given the length of the payload.
+ * @payload_len: Length of the payload.
+ *
+ * Return: len field of the Block Transfer message which contains this payload.
+ */
+static inline u8 bt_msg_payload_to_len(u8 payload_len)
+{
+	if (unlikely(payload_len > BT_MSG_PAYLOAD_LEN_MAX)) {
+		payload_len = BT_MSG_PAYLOAD_LEN_MAX;
+		WARN(1, "BT message payload is too large. Truncating to %u.\n",
+		     BT_MSG_PAYLOAD_LEN_MAX);
+	}
+	return payload_len + 3;
+}
+
+#endif /* __LINUX_IPMI_BMC_H */
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

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

[Brendan Higgins]

From: Benjamin Fair <benjaminfair@google.com>

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 <benjaminfair@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
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

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

[Corey Minyard]

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.

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

[Corey Minyard]

On 08/04/2017 08:18 PM, Brendan Higgins wrote:
> This patchset introduces IPMI Block Transfer over I2C (BT-I2C), which has the
> same semantics as IPMI Block Transfer except it done over I2C.
>
> For the OpenBMC people, this is based on an RFC:
> https://lists.ozlabs.org/pipermail/openbmc/2016-September/004505.html
>
> The documentation discusses the reason for this in greater detail, suffice it to
> say SSIF cannot be correctly implemented on some naive I2C devices. There are
> some additional reasons why we don't like SSIF, but those are again covered in
> the documentation for all those who are interested.

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

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

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

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

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

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

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

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

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

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

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

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

-corey

>
> In addition, since I am adding both host side and BMC side support, I figured
> that now is a good time to resolve the problem of where to put BMC side IPMI
> drivers; right now we have it (there is only one) in drivers/char/ipmi/ with the
> rest of the host side IPMI drivers, but I think it makes sense to put all of the
> host side IPMI drivers in one directory and all of the BMC side drivers in
> another, preferably in a way that does not effect all of the current OpenIPMI
> users. I have not created a MAINTAINERS entry for the new directory yet, as I
> figured there might be some discussion to be had about it.
>
> I have tested this patchset on the Aspeed 2500 EVB.
>
> Changes since previous update:
>    - Cleaned up some documentation.
>    - Added patch which moves the Aspeed BT-BMC driver to the new ipmi_bmc
>      directory.


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

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

[Brendan Higgins]

On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard <minyard@acm.org> 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

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

[Brendan Higgins]

On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard <minyard@acm.org> 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

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

[Corey Minyard]

On 08/07/2017 08:25 PM, Brendan Higgins wrote:
> On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard <minyard@acm.org> 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.
>>
>> 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.

I'm not too picky, either, I just didn't want someone to get confused.

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

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.

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

As long as you have a legitimate need, I don't have a problem with 
this.  The upper layer
of the driver can already handle this, there's just a small piece that 
interfaces to
the lower layer that needs to be modified.  And the ability to query the 
maximum
number of outstanding messages from the lower layer.

BT will be harder, the SI interface code would need more significant 
updates.


snip

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

I am not convinced that SSIF is not suitable for this.  It is true that 
the lack of NACK
support won't work right now, but for the current SSIF driver that is 
adding a condition
to a single if statement.  I would be happy to have that.  The current 
code returns an
error in this case, but it might be better to just ignore it, anyway.

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

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.

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.

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

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.

-corey


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

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

[Corey Minyard]

On 08/07/2017 08:25 PM, Brendan Higgins wrote:
> On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard <minyard-HInyCGIudOg@public.gmane.org> 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.
>>
>> 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.

I'm not too picky, either, I just didn't want someone to get confused.

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

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.

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

As long as you have a legitimate need, I don't have a problem with 
this.  The upper layer
of the driver can already handle this, there's just a small piece that 
interfaces to
the lower layer that needs to be modified.  And the ability to query the 
maximum
number of outstanding messages from the lower layer.

BT will be harder, the SI interface code would need more significant 
updates.


snip

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

I am not convinced that SSIF is not suitable for this.  It is true that 
the lack of NACK
support won't work right now, but for the current SSIF driver that is 
adding a condition
to a single if statement.  I would be happy to have that.  The current 
code returns an
error in this case, but it might be better to just ignore it, anyway.

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

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.

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.

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

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.

-corey


>>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Brendan Higgins]

> 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

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

[Brendan Higgins]

> 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

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

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 4706 bytes --]

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


[-- Attachment #2: ssif-openbmc.patch --]
[-- Type: text/x-patch, Size: 4842 bytes --]

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 11237e8..d467b1a 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -60,6 +60,11 @@
 
 #define IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD	0x57
 
+static u8 openbmc_iana[3] = { 0x10, 0x20, 0x30 };
+#define IPMI_OPENBMC_CAPABILITY_REQUEST_CMD	0x01
+#define SSIF_OPENBMC_REQUEST	0x80
+#define SSIF_OPENBMC_RESPONSE	0x81
+
 #define	SSIF_IPMI_REQUEST			2
 #define	SSIF_IPMI_MULTI_PART_REQUEST_START	6
 #define	SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE	7
@@ -224,6 +229,12 @@ struct ssif_info {
 	bool		    supports_alert;
 
 	/*
+	 * OpenBMC mode, with large message and sequence number
+	 * support.  An int because we use it as a number, too.
+	 */
+	unsigned int	    is_openbmc;
+
+	/*
 	 * Used to tell what we should do with alerts.  If we are
 	 * waiting on a response, read the data immediately.
 	 */
@@ -537,12 +548,13 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
 static void start_get(struct ssif_info *ssif_info)
 {
 	int rv;
+	int command = ssif_info->is_openbmc ? SSIF_OPENBMC_RESPONSE :
+			SSIF_IPMI_RESPONSE;
 
 	ssif_info->rtc_us_timer = 0;
 	ssif_info->multi_pos = 0;
 
-	rv = ssif_i2c_send(ssif_info, msg_done_handler, I2C_SMBUS_READ,
-			  SSIF_IPMI_RESPONSE,
+	rv = ssif_i2c_send(ssif_info, msg_done_handler, I2C_SMBUS_READ, command,
 			  ssif_info->recv, I2C_SMBUS_BLOCK_DATA);
 	if (rv < 0) {
 		/* request failed, just return the error. */
@@ -611,7 +623,8 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
 	 * start messing with driver states or the queues.
 	 */
 
-	if (result < 0) {
+	if (result < 0 || len < 3) {
+	ignore_msg:
 		ssif_info->retries_left--;
 		if (ssif_info->retries_left > 0) {
 			ssif_inc_stat(ssif_info, receive_retries);
@@ -633,7 +646,13 @@ static void msg_done_handler(struct ssif_info *ssif_info, int result,
 		goto continue_op;
 	}
 
-	if ((len > 1) && (ssif_info->multi_pos == 0)
+	if (ssif_info->is_openbmc) {
+		if (len < 4)
+			goto ignore_msg;
+		/* Eventually extract sequence number from data[0]. */
+		data++;
+		len--;
+	} else if ((len > 1) && (ssif_info->multi_pos == 0)
 				&& (data[0] == 0x00) && (data[1] == 0x01)) {
 		/* Start of multi-part read.  Start the next transaction. */
 		int i;
@@ -966,7 +985,11 @@ static int start_resend(struct ssif_info *ssif_info)
 
 	ssif_info->got_alert = false;
 
-	if (ssif_info->data_len > 32) {
+	if (ssif_info->is_openbmc) {
+		ssif_info->multi_data = NULL;
+		command = SSIF_OPENBMC_REQUEST;
+		ssif_info->data[0] = ssif_info->data_len;
+	} else if (ssif_info->data_len > 32) {
 		command = SSIF_IPMI_MULTI_PART_REQUEST_START;
 		ssif_info->multi_data = ssif_info->data;
 		ssif_info->multi_len = ssif_info->data_len;
@@ -1000,7 +1023,11 @@ static int start_send(struct ssif_info *ssif_info,
 		return -E2BIG;
 
 	ssif_info->retries_left = SSIF_SEND_RETRIES;
-	memcpy(ssif_info->data + 1, data, len);
+	memcpy(ssif_info->data + 1 + ssif_info->is_openbmc, data, len);
+	if (ssif_info->is_openbmc) {
+		ssif_info->data[1] = 0; /* Sequence number eventually. */
+		len++;
+	}
 	ssif_info->data_len = len;
 	return start_resend(ssif_info);
 }
@@ -1507,7 +1534,7 @@ static struct ipmi_fw_revision_range ssif_broken_alert_bmcs[] = {
 
 static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
-	unsigned char     msg[3];
+	unsigned char     msg[6];
 	unsigned char     *resp;
 	struct ssif_info   *ssif_info;
 	int               rv = 0;
@@ -1643,6 +1670,24 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		ssif_info->supports_pec = 0;
 	}
 
+	msg[0] = IPMI_NETFN_OEM_GROUP_REQUEST << 2;
+	msg[1] = IPMI_OPENBMC_CAPABILITY_REQUEST_CMD;
+	memcpy(msg, openbmc_iana, sizeof(openbmc_iana));
+	rv = do_cmd(client, 5, msg, &len, resp);
+	if (!rv && (len >= 3) && (resp[2] == 0)) {
+		/* It appears we have an OpenBMC device. */
+
+		/* Turn of multi support. */
+		ssif_info->multi_support = SSIF_NO_MULTI;
+		ssif_info->max_xmit_msg_size = 32;
+		ssif_info->max_recv_msg_size = 32;
+		if (len > 3)
+			ssif_info->max_xmit_msg_size = resp[3];
+		if (len > 4)
+			ssif_info->max_recv_msg_size = resp[4];
+		ssif_info->is_openbmc = 1;
+	}
+
 	/* Make sure the NMI timeout is cleared. */
 	msg[0] = IPMI_NETFN_APP_REQUEST << 2;
 	msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD;
diff --git a/include/uapi/linux/ipmi_msgdefs.h b/include/uapi/linux/ipmi_msgdefs.h
index df97e6e..bb0af2e 100644
--- a/include/uapi/linux/ipmi_msgdefs.h
+++ b/include/uapi/linux/ipmi_msgdefs.h
@@ -71,6 +71,9 @@
 #define IPMI_NETFN_FIRMWARE_REQUEST		0x08
 #define IPMI_NETFN_FIRMWARE_RESPONSE		0x09
 
+#define IPMI_NETFN_OEM_GROUP_REQUEST		0x2e
+#define IPMI_NETFN_OEM_GROUP_RESPONSE		0x2f
+
 /* The default slave address */
 #define IPMI_BMC_SLAVE_ADDR	0x20
 

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

[Brendan Higgins]

On Wed, Aug 9, 2017 at 7:26 PM, Corey Minyard <tcminyard@gmail.com> 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:
>> 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.

Sounds good

>
> Thanks,
>
> -corey
>

Cheers

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

[Brendan Higgins]

On Wed, Aug 9, 2017 at 7:26 PM, Corey Minyard <tcminyard@gmail.com> 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:
>> 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.

Sounds good

>
> Thanks,
>
> -corey
>

Cheers