[RFC v1 0/4] ipmi_bmc: framework for IPMI on BMCs

[RFC v1 0/4] ipmi_bmc: framework for IPMI on BMCs

[Brendan Higgins]

This introduces a framework for implementing the BMC side of the IPMI protocol,
roughly mirroring the host side OpenIPMI framework; it attempts to abstract away
hardware interfaces, such as Block Transfer interface hardware implementations
from IPMI command handlers.

It does this by implementing the traditional driver model of a bus with devices;
however, in this case a struct ipmi_bmc_bus represents a hardware interface,
where a struct ipmi_bmc_device represents a handler. A handler filters messages
by registering a function which returns whether a given message matches the
handler; it also has the concept of a default handler which is forwarded all
messages which are not matched by some other interface.

In this patchset, we introduce an example of a default handler: a misc device
file interface which implements the same interface as the the device file
interface used by the Aspeed BT driver.

Currently, OpenBMC handles all IPMI message routing and handling in userland;
the existing drivers simply provide a file interface for the hardware on the
device. In this patchset, we propose a common file interface to be shared by all
IPMI hardware interfaces, but also a framework for implementing handlers at the
kernel level, similar to how the existing OpenIPMI framework supports both
kernel users, as well as misc device file interface.

This patchset depends on the "ipmi: bt-i2c: added IPMI Block Transfer over I2C"
patchset, which can be found here:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1461960.html
However, I can fix this if desired.

Tested on the AST2500 EVB.

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

[Brendan Higgins]

From: Benjamin Fair <benjaminfair@google.com>

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

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

diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
index 8bff32b55c24..9c7cd48d899f 100644
--- a/drivers/char/ipmi_bmc/Makefile
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -2,5 +2,6 @@
 # Makefile for the ipmi bmc drivers.
 #
 
+obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
 obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
diff --git a/drivers/char/ipmi_bmc/ipmi_bmc.c b/drivers/char/ipmi_bmc/ipmi_bmc.c
new file mode 100644
index 000000000000..c1324ac9a83c
--- /dev/null
+++ b/drivers/char/ipmi_bmc/ipmi_bmc.c
@@ -0,0 +1,294 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/ipmi_bmc.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
+
+#define PFX "IPMI BMC core: "
+
+struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx()
+{
+	static struct ipmi_bmc_ctx global_ctx;
+
+	return &global_ctx;
+}
+
+int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
+			   struct bt_msg *bt_response)
+{
+	struct ipmi_bmc_bus *bus;
+	int ret = -ENODEV;
+
+	rcu_read_lock();
+	bus = rcu_dereference(ctx->bus);
+
+	if (bus)
+		ret = bus->send_response(bus, bt_response);
+
+	rcu_read_unlock();
+	return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_send_response);
+
+bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx)
+{
+	struct ipmi_bmc_bus *bus;
+	bool ret = false;
+
+	rcu_read_lock();
+	bus = rcu_dereference(ctx->bus);
+
+	if (bus)
+		ret = bus->is_response_open(bus);
+
+	rcu_read_unlock();
+	return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_is_response_open);
+
+int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
+			     struct ipmi_bmc_device *device_in)
+{
+	struct ipmi_bmc_device *device;
+
+	mutex_lock(&ctx->drivers_mutex);
+	/* Make sure it hasn't already been registered. */
+	list_for_each_entry(device, &ctx->devices, link) {
+		if (device == device_in) {
+			mutex_unlock(&ctx->drivers_mutex);
+			return -EINVAL;
+		}
+	}
+
+	list_add_rcu(&device_in->link, &ctx->devices);
+	mutex_unlock(&ctx->drivers_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_device);
+
+int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
+			       struct ipmi_bmc_device *device_in)
+{
+	struct ipmi_bmc_device *device;
+	bool found = false;
+
+	mutex_lock(&ctx->drivers_mutex);
+	/* Make sure it is currently registered. */
+	list_for_each_entry(device, &ctx->devices, link) {
+		if (device == device_in) {
+			found = true;
+			break;
+		}
+	}
+	if (!found) {
+		mutex_unlock(&ctx->drivers_mutex);
+		return -ENXIO;
+	}
+
+	list_del_rcu(&device_in->link);
+	mutex_unlock(&ctx->drivers_mutex);
+	synchronize_rcu();
+
+	return 0;
+}
+EXPORT_SYMBOL(ipmi_bmc_unregister_device);
+
+int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
+				     struct ipmi_bmc_device *device)
+{
+	int ret;
+
+	mutex_lock(&ctx->drivers_mutex);
+	if (!ctx->default_device) {
+		ctx->default_device = device;
+		ret = 0;
+	} else {
+		ret = -EBUSY;
+	}
+	mutex_unlock(&ctx->drivers_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_default_device);
+
+int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
+				       struct ipmi_bmc_device *device)
+{
+	int ret;
+
+	mutex_lock(&ctx->drivers_mutex);
+	if (ctx->default_device == device) {
+		ctx->default_device = NULL;
+		ret = 0;
+	} else {
+		ret = -ENXIO;
+	}
+	mutex_unlock(&ctx->drivers_mutex);
+	synchronize_rcu();
+
+	return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_unregister_default_device);
+
+static u8 errno_to_ccode(int errno)
+{
+	switch (errno) {
+	case EBUSY:
+		return 0xC0; /* Node Busy */
+	case EINVAL:
+		return 0xC1; /* Invalid Command */
+	case ETIMEDOUT:
+		return 0xC3; /* Timeout while processing command */
+	case ENOMEM:
+		return 0xC4; /* Out of space */
+	default:
+		return 0xFF; /* Unspecified error */
+	}
+}
+
+static void ipmi_bmc_send_error_response(struct ipmi_bmc_ctx *ctx,
+					 struct bt_msg *bt_request,
+					 u8 ccode)
+{
+	struct bt_msg error_response;
+	int ret;
+
+	/* Payload contains 1 byte for completion code */
+	error_response.len = bt_msg_payload_to_len(1);
+	error_response.netfn_lun = bt_request->netfn_lun |
+				   IPMI_NETFN_LUN_RESPONSE_MASK;
+	error_response.seq = bt_request->seq;
+	error_response.cmd = bt_request->cmd;
+	error_response.payload[0] = ccode;
+
+	/*
+	 * TODO(benjaminfair): Retry sending the response if it fails. The error
+	 * response might fail to send if another response is in progress. In
+	 * that case, the request will timeout rather than getting a more
+	 * specific completion code. This should buffer up responses and send
+	 * them when it can. Device drivers will generally handle error
+	 * reporting themselves; this code is only a fallback when that's not
+	 * available or when the drivers themselves fail.
+	 */
+	ret = ipmi_bmc_send_response(ctx, &error_response);
+	if (ret)
+		pr_warn(PFX "Failed to reply with completion code %u: ipmi_bmc_send_response returned %d\n",
+			(u32) ccode, ret);
+}
+
+void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
+			     struct bt_msg *bt_request)
+{
+	struct ipmi_bmc_device *default_device;
+	struct ipmi_bmc_device *device;
+	int ret = -EINVAL;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(device, &ctx->devices, link) {
+		if (device->match_request(device, bt_request)) {
+			ret = device->handle_request(device, bt_request);
+			goto out;
+		}
+	}
+
+	/* No specific handler found. Use default handler instead */
+	default_device = rcu_dereference(ctx->default_device);
+	if (default_device)
+		ret = default_device->handle_request(default_device,
+						     bt_request);
+
+out:
+	rcu_read_unlock();
+	if (ret)
+		ipmi_bmc_send_error_response(ctx, bt_request,
+					     errno_to_ccode(-ret));
+}
+EXPORT_SYMBOL(ipmi_bmc_handle_request);
+
+void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx)
+{
+	struct ipmi_bmc_device *default_device;
+	struct ipmi_bmc_device *device;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(device, &ctx->devices, link) {
+		device->signal_response_open(device);
+	}
+
+	default_device = rcu_dereference(ctx->default_device);
+	if (default_device)
+		default_device->signal_response_open(default_device);
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL(ipmi_bmc_signal_response_open);
+
+int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
+			  struct ipmi_bmc_bus *bus_in)
+{
+	int ret;
+
+	mutex_lock(&ctx->drivers_mutex);
+	if (!ctx->bus) {
+		ctx->bus = bus_in;
+		ret = 0;
+	} else {
+		ret = -EBUSY;
+	}
+	mutex_unlock(&ctx->drivers_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_register_bus);
+
+int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
+			    struct ipmi_bmc_bus *bus_in)
+{
+	int ret;
+
+	mutex_lock(&ctx->drivers_mutex);
+	/* Tried to unregister when another bus is registered */
+	if (ctx->bus == bus_in) {
+		ctx->bus = NULL;
+		ret = 0;
+	} else {
+		ret = -ENXIO;
+	}
+	mutex_unlock(&ctx->drivers_mutex);
+	synchronize_rcu();
+
+	return ret;
+}
+EXPORT_SYMBOL(ipmi_bmc_unregister_bus);
+
+static int __init ipmi_bmc_init(void)
+{
+	struct ipmi_bmc_ctx *ctx = ipmi_bmc_get_global_ctx();
+
+	mutex_init(&ctx->drivers_mutex);
+	INIT_LIST_HEAD(&ctx->devices);
+	return 0;
+}
+module_init(ipmi_bmc_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Benjamin Fair <benjaminfair@google.com>");
+MODULE_DESCRIPTION("Core IPMI driver for the BMC side");
diff --git a/include/linux/ipmi_bmc.h b/include/linux/ipmi_bmc.h
index d0885c0bf190..2b494a79ec14 100644
--- a/include/linux/ipmi_bmc.h
+++ b/include/linux/ipmi_bmc.h
@@ -20,6 +20,13 @@
 #include <linux/types.h>
 
 #define BT_MSG_PAYLOAD_LEN_MAX 252
+#define BT_MSG_SEQ_MAX 255
+
+/*
+ * The bit in this mask is set in the netfn_lun field of an IPMI message to
+ * indicate that it is a response.
+ */
+#define IPMI_NETFN_LUN_RESPONSE_MASK (1 << 2)
 
 /**
  * struct bt_msg - Block Transfer IPMI message.
@@ -42,6 +49,84 @@ struct bt_msg {
 	u8 payload[BT_MSG_PAYLOAD_LEN_MAX];
 } __packed;
 
+/**
+ * struct ipmi_bmc_device - Device driver that wants to receive ipmi requests.
+ * @link: Used to make a linked list of devices.
+ * @match_request: Used to determine whether a request can be handled by this
+ *                 device. Note that the matchers are checked in an arbitrary
+ *                 order.
+ * @handle_request: Called when a request is received for this device.
+ * @signal_response_open: Called when a response is done being sent and another
+ *                        device can send a message. Make sure to check that the
+ *                        bus isn't busy even after this is called, because all
+ *                        devices receive this call and another one may have
+ *                        already submitted a new response.
+ *
+ * This collection of callback functions represents an upper-level handler of
+ * IPMI requests.
+ *
+ * Note that the callbacks may be called from an interrupt context.
+ */
+struct ipmi_bmc_device {
+	struct list_head link;
+
+	bool (*match_request)(struct ipmi_bmc_device *device,
+			      struct bt_msg *bt_request);
+	int (*handle_request)(struct ipmi_bmc_device *device,
+			      struct bt_msg *bt_request);
+	void (*signal_response_open)(struct ipmi_bmc_device *device);
+};
+
+/**
+ * struct ipmi_bmc_bus - Bus driver that exchanges messages with the host.
+ * @send_response: Submits the given response to be sent to the host. May return
+ *                 -EBUSY if a response is already in progress, in which case
+ *                 the caller should wait for the response_open() callback to be
+ *                 called.
+ * @is_response_open: Determines whether a response can currently be sent. Note
+ *                    that &ipmi_bmc_bus->send_response() may still fail with
+ *                    -EBUSY even after this returns true.
+ *
+ * This collection of callback functions represents a lower-level driver which
+ * acts as a connection to the host.
+ */
+struct ipmi_bmc_bus {
+	int (*send_response)(struct ipmi_bmc_bus *bus,
+			     struct bt_msg *bt_response);
+	bool (*is_response_open)(struct ipmi_bmc_bus *bus);
+};
+
+/**
+ * struct ipmi_bmc_ctx - Context object used to interact with the IPMI BMC
+ *                       core driver.
+ * @bus: Pointer to the bus which is currently registered, or NULL for none.
+ * @default_device: Pointer to a device which will receive messages that match
+ *                  no other devices, or NULL if none is registered.
+ * @devices: List of devices which are currently registered, besides the default
+ *           device.
+ * @drivers_mutex: Mutex which protects against concurrent editing of the
+ *                 bus driver, default device driver, and devices list.
+ *
+ * This struct should only be modified by the IPMI BMC core code and not by bus
+ * or device drivers.
+ */
+struct ipmi_bmc_ctx {
+	struct ipmi_bmc_bus __rcu	*bus;
+	struct ipmi_bmc_device __rcu	*default_device;
+	struct list_head		devices;
+	struct mutex			drivers_mutex;
+};
+
+/**
+ * ipmi_bmc_get_global_ctx() - Get a pointer to the global ctx.
+ *
+ * This function gets a reference to the global ctx object which is used by
+ * bus and device drivers to interact with the IPMI BMC core driver.
+ *
+ * Return: Pointer to the global ctx object.
+ */
+struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx(void);
+
 /**
  * bt_msg_len() - Determine the total length of a Block Transfer message.
  * @bt_msg: Pointer to the message.
@@ -73,4 +158,103 @@ static inline u8 bt_msg_payload_to_len(u8 payload_len)
 	return payload_len + 3;
 }
 
+/**
+ * ipmi_bmc_send_response() - Send an IPMI response on the current bus.
+ * @bt_response: The response message to send.
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
+			   struct bt_msg *bt_response);
+/**
+ * ipmi_bmc_is_response_open() - Check whether we can currently send a new
+ *                               response.
+ *
+ * Note that even if this function returns true, ipmi_bmc_send_response() may
+ * still fail with -EBUSY if a response is submitted in the time between the two
+ * calls.
+ *
+ * Return: true if we can send a new response, false if one is already in
+ *         progress.
+ */
+bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx);
+/**
+ * ipmi_bmc_register_device() - Register a new device driver.
+ * @device: Pointer to the struct which represents this device,
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
+			     struct ipmi_bmc_device *device);
+/**
+ * ipmi_bmc_unregister_device() - Unregister an existing device driver.
+ * @device: Pointer to the struct which represents the existing device.
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
+			       struct ipmi_bmc_device *device);
+/**
+ * ipmi_bmc_register_default_device() - Register a new default device driver.
+ * @device: Pointer to the struct which represents this device,
+ *
+ * Make this device the default device. If none of the other devices match on a
+ * particular message, this device will receive it instead.  Note that only one
+ * default device may be registered at a time.
+ *
+ * This functionalisty is currently used to allow messages which aren't directly
+ * handled by the kernel to be sent to userspace and handled there.
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
+				     struct ipmi_bmc_device *device);
+/**
+ * ipmi_bmc_unregister_default_device() - Unregister the existing default device
+ *                                        driver.
+ * @device: Pointer to the struct which represents the existing device.
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
+				       struct ipmi_bmc_device *device);
+/**
+ * ipmi_bmc_handle_request() - Handle a new request that was received.
+ * @bt_request: The request that was received.
+ *
+ * This is called by the bus driver when it receives a new request message.
+ *
+ * Note that it may be called from an interrupt context.
+ */
+void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
+			    struct bt_msg *bt_request);
+/**
+ * ipmi_bmc_signal_response_open() - Notify the upper layer that a response can
+ *                                   be sent.
+ *
+ * This is called by the bus driver after it finishes sending a response and is
+ * ready to begin sending another one.
+ */
+void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx);
+/**
+ * ipmi_bmc_register_bus() - Register a new bus driver.
+ * @bus: Pointer to the struct which represents this bus.
+ *
+ * Register a bus driver to handle communication with the host.
+ *
+ * Only one bus driver can be registered at any time.
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
+			  struct ipmi_bmc_bus *bus);
+/**
+ * ipmi_bmc_unregister_bus() - Unregister an existing bus driver.
+ * @bus: Pointer to the struct which represents the existing bus.
+ *
+ * Return: 0 on success, negative errno otherwise.
+ */
+int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
+			    struct ipmi_bmc_bus *bus);
+
 #endif /* __LINUX_IPMI_BMC_H */
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[RFC v1 2/4] ipmi_bmc: device interface to IPMI BMC framework

[Brendan Higgins]

From: Benjamin Fair <benjaminfair@google.com>

This creates a char device which allows userspace programs to send and
receive IPMI messages. Messages are only routed to userspace if no other
kernel driver can handle them.

Signed-off-by: Benjamin Fair <benjaminfair@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 drivers/char/ipmi_bmc/Kconfig            |   6 +
 drivers/char/ipmi_bmc/Makefile           |   1 +
 drivers/char/ipmi_bmc/ipmi_bmc_devintf.c | 241 +++++++++++++++++++++++++++++++
 3 files changed, 248 insertions(+)
 create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc_devintf.c

diff --git a/drivers/char/ipmi_bmc/Kconfig b/drivers/char/ipmi_bmc/Kconfig
index b6af38455702..262a17866aa2 100644
--- a/drivers/char/ipmi_bmc/Kconfig
+++ b/drivers/char/ipmi_bmc/Kconfig
@@ -11,6 +11,12 @@ menuconfig IPMI_BMC
 
 if IPMI_BMC
 
+config IPMI_BMC_DEVICE_INTERFACE
+	tristate 'Device interface for BMC-side IPMI'
+	help
+	  This provides a file interface to the IPMI BMC core so userland
+	  processes may use IPMI.
+
 config IPMI_BMC_BT_I2C
 	depends on I2C
 	select I2C_SLAVE
diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
index 9c7cd48d899f..ead8abffbd11 100644
--- a/drivers/char/ipmi_bmc/Makefile
+++ b/drivers/char/ipmi_bmc/Makefile
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
+obj-$(CONFIG_IPMI_BMC_DEVICE_INTERFACE) += ipmi_bmc_devintf.o
 obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
 obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_devintf.c b/drivers/char/ipmi_bmc/ipmi_bmc_devintf.c
new file mode 100644
index 000000000000..2421237ed575
--- /dev/null
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_devintf.c
@@ -0,0 +1,241 @@
+/*
+ * 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/atomic.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/io.h>
+#include <linux/ipmi_bmc.h>
+#include <linux/kfifo.h>
+#include <linux/log2.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/poll.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+#include <linux/uaccess.h>
+#include <linux/wait.h>
+
+#define PFX "IPMI BMC devintf: "
+
+#define DEVICE_NAME "ipmi-bt-host"
+
+/* Must be a power of two */
+#define REQUEST_FIFO_SIZE roundup_pow_of_two(BT_MSG_SEQ_MAX)
+
+struct bmc_devintf_data {
+	struct miscdevice	miscdev;
+	struct ipmi_bmc_device	bmc_device;
+	struct ipmi_bmc_ctx	*bmc_ctx;
+	wait_queue_head_t	wait_queue;
+	/* FIFO of waiting messages */
+	DECLARE_KFIFO(requests, struct bt_msg, REQUEST_FIFO_SIZE);
+};
+
+static inline struct bmc_devintf_data *file_to_bmc_devintf_data(
+		struct file *file)
+{
+	return container_of(file->private_data, struct bmc_devintf_data,
+			    miscdev);
+}
+
+static ssize_t ipmi_bmc_devintf_read(struct file *file, char __user *buf,
+				     size_t count, loff_t *ppos)
+{
+	struct bmc_devintf_data *devintf_data = file_to_bmc_devintf_data(file);
+	bool non_blocking = file->f_flags & O_NONBLOCK;
+	struct bt_msg msg;
+
+	if (non_blocking && kfifo_is_empty(&devintf_data->requests)) {
+		return -EAGAIN;
+	} else if (!non_blocking) {
+		if (wait_event_interruptible(devintf_data->wait_queue,
+				!kfifo_is_empty(&devintf_data->requests)))
+			return -ERESTARTSYS;
+	}
+
+	/* TODO(benjaminfair): eliminate this extra copy */
+	if (unlikely(!kfifo_get(&devintf_data->requests, &msg))) {
+		pr_err(PFX "Unable to read request from fifo\n");
+		return -EIO;
+	}
+
+	/* TODO(benjaminfair): handle partial reads of a message */
+	if (count > bt_msg_len(&msg))
+		count = bt_msg_len(&msg);
+
+	if (copy_to_user(buf, &msg, count))
+		return -EFAULT;
+
+	return count;
+}
+
+static ssize_t ipmi_bmc_devintf_write(struct file *file, const char __user *buf,
+				      size_t count, loff_t *ppos)
+{
+	struct bmc_devintf_data *devintf_data = file_to_bmc_devintf_data(file);
+	bool non_blocking = file->f_flags & O_NONBLOCK;
+	struct bt_msg msg;
+	ssize_t ret = 0;
+
+	if (count > sizeof(struct bt_msg))
+		return -EINVAL;
+
+	if (copy_from_user(&msg, buf, count))
+		return -EFAULT;
+
+	if (count != bt_msg_len(&msg))
+		return -EINVAL;
+
+	ret = ipmi_bmc_send_response(devintf_data->bmc_ctx, &msg);
+
+	/* Try again if blocking is allowed */
+	while (!non_blocking && ret == -EAGAIN) {
+		if (wait_event_interruptible(devintf_data->wait_queue,
+				ipmi_bmc_is_response_open(
+						devintf_data->bmc_ctx)))
+			return -ERESTARTSYS;
+
+		ret = ipmi_bmc_send_response(devintf_data->bmc_ctx, &msg);
+	}
+
+	if (ret < 0)
+		return ret;
+	else
+		return count;
+}
+
+static unsigned int ipmi_bmc_devintf_poll(struct file *file, poll_table *wait)
+{
+	struct bmc_devintf_data *devintf_data = file_to_bmc_devintf_data(file);
+	unsigned int mask = 0;
+
+	poll_wait(file, &devintf_data->wait_queue, wait);
+
+	if (!kfifo_is_empty(&devintf_data->requests))
+		mask |= POLLIN;
+	if (ipmi_bmc_is_response_open(devintf_data->bmc_ctx))
+		mask |= POLLOUT;
+
+	return mask;
+}
+
+static const struct file_operations ipmi_bmc_fops = {
+	.owner		= THIS_MODULE,
+	.read		= ipmi_bmc_devintf_read,
+	.write		= ipmi_bmc_devintf_write,
+	.poll		= ipmi_bmc_devintf_poll,
+};
+
+static inline struct bmc_devintf_data *device_to_bmc_devintf_data(
+		struct ipmi_bmc_device *device)
+{
+	return container_of(device, struct bmc_devintf_data, bmc_device);
+}
+
+static int ipmi_bmc_devintf_handle_request(struct ipmi_bmc_device *device,
+					   struct bt_msg *bt_request)
+{
+	struct bmc_devintf_data *devintf_data =
+		device_to_bmc_devintf_data(device);
+
+	if (!bt_request->len)
+		return -EINVAL;
+
+	if (!kfifo_put(&devintf_data->requests, *bt_request))
+		return -EBUSY;
+
+	wake_up_all(&devintf_data->wait_queue);
+
+	return 0;
+}
+
+static bool ipmi_bmc_devintf_match_request(struct ipmi_bmc_device *device,
+					   struct bt_msg *bt_request)
+{
+	/* Since this is a default device, match all requests */
+	return true;
+}
+
+static void ipmi_bmc_devintf_signal_response_open(
+		struct ipmi_bmc_device *device)
+{
+	struct bmc_devintf_data *devintf_data =
+		device_to_bmc_devintf_data(device);
+
+	wake_up_all(&devintf_data->wait_queue);
+}
+
+/*
+ * TODO: if we want to support multiple interfaces, initialize this global
+ * variable elsewhere
+ */
+static struct bmc_devintf_data *devintf_data;
+
+static int __init init_ipmi_bmc_devintf(void)
+{
+	int ret;
+
+	devintf_data = kzalloc(sizeof(*devintf_data), GFP_KERNEL);
+	if (!devintf_data)
+		return -ENOMEM;
+
+	init_waitqueue_head(&devintf_data->wait_queue);
+	INIT_KFIFO(devintf_data->requests);
+
+	devintf_data->bmc_device.handle_request =
+		ipmi_bmc_devintf_handle_request;
+	devintf_data->bmc_device.match_request =
+		ipmi_bmc_devintf_match_request;
+	devintf_data->bmc_device.signal_response_open =
+		ipmi_bmc_devintf_signal_response_open;
+
+	devintf_data->miscdev.minor = MISC_DYNAMIC_MINOR;
+	devintf_data->miscdev.name = DEVICE_NAME;
+	devintf_data->miscdev.fops = &ipmi_bmc_fops;
+
+	devintf_data->bmc_ctx = ipmi_bmc_get_global_ctx();
+
+	ret = ipmi_bmc_register_default_device(devintf_data->bmc_ctx,
+					       &devintf_data->bmc_device);
+	if (ret) {
+		pr_err(PFX "unable to register IPMI BMC device\n");
+		return ret;
+	}
+
+	ret = misc_register(&devintf_data->miscdev);
+	if (ret) {
+		ipmi_bmc_unregister_default_device(devintf_data->bmc_ctx,
+						   &devintf_data->bmc_device);
+		pr_err(PFX "unable to register misc device\n");
+		return ret;
+	}
+
+	pr_info(PFX "initialized\n");
+	return 0;
+}
+module_init(init_ipmi_bmc_devintf);
+
+static void __exit exit_ipmi_bmc_devintf(void)
+{
+	misc_deregister(&devintf_data->miscdev);
+	WARN_ON(ipmi_bmc_unregister_default_device(devintf_data->bmc_ctx,
+						   &devintf_data->bmc_device));
+}
+module_exit(exit_ipmi_bmc_devintf);
+
+MODULE_AUTHOR("Benjamin Fair <benjaminfair@google.com>");
+MODULE_DESCRIPTION("Device file interface to IPMI Block Transfer core.");
+MODULE_LICENSE("GPL v2");
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

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

[Brendan Higgins]

From: Benjamin Fair <benjaminfair@google.com>

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

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

diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
index 686b83fa42a4..6665aa9d4300 100644
--- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_i2c.c
@@ -14,102 +14,51 @@
 #include <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 ipmi_bmc_bus	bus;
 	struct i2c_client	*client;
-	struct miscdevice	miscdev;
+	struct ipmi_bmc_ctx	*bmc_ctx;
 	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)
+static bool bt_i2c_is_response_open(struct ipmi_bmc_bus *bus)
 {
-	int res;
+	struct bt_i2c_slave *bt_slave;
+	bool response_in_progress;
 	unsigned long flags;
-	struct bt_request_elem *queue_elem;
-
-	if (!non_blocking) {
-try_again:
-		res = wait_event_interruptible(
-				bt_slave->wait_queue,
-				atomic_read(&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;
-	}
+	bt_slave = container_of(bus, struct bt_i2c_slave, bus);
 
-	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_lock_irqsave(&bt_slave->lock, flags);
+	response_in_progress = bt_slave->response_in_progress;
 	spin_unlock_irqrestore(&bt_slave->lock, flags);
-	return 0;
+
+	return !response_in_progress;
 }
 
-static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
-			    struct bt_msg *bt_response)
+static int bt_i2c_send_response(struct ipmi_bmc_bus *bus,
+				struct bt_msg *bt_response)
 {
-	int res;
+	struct bt_i2c_slave *bt_slave;
 	unsigned long flags;
 
-	if (!non_blocking) {
-try_again:
-		res = wait_event_interruptible(bt_slave->wait_queue,
-					       !bt_slave->response_in_progress);
-		if (res)
-			return res;
-	}
+	bt_slave = container_of(bus, struct bt_i2c_slave, bus);
 
 	spin_lock_irqsave(&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;
+		return -EAGAIN;
 	}
 
 	memcpy(&bt_slave->response, bt_response, sizeof(*bt_response));
@@ -118,106 +67,13 @@ static int send_bt_response(struct bt_i2c_slave *bt_slave, bool non_blocking,
 	return 0;
 }
 
-static inline struct bt_i2c_slave *to_bt_i2c_slave(struct file *file)
-{
-	return container_of(file->private_data, struct bt_i2c_slave, miscdev);
-}
-
-static ssize_t bt_read(struct file *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);
+	ipmi_bmc_signal_response_open(bt_slave->bmc_ctx);
 	return 0;
 }
 
@@ -240,7 +96,8 @@ static int bt_i2c_slave_cb(struct i2c_client *client,
 
 		buf[bt_slave->msg_idx++] = *val;
 		if (bt_slave->msg_idx >= bt_msg_len(&bt_slave->request))
-			handle_request(bt_slave);
+			ipmi_bmc_handle_request(bt_slave->bmc_ctx,
+						&bt_slave->request);
 		break;
 
 	case I2C_SLAVE_READ_REQUESTED:
@@ -290,26 +147,24 @@ static int bt_i2c_probe(struct i2c_client *client,
 		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);
+	bt_slave->bus.send_response = bt_i2c_send_response;
+	bt_slave->bus.is_response_open = bt_i2c_is_response_open;
 
-	mutex_init(&bt_slave->file_mutex);
+	bt_slave->bmc_ctx = ipmi_bmc_get_global_ctx();
 
-	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)
+	ret = ipmi_bmc_register_bus(bt_slave->bmc_ctx, &bt_slave->bus);
+	if (ret) {
+		pr_err(PFX "Failed to register IPMI BMC bus\n");
 		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);
+		ipmi_bmc_unregister_bus(bt_slave->bmc_ctx, &bt_slave->bus);
 		return ret;
 	}
 
@@ -321,8 +176,7 @@ 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;
+	return ipmi_bmc_unregister_bus(bt_slave->bmc_ctx, &bt_slave->bus);
 }
 
 static const struct i2c_device_id bt_i2c_id[] = {
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

[RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Brendan Higgins]

From: Benjamin Fair <benjaminfair@google.com>

The driver was handling interaction with userspace on its own. This
patch changes it to use the functionality of the ipmi_bmc framework
instead.

Note that this removes the ability for the BMC to set SMS_ATN by making
an ioctl. If this functionality is required, it can be added back in
with a later patch.

Signed-off-by: Benjamin Fair <benjaminfair@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c | 258 +++++++++--------------------
 include/uapi/linux/bt-bmc.h                |  18 --
 2 files changed, 74 insertions(+), 202 deletions(-)
 delete mode 100644 include/uapi/linux/bt-bmc.h

diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
index 70d434bc1cbf..7c8082c511ee 100644
--- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
+++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
@@ -7,25 +7,19 @@
  * 2 of the License, or (at your option) any later version.
  */
 
-#include <linux/atomic.h>
-#include <linux/bt-bmc.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/ipmi_bmc.h>
 #include <linux/mfd/syscon.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/poll.h>
 #include <linux/regmap.h>
 #include <linux/sched.h>
 #include <linux/timer.h>
 
-/*
- * This is a BMC device used to communicate to the host
- */
-#define DEVICE_NAME	"ipmi-bt-host"
+#define DEVICE_NAME "ipmi-bmc-bt-aspeed"
 
 #define BT_IO_BASE	0xe4
 #define BT_IRQ		10
@@ -61,18 +55,17 @@
 #define BT_BMC_BUFFER_SIZE 256
 
 struct bt_bmc {
+	struct ipmi_bmc_bus	bus;
 	struct device		dev;
-	struct miscdevice	miscdev;
+	struct ipmi_bmc_ctx	*bmc_ctx;
+	struct bt_msg		request;
 	struct regmap		*map;
 	int			offset;
 	int			irq;
-	wait_queue_head_t	queue;
 	struct timer_list	poll_timer;
-	struct mutex		mutex;
+	spinlock_t		lock;
 };
 
-static atomic_t open_count = ATOMIC_INIT(0);
-
 static const struct regmap_config bt_regmap_cfg = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -158,27 +151,28 @@ static ssize_t bt_writen(struct bt_bmc *bt_bmc, u8 *buf, size_t n)
 	return n;
 }
 
+/* TODO(benjaminfair): support ioctl BT_BMC_IOCTL_SMS_ATN */
 static void set_sms_atn(struct bt_bmc *bt_bmc)
 {
 	bt_outb(bt_bmc, BT_CTRL_SMS_ATN, BT_CTRL);
 }
 
-static struct bt_bmc *file_bt_bmc(struct file *file)
+/* Called with bt_bmc->lock held */
+static bool __is_request_avail(struct bt_bmc *bt_bmc)
 {
-	return container_of(file->private_data, struct bt_bmc, miscdev);
+	return bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN;
 }
 
-static int bt_bmc_open(struct inode *inode, struct file *file)
+static bool is_request_avail(struct bt_bmc *bt_bmc)
 {
-	struct bt_bmc *bt_bmc = file_bt_bmc(file);
+	unsigned long flags;
+	bool result;
 
-	if (atomic_inc_return(&open_count) == 1) {
-		clr_b_busy(bt_bmc);
-		return 0;
-	}
+	spin_lock_irqsave(&bt_bmc->lock, flags);
+	result = __is_request_avail(bt_bmc);
+	spin_unlock_irqrestore(&bt_bmc->lock, flags);
 
-	atomic_dec(&open_count);
-	return -EBUSY;
+	return result;
 }
 
 /*
@@ -194,67 +188,43 @@ static int bt_bmc_open(struct inode *inode, struct file *file)
  *    Length  NetFn/LUN  Seq     Cmd     Data
  *
  */
-static ssize_t bt_bmc_read(struct file *file, char __user *buf,
-			   size_t count, loff_t *ppos)
+static void get_request(struct bt_bmc *bt_bmc)
 {
-	struct bt_bmc *bt_bmc = file_bt_bmc(file);
-	u8 len;
-	int len_byte = 1;
-	u8 kbuffer[BT_BMC_BUFFER_SIZE];
-	ssize_t ret = 0;
-	ssize_t nread;
+	u8 *request_buf = (u8 *) &bt_bmc->request;
+	unsigned long flags;
 
-	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT;
+	spin_lock_irqsave(&bt_bmc->lock, flags);
 
-	WARN_ON(*ppos);
-
-	if (wait_event_interruptible(bt_bmc->queue,
-				     bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))
-		return -ERESTARTSYS;
-
-	mutex_lock(&bt_bmc->mutex);
-
-	if (unlikely(!(bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))) {
-		ret = -EIO;
-		goto out_unlock;
+	if (!__is_request_avail(bt_bmc)) {
+		spin_unlock_irqrestore(&bt_bmc->lock, flags);
+		return;
 	}
 
 	set_b_busy(bt_bmc);
 	clr_h2b_atn(bt_bmc);
 	clr_rd_ptr(bt_bmc);
 
-	/*
-	 * The BT frames start with the message length, which does not
-	 * include the length byte.
-	 */
-	kbuffer[0] = bt_read(bt_bmc);
-	len = kbuffer[0];
-
-	/* We pass the length back to userspace as well */
-	if (len + 1 > count)
-		len = count - 1;
-
-	while (len) {
-		nread = min_t(ssize_t, len, sizeof(kbuffer) - len_byte);
-
-		bt_readn(bt_bmc, kbuffer + len_byte, nread);
-
-		if (copy_to_user(buf, kbuffer, nread + len_byte)) {
-			ret = -EFAULT;
-			break;
-		}
-		len -= nread;
-		buf += nread + len_byte;
-		ret += nread + len_byte;
-		len_byte = 0;
-	}
+	bt_bmc->request.len = bt_read(bt_bmc);
+	bt_readn(bt_bmc, request_buf + 1, bt_bmc->request.len);
 
 	clr_b_busy(bt_bmc);
+	ipmi_bmc_handle_request(bt_bmc->bmc_ctx, &bt_bmc->request);
+
+	spin_unlock_irqrestore(&bt_bmc->lock, flags);
+}
+
+static bool bt_bmc_is_response_open(struct ipmi_bmc_bus *bus)
+{
+	struct bt_bmc *bt_bmc = container_of(bus, struct bt_bmc, bus);
+	bool response_in_progress;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bt_bmc->lock, flags);
+	response_in_progress = bt_inb(bt_bmc, BT_CTRL) & (BT_CTRL_H_BUSY |
+							  BT_CTRL_B2H_ATN);
+	spin_unlock_irqrestore(&bt_bmc->lock, flags);
 
-out_unlock:
-	mutex_unlock(&bt_bmc->mutex);
-	return ret;
+	return !response_in_progress;
 }
 
 /*
@@ -263,122 +233,38 @@ static ssize_t bt_bmc_read(struct file *file, char __user *buf,
  *    Byte 1  Byte 2     Byte 3  Byte 4  Byte 5  Byte 6:N
  *    Length  NetFn/LUN  Seq     Cmd     Code    Data
  */
-static ssize_t bt_bmc_write(struct file *file, const char __user *buf,
-			    size_t count, loff_t *ppos)
+static int bt_bmc_send_response(struct ipmi_bmc_bus *bus,
+				struct bt_msg *bt_response)
 {
-	struct bt_bmc *bt_bmc = file_bt_bmc(file);
-	u8 kbuffer[BT_BMC_BUFFER_SIZE];
-	ssize_t ret = 0;
-	ssize_t nwritten;
+	struct bt_bmc *bt_bmc = container_of(bus, struct bt_bmc, bus);
+	unsigned long flags;
 
-	/*
-	 * send a minimum response size
-	 */
-	if (count < 5)
-		return -EINVAL;
-
-	if (!access_ok(VERIFY_READ, buf, count))
-		return -EFAULT;
-
-	WARN_ON(*ppos);
-
-	/*
-	 * There's no interrupt for clearing bmc busy so we have to
-	 * poll
-	 */
-	if (wait_event_interruptible(bt_bmc->queue,
-				     !(bt_inb(bt_bmc, BT_CTRL) &
-				       (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))))
-		return -ERESTARTSYS;
-
-	mutex_lock(&bt_bmc->mutex);
-
-	if (unlikely(bt_inb(bt_bmc, BT_CTRL) &
-		     (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))) {
-		ret = -EIO;
-		goto out_unlock;
+	spin_lock_irqsave(&bt_bmc->lock, flags);
+	if (!bt_bmc_is_response_open(bus)) {
+		spin_unlock_irqrestore(&bt_bmc->lock, flags);
+		return -EAGAIN;
 	}
 
 	clr_wr_ptr(bt_bmc);
-
-	while (count) {
-		nwritten = min_t(ssize_t, count, sizeof(kbuffer));
-		if (copy_from_user(&kbuffer, buf, nwritten)) {
-			ret = -EFAULT;
-			break;
-		}
-
-		bt_writen(bt_bmc, kbuffer, nwritten);
-
-		count -= nwritten;
-		buf += nwritten;
-		ret += nwritten;
-	}
-
+	bt_writen(bt_bmc, (u8 *) bt_response, bt_msg_len(bt_response));
 	set_b2h_atn(bt_bmc);
 
-out_unlock:
-	mutex_unlock(&bt_bmc->mutex);
-	return ret;
-}
-
-static long bt_bmc_ioctl(struct file *file, unsigned int cmd,
-			 unsigned long param)
-{
-	struct bt_bmc *bt_bmc = file_bt_bmc(file);
-
-	switch (cmd) {
-	case BT_BMC_IOCTL_SMS_ATN:
-		set_sms_atn(bt_bmc);
-		return 0;
-	}
-	return -EINVAL;
-}
-
-static int bt_bmc_release(struct inode *inode, struct file *file)
-{
-	struct bt_bmc *bt_bmc = file_bt_bmc(file);
-
-	atomic_dec(&open_count);
-	set_b_busy(bt_bmc);
+	spin_unlock_irqrestore(&bt_bmc->lock, flags);
 	return 0;
 }
 
-static unsigned int bt_bmc_poll(struct file *file, poll_table *wait)
-{
-	struct bt_bmc *bt_bmc = file_bt_bmc(file);
-	unsigned int mask = 0;
-	u8 ctrl;
-
-	poll_wait(file, &bt_bmc->queue, wait);
-
-	ctrl = bt_inb(bt_bmc, BT_CTRL);
-
-	if (ctrl & BT_CTRL_H2B_ATN)
-		mask |= POLLIN;
-
-	if (!(ctrl & (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN)))
-		mask |= POLLOUT;
-
-	return mask;
-}
-
-static const struct file_operations bt_bmc_fops = {
-	.owner		= THIS_MODULE,
-	.open		= bt_bmc_open,
-	.read		= bt_bmc_read,
-	.write		= bt_bmc_write,
-	.release	= bt_bmc_release,
-	.poll		= bt_bmc_poll,
-	.unlocked_ioctl	= bt_bmc_ioctl,
-};
-
 static void poll_timer(unsigned long data)
 {
 	struct bt_bmc *bt_bmc = (void *)data;
 
 	bt_bmc->poll_timer.expires += msecs_to_jiffies(500);
-	wake_up(&bt_bmc->queue);
+
+	if (bt_bmc_is_response_open(&bt_bmc->bus))
+		ipmi_bmc_signal_response_open(bt_bmc->bmc_ctx);
+
+	if (is_request_avail(bt_bmc))
+		get_request(bt_bmc);
+
 	add_timer(&bt_bmc->poll_timer);
 }
 
@@ -399,7 +285,12 @@ static irqreturn_t bt_bmc_irq(int irq, void *arg)
 	/* ack pending IRQs */
 	regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg);
 
-	wake_up(&bt_bmc->queue);
+	if (bt_bmc_is_response_open(&bt_bmc->bus))
+		ipmi_bmc_signal_response_open(bt_bmc->bmc_ctx);
+
+	if (is_request_avail(bt_bmc))
+		get_request(bt_bmc);
+
 	return IRQ_HANDLED;
 }
 
@@ -474,16 +365,14 @@ static int bt_bmc_probe(struct platform_device *pdev)
 			return rc;
 	}
 
-	mutex_init(&bt_bmc->mutex);
-	init_waitqueue_head(&bt_bmc->queue);
+	spin_lock_init(&bt_bmc->lock);
+	bt_bmc->bus.send_response = bt_bmc_send_response;
+	bt_bmc->bus.is_response_open = bt_bmc_is_response_open;
+	bt_bmc->bmc_ctx = ipmi_bmc_get_global_ctx();
 
-	bt_bmc->miscdev.minor	= MISC_DYNAMIC_MINOR,
-		bt_bmc->miscdev.name	= DEVICE_NAME,
-		bt_bmc->miscdev.fops	= &bt_bmc_fops,
-		bt_bmc->miscdev.parent = dev;
-	rc = misc_register(&bt_bmc->miscdev);
+	rc = ipmi_bmc_register_bus(bt_bmc->bmc_ctx, &bt_bmc->bus);
 	if (rc) {
-		dev_err(dev, "Unable to register misc device\n");
+		dev_err(dev, "Unable to register IPMI BMC bus\n");
 		return rc;
 	}
 
@@ -514,11 +403,12 @@ static int bt_bmc_probe(struct platform_device *pdev)
 static int bt_bmc_remove(struct platform_device *pdev)
 {
 	struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
+	int rc;
 
-	misc_deregister(&bt_bmc->miscdev);
+	rc = ipmi_bmc_unregister_bus(bt_bmc->bmc_ctx, &bt_bmc->bus);
 	if (!bt_bmc->irq)
 		del_timer_sync(&bt_bmc->poll_timer);
-	return 0;
+	return rc;
 }
 
 static const struct of_device_id bt_bmc_match[] = {
diff --git a/include/uapi/linux/bt-bmc.h b/include/uapi/linux/bt-bmc.h
deleted file mode 100644
index d9ec766a63d0..000000000000
--- a/include/uapi/linux/bt-bmc.h
+++ /dev/null
@@ -1,18 +0,0 @@
-/*
- * Copyright (c) 2015-2016, IBM Corporation.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version
- * 2 of the License, or (at your option) any later version.
- */
-
-#ifndef _UAPI_LINUX_BT_BMC_H
-#define _UAPI_LINUX_BT_BMC_H
-
-#include <linux/ioctl.h>
-
-#define __BT_BMC_IOCTL_MAGIC	0xb1
-#define BT_BMC_IOCTL_SMS_ATN	_IO(__BT_BMC_IOCTL_MAGIC, 0x00)
-
-#endif /* _UAPI_LINUX_BT_BMC_H */
-- 
2.14.0.rc1.383.gd1ce394fe2-goog

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Chris Austen]

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


"openbmc" <openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org> wrote on
08/07/2017 10:53:01 PM:

> From: Brendan Higgins <brendanhiggins@google.com>
> To: minyard@acm.org, benjaminfair@google.com, clg@kaod.org,
> joel@jms.id.au, andrew@aj.id.au
> Cc: openipmi-developer@lists.sourceforge.net, Brendan Higgins
> <brendanhiggins@google.com>, openbmc@lists.ozlabs.org, linux-
> kernel@vger.kernel.org
> Date: 08/07/2017 10:55 PM
> Subject: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC
framework
> Sent by: "openbmc" <openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org>
>
> From: Benjamin Fair <benjaminfair@google.com>
>
> The driver was handling interaction with userspace on its own. This
> patch changes it to use the functionality of the ipmi_bmc framework
> instead.
>
> Note that this removes the ability for the BMC to set SMS_ATN by making
> an ioctl. If this functionality is required, it can be added back in
> with a later patch.


Isn't SMS_ATN the way a BMC initiates an IPMI message to the OS?  Say for
instance a request to shutdown?


>
> Signed-off-by: Benjamin Fair <benjaminfair@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>  drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c | 258 ++++++++
> +--------------------
>  include/uapi/linux/bt-bmc.h                |  18 --
>  2 files changed, 74 insertions(+), 202 deletions(-)
>  delete mode 100644 include/uapi/linux/bt-bmc.h
>
> diff --git a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c b/drivers/
> char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
> index 70d434bc1cbf..7c8082c511ee 100644
> --- a/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
> +++ b/drivers/char/ipmi_bmc/ipmi_bmc_bt_aspeed.c
> @@ -7,25 +7,19 @@
>   * 2 of the License, or (at your option) any later version.
>   */
>
> -#include <linux/atomic.h>
> -#include <linux/bt-bmc.h>
>  #include <linux/errno.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> +#include <linux/ipmi_bmc.h>
>  #include <linux/mfd/syscon.h>
> -#include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> -#include <linux/poll.h>
>  #include <linux/regmap.h>
>  #include <linux/sched.h>
>  #include <linux/timer.h>
>
> -/*
> - * This is a BMC device used to communicate to the host
> - */
> -#define DEVICE_NAME   "ipmi-bt-host"
> +#define DEVICE_NAME "ipmi-bmc-bt-aspeed"
>
>  #define BT_IO_BASE   0xe4
>  #define BT_IRQ      10
> @@ -61,18 +55,17 @@
>  #define BT_BMC_BUFFER_SIZE 256
>
>  struct bt_bmc {
> +   struct ipmi_bmc_bus   bus;
>     struct device      dev;
> -   struct miscdevice   miscdev;
> +   struct ipmi_bmc_ctx   *bmc_ctx;
> +   struct bt_msg      request;
>     struct regmap      *map;
>     int         offset;
>     int         irq;
> -   wait_queue_head_t   queue;
>     struct timer_list   poll_timer;
> -   struct mutex      mutex;
> +   spinlock_t      lock;
>  };
>
> -static atomic_t open_count = ATOMIC_INIT(0);
> -
>  static const struct regmap_config bt_regmap_cfg = {
>     .reg_bits = 32,
>     .val_bits = 32,
> @@ -158,27 +151,28 @@ static ssize_t bt_writen(struct bt_bmc
> *bt_bmc, u8 *buf, size_t n)
>     return n;
>  }
>
> +/* TODO(benjaminfair): support ioctl BT_BMC_IOCTL_SMS_ATN */
>  static void set_sms_atn(struct bt_bmc *bt_bmc)
>  {
>     bt_outb(bt_bmc, BT_CTRL_SMS_ATN, BT_CTRL);
>  }
>
> -static struct bt_bmc *file_bt_bmc(struct file *file)
> +/* Called with bt_bmc->lock held */
> +static bool __is_request_avail(struct bt_bmc *bt_bmc)
>  {
> -   return container_of(file->private_data, struct bt_bmc, miscdev);
> +   return bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN;
>  }
>
> -static int bt_bmc_open(struct inode *inode, struct file *file)
> +static bool is_request_avail(struct bt_bmc *bt_bmc)
>  {
> -   struct bt_bmc *bt_bmc = file_bt_bmc(file);
> +   unsigned long flags;
> +   bool result;
>
> -   if (atomic_inc_return(&open_count) == 1) {
> -      clr_b_busy(bt_bmc);
> -      return 0;
> -   }
> +   spin_lock_irqsave(&bt_bmc->lock, flags);
> +   result = __is_request_avail(bt_bmc);
> +   spin_unlock_irqrestore(&bt_bmc->lock, flags);
>
> -   atomic_dec(&open_count);
> -   return -EBUSY;
> +   return result;
>  }
>
>  /*
> @@ -194,67 +188,43 @@ static int bt_bmc_open(struct inode *inode,
> struct file *file)
>   *    Length  NetFn/LUN  Seq     Cmd     Data
>   *
>   */
> -static ssize_t bt_bmc_read(struct file *file, char __user *buf,
> -            size_t count, loff_t *ppos)
> +static void get_request(struct bt_bmc *bt_bmc)
>  {
> -   struct bt_bmc *bt_bmc = file_bt_bmc(file);
> -   u8 len;
> -   int len_byte = 1;
> -   u8 kbuffer[BT_BMC_BUFFER_SIZE];
> -   ssize_t ret = 0;
> -   ssize_t nread;
> +   u8 *request_buf = (u8 *) &bt_bmc->request;
> +   unsigned long flags;
>
> -   if (!access_ok(VERIFY_WRITE, buf, count))
> -      return -EFAULT;
> +   spin_lock_irqsave(&bt_bmc->lock, flags);
>
> -   WARN_ON(*ppos);
> -
> -   if (wait_event_interruptible(bt_bmc->queue,
> -                 bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))
> -      return -ERESTARTSYS;
> -
> -   mutex_lock(&bt_bmc->mutex);
> -
> -   if (unlikely(!(bt_inb(bt_bmc, BT_CTRL) & BT_CTRL_H2B_ATN))) {
> -      ret = -EIO;
> -      goto out_unlock;
> +   if (!__is_request_avail(bt_bmc)) {
> +      spin_unlock_irqrestore(&bt_bmc->lock, flags);
> +      return;
>     }
>
>     set_b_busy(bt_bmc);
>     clr_h2b_atn(bt_bmc);
>     clr_rd_ptr(bt_bmc);
>
> -   /*
> -    * The BT frames start with the message length, which does not
> -    * include the length byte.
> -    */
> -   kbuffer[0] = bt_read(bt_bmc);
> -   len = kbuffer[0];
> -
> -   /* We pass the length back to userspace as well */
> -   if (len + 1 > count)
> -      len = count - 1;
> -
> -   while (len) {
> -      nread = min_t(ssize_t, len, sizeof(kbuffer) - len_byte);
> -
> -      bt_readn(bt_bmc, kbuffer + len_byte, nread);
> -
> -      if (copy_to_user(buf, kbuffer, nread + len_byte)) {
> -         ret = -EFAULT;
> -         break;
> -      }
> -      len -= nread;
> -      buf += nread + len_byte;
> -      ret += nread + len_byte;
> -      len_byte = 0;
> -   }
> +   bt_bmc->request.len = bt_read(bt_bmc);
> +   bt_readn(bt_bmc, request_buf + 1, bt_bmc->request.len);
>
>     clr_b_busy(bt_bmc);
> +   ipmi_bmc_handle_request(bt_bmc->bmc_ctx, &bt_bmc->request);
> +
> +   spin_unlock_irqrestore(&bt_bmc->lock, flags);
> +}
> +
> +static bool bt_bmc_is_response_open(struct ipmi_bmc_bus *bus)
> +{
> +   struct bt_bmc *bt_bmc = container_of(bus, struct bt_bmc, bus);
> +   bool response_in_progress;
> +   unsigned long flags;
> +
> +   spin_lock_irqsave(&bt_bmc->lock, flags);
> +   response_in_progress = bt_inb(bt_bmc, BT_CTRL) & (BT_CTRL_H_BUSY |
> +                       BT_CTRL_B2H_ATN);
> +   spin_unlock_irqrestore(&bt_bmc->lock, flags);
>
> -out_unlock:
> -   mutex_unlock(&bt_bmc->mutex);
> -   return ret;
> +   return !response_in_progress;
>  }
>
>  /*
> @@ -263,122 +233,38 @@ static ssize_t bt_bmc_read(struct file *file,
> char __user *buf,
>   *    Byte 1  Byte 2     Byte 3  Byte 4  Byte 5  Byte 6:N
>   *    Length  NetFn/LUN  Seq     Cmd     Code    Data
>   */
> -static ssize_t bt_bmc_write(struct file *file, const char __user *buf,
> -             size_t count, loff_t *ppos)
> +static int bt_bmc_send_response(struct ipmi_bmc_bus *bus,
> +            struct bt_msg *bt_response)
>  {
> -   struct bt_bmc *bt_bmc = file_bt_bmc(file);
> -   u8 kbuffer[BT_BMC_BUFFER_SIZE];
> -   ssize_t ret = 0;
> -   ssize_t nwritten;
> +   struct bt_bmc *bt_bmc = container_of(bus, struct bt_bmc, bus);
> +   unsigned long flags;
>
> -   /*
> -    * send a minimum response size
> -    */
> -   if (count < 5)
> -      return -EINVAL;
> -
> -   if (!access_ok(VERIFY_READ, buf, count))
> -      return -EFAULT;
> -
> -   WARN_ON(*ppos);
> -
> -   /*
> -    * There's no interrupt for clearing bmc busy so we have to
> -    * poll
> -    */
> -   if (wait_event_interruptible(bt_bmc->queue,
> -                 !(bt_inb(bt_bmc, BT_CTRL) &
> -                   (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))))
> -      return -ERESTARTSYS;
> -
> -   mutex_lock(&bt_bmc->mutex);
> -
> -   if (unlikely(bt_inb(bt_bmc, BT_CTRL) &
> -           (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN))) {
> -      ret = -EIO;
> -      goto out_unlock;
> +   spin_lock_irqsave(&bt_bmc->lock, flags);
> +   if (!bt_bmc_is_response_open(bus)) {
> +      spin_unlock_irqrestore(&bt_bmc->lock, flags);
> +      return -EAGAIN;
>     }
>
>     clr_wr_ptr(bt_bmc);
> -
> -   while (count) {
> -      nwritten = min_t(ssize_t, count, sizeof(kbuffer));
> -      if (copy_from_user(&kbuffer, buf, nwritten)) {
> -         ret = -EFAULT;
> -         break;
> -      }
> -
> -      bt_writen(bt_bmc, kbuffer, nwritten);
> -
> -      count -= nwritten;
> -      buf += nwritten;
> -      ret += nwritten;
> -   }
> -
> +   bt_writen(bt_bmc, (u8 *) bt_response, bt_msg_len(bt_response));
>     set_b2h_atn(bt_bmc);
>
> -out_unlock:
> -   mutex_unlock(&bt_bmc->mutex);
> -   return ret;
> -}
> -
> -static long bt_bmc_ioctl(struct file *file, unsigned int cmd,
> -          unsigned long param)
> -{
> -   struct bt_bmc *bt_bmc = file_bt_bmc(file);
> -
> -   switch (cmd) {
> -   case BT_BMC_IOCTL_SMS_ATN:
> -      set_sms_atn(bt_bmc);
> -      return 0;
> -   }
> -   return -EINVAL;
> -}
> -
> -static int bt_bmc_release(struct inode *inode, struct file *file)
> -{
> -   struct bt_bmc *bt_bmc = file_bt_bmc(file);
> -
> -   atomic_dec(&open_count);
> -   set_b_busy(bt_bmc);
> +   spin_unlock_irqrestore(&bt_bmc->lock, flags);
>     return 0;
>  }
>
> -static unsigned int bt_bmc_poll(struct file *file, poll_table *wait)
> -{
> -   struct bt_bmc *bt_bmc = file_bt_bmc(file);
> -   unsigned int mask = 0;
> -   u8 ctrl;
> -
> -   poll_wait(file, &bt_bmc->queue, wait);
> -
> -   ctrl = bt_inb(bt_bmc, BT_CTRL);
> -
> -   if (ctrl & BT_CTRL_H2B_ATN)
> -      mask |= POLLIN;
> -
> -   if (!(ctrl & (BT_CTRL_H_BUSY | BT_CTRL_B2H_ATN)))
> -      mask |= POLLOUT;
> -
> -   return mask;
> -}
> -
> -static const struct file_operations bt_bmc_fops = {
> -   .owner      = THIS_MODULE,
> -   .open      = bt_bmc_open,
> -   .read      = bt_bmc_read,
> -   .write      = bt_bmc_write,
> -   .release   = bt_bmc_release,
> -   .poll      = bt_bmc_poll,
> -   .unlocked_ioctl   = bt_bmc_ioctl,
> -};
> -
>  static void poll_timer(unsigned long data)
>  {
>     struct bt_bmc *bt_bmc = (void *)data;
>
>     bt_bmc->poll_timer.expires += msecs_to_jiffies(500);
> -   wake_up(&bt_bmc->queue);
> +
> +   if (bt_bmc_is_response_open(&bt_bmc->bus))
> +      ipmi_bmc_signal_response_open(bt_bmc->bmc_ctx);
> +
> +   if (is_request_avail(bt_bmc))
> +      get_request(bt_bmc);
> +
>     add_timer(&bt_bmc->poll_timer);
>  }
>
> @@ -399,7 +285,12 @@ static irqreturn_t bt_bmc_irq(int irq, void *arg)
>     /* ack pending IRQs */
>     regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg);
>
> -   wake_up(&bt_bmc->queue);
> +   if (bt_bmc_is_response_open(&bt_bmc->bus))
> +      ipmi_bmc_signal_response_open(bt_bmc->bmc_ctx);
> +
> +   if (is_request_avail(bt_bmc))
> +      get_request(bt_bmc);
> +
>     return IRQ_HANDLED;
>  }
>
> @@ -474,16 +365,14 @@ static int bt_bmc_probe(struct platform_device
*pdev)
>           return rc;
>     }
>
> -   mutex_init(&bt_bmc->mutex);
> -   init_waitqueue_head(&bt_bmc->queue);
> +   spin_lock_init(&bt_bmc->lock);
> +   bt_bmc->bus.send_response = bt_bmc_send_response;
> +   bt_bmc->bus.is_response_open = bt_bmc_is_response_open;
> +   bt_bmc->bmc_ctx = ipmi_bmc_get_global_ctx();
>
> -   bt_bmc->miscdev.minor   = MISC_DYNAMIC_MINOR,
> -      bt_bmc->miscdev.name   = DEVICE_NAME,
> -      bt_bmc->miscdev.fops   = &bt_bmc_fops,
> -      bt_bmc->miscdev.parent = dev;
> -   rc = misc_register(&bt_bmc->miscdev);
> +   rc = ipmi_bmc_register_bus(bt_bmc->bmc_ctx, &bt_bmc->bus);
>     if (rc) {
> -      dev_err(dev, "Unable to register misc device\n");
> +      dev_err(dev, "Unable to register IPMI BMC bus\n");
>        return rc;
>     }
>
> @@ -514,11 +403,12 @@ static int bt_bmc_probe(struct platform_device
*pdev)
>  static int bt_bmc_remove(struct platform_device *pdev)
>  {
>     struct bt_bmc *bt_bmc = dev_get_drvdata(&pdev->dev);
> +   int rc;
>
> -   misc_deregister(&bt_bmc->miscdev);
> +   rc = ipmi_bmc_unregister_bus(bt_bmc->bmc_ctx, &bt_bmc->bus);
>     if (!bt_bmc->irq)
>        del_timer_sync(&bt_bmc->poll_timer);
> -   return 0;
> +   return rc;
>  }
>
>  static const struct of_device_id bt_bmc_match[] = {
> diff --git a/include/uapi/linux/bt-bmc.h b/include/uapi/linux/bt-bmc.h
> deleted file mode 100644
> index d9ec766a63d0..000000000000
> --- a/include/uapi/linux/bt-bmc.h
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -/*
> - * Copyright (c) 2015-2016, IBM Corporation.
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License
> - * as published by the Free Software Foundation; either version
> - * 2 of the License, or (at your option) any later version.
> - */
> -
> -#ifndef _UAPI_LINUX_BT_BMC_H
> -#define _UAPI_LINUX_BT_BMC_H
> -
> -#include <linux/ioctl.h>
> -
> -#define __BT_BMC_IOCTL_MAGIC   0xb1
> -#define BT_BMC_IOCTL_SMS_ATN   _IO(__BT_BMC_IOCTL_MAGIC, 0x00)
> -
> -#endif /* _UAPI_LINUX_BT_BMC_H */
> --
> 2.14.0.rc1.383.gd1ce394fe2-goog
>

[-- Attachment #2: Type: text/html, Size: 19864 bytes --]

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Jeremy Kerr]

Hi Brendan,

> The driver was handling interaction with userspace on its own. This
> patch changes it to use the functionality of the ipmi_bmc framework
> instead.
> 
> Note that this removes the ability for the BMC to set SMS_ATN by making
> an ioctl. If this functionality is required, it can be added back in
> with a later patch.

As Chris has mentioned, we do use this actively at the moment, so I'd
prefer if we could not drop the support for SMS_ATN. However, using a
different interface should be fine, if that helps.

Cheers,


Jeremy

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Brendan Higgins]

On Wed, Aug 9, 2017 at 7:31 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Brendan,
>
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
>
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

Whoops, I did not realize that anyone was using it. Yeah, adding it back in
should not be too hard.

>
> Cheers,
>
>
> Jeremy

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Cédric Le Goater]

On 08/10/2017 04:31 AM, Jeremy Kerr wrote:
> Hi Brendan,
> 
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
> 
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

The ioctl is part of the kernel user API now. We should keep it.

Thanks,

C.  

Re: [RFC v1 4/4] ipmi_bmc: bt-aspeed: port driver to IPMI BMC framework

[Cédric Le Goater]

On 08/10/2017 04:31 AM, Jeremy Kerr wrote:
> Hi Brendan,
> 
>> The driver was handling interaction with userspace on its own. This
>> patch changes it to use the functionality of the ipmi_bmc framework
>> instead.
>>
>> Note that this removes the ability for the BMC to set SMS_ATN by making
>> an ioctl. If this functionality is required, it can be added back in
>> with a later patch.
> 
> As Chris has mentioned, we do use this actively at the moment, so I'd
> prefer if we could not drop the support for SMS_ATN. However, using a
> different interface should be fine, if that helps.

The ioctl is part of the kernel user API now. We should keep it.

Thanks,

C.  

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

[Corey Minyard]

On 08/07/2017 10:52 PM, Brendan Higgins wrote:
> From: Benjamin Fair <benjaminfair@google.com>
>
> This patch introduces a framework for writing IPMI drivers which run on
> a Board Management Controller. It is similar in function to OpenIPMI.
> The framework handles registering devices and routing messages.

Ok, I think I understand this.  I have a few nitpicky comments inline.  
The RCU usage
looks correct, and the basic design seems sound.

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



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

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

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

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

I guess I'm trying to figure out how you expect all this work out in the 
end.  What
you have now is a message mux/demux that can only have on thing underneath
it and one thing above it, which obviously isn't very useful.  Are you 
thinking you
can have other in-kernel things that can handle specific messages? I'm 
having
a hard time imagining that's the case.  Or are you thinking that you 
will create
a userland interface to create a bus and then when a LAN connection comes
in you create one of these BMC contexts and route the LAN traffic 
through this
code?  That's kind of clever, but I'm wondering if there would be better 
ways
to do this than this design.

-corey

> Signed-off-by: Benjamin Fair <benjaminfair@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>   drivers/char/ipmi_bmc/Makefile   |   1 +
>   drivers/char/ipmi_bmc/ipmi_bmc.c | 294 +++++++++++++++++++++++++++++++++++++++
>   include/linux/ipmi_bmc.h         | 184 ++++++++++++++++++++++++
>   3 files changed, 479 insertions(+)
>   create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc.c
>
> diff --git a/drivers/char/ipmi_bmc/Makefile b/drivers/char/ipmi_bmc/Makefile
> index 8bff32b55c24..9c7cd48d899f 100644
> --- a/drivers/char/ipmi_bmc/Makefile
> +++ b/drivers/char/ipmi_bmc/Makefile
> @@ -2,5 +2,6 @@
>   # Makefile for the ipmi bmc drivers.
>   #
>   
> +obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
>   obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
>   obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
> diff --git a/drivers/char/ipmi_bmc/ipmi_bmc.c b/drivers/char/ipmi_bmc/ipmi_bmc.c
> new file mode 100644
> index 000000000000..c1324ac9a83c
> --- /dev/null
> +++ b/drivers/char/ipmi_bmc/ipmi_bmc.c

This is not really a BMC, it's a BMC message router, or something like that.

> @@ -0,0 +1,294 @@
> +/*
> + * Copyright 2017 Google Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/ipmi_bmc.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/rculist.h>
> +#include <linux/rcupdate.h>
> +
> +#define PFX "IPMI BMC core: "
> +
> +struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx()
> +{
> +	static struct ipmi_bmc_ctx global_ctx;
> +
> +	return &global_ctx;
> +}
> +
> +int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
> +			   struct bt_msg *bt_response)
> +{
> +	struct ipmi_bmc_bus *bus;
> +	int ret = -ENODEV;
> +
> +	rcu_read_lock();
> +	bus = rcu_dereference(ctx->bus);
> +
> +	if (bus)
> +		ret = bus->send_response(bus, bt_response);
> +
> +	rcu_read_unlock();
> +	return ret;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_send_response);
> +
> +bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx)
> +{
> +	struct ipmi_bmc_bus *bus;
> +	bool ret = false;
> +
> +	rcu_read_lock();
> +	bus = rcu_dereference(ctx->bus);
> +
> +	if (bus)
> +		ret = bus->is_response_open(bus);
> +
> +	rcu_read_unlock();
> +	return ret;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_is_response_open);
> +
> +int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
> +			     struct ipmi_bmc_device *device_in)
> +{
> +	struct ipmi_bmc_device *device;
> +
> +	mutex_lock(&ctx->drivers_mutex);
> +	/* Make sure it hasn't already been registered. */
> +	list_for_each_entry(device, &ctx->devices, link) {
> +		if (device == device_in) {
> +			mutex_unlock(&ctx->drivers_mutex);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	list_add_rcu(&device_in->link, &ctx->devices);
> +	mutex_unlock(&ctx->drivers_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_register_device);
> +
> +int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
> +			       struct ipmi_bmc_device *device_in)
> +{
> +	struct ipmi_bmc_device *device;
> +	bool found = false;
> +
> +	mutex_lock(&ctx->drivers_mutex);
> +	/* Make sure it is currently registered. */
> +	list_for_each_entry(device, &ctx->devices, link) {
> +		if (device == device_in) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (!found) {
> +		mutex_unlock(&ctx->drivers_mutex);
> +		return -ENXIO;
> +	}
> +
> +	list_del_rcu(&device_in->link);
> +	mutex_unlock(&ctx->drivers_mutex);
> +	synchronize_rcu();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_unregister_device);
> +
> +int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
> +				     struct ipmi_bmc_device *device)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctx->drivers_mutex);
> +	if (!ctx->default_device) {
> +		ctx->default_device = device;
> +		ret = 0;
> +	} else {
> +		ret = -EBUSY;
> +	}
> +	mutex_unlock(&ctx->drivers_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_register_default_device);
> +
> +int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
> +				       struct ipmi_bmc_device *device)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctx->drivers_mutex);
> +	if (ctx->default_device == device) {
> +		ctx->default_device = NULL;
> +		ret = 0;
> +	} else {
> +		ret = -ENXIO;
> +	}
> +	mutex_unlock(&ctx->drivers_mutex);
> +	synchronize_rcu();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_unregister_default_device);
> +
> +static u8 errno_to_ccode(int errno)
> +{
> +	switch (errno) {
> +	case EBUSY:
> +		return 0xC0; /* Node Busy */
> +	case EINVAL:
> +		return 0xC1; /* Invalid Command */
> +	case ETIMEDOUT:
> +		return 0xC3; /* Timeout while processing command */
> +	case ENOMEM:
> +		return 0xC4; /* Out of space */
> +	default:
> +		return 0xFF; /* Unspecified error */

Instead of the hard-coded constants, you can use 
include/uapi/linux/ipmi_msgdefs.h
and add things there as needed.  You probably knew that, but these 
numbers should
all be the same.  Same for other constant that might apply.

> +	}
> +}
> +
> +static void ipmi_bmc_send_error_response(struct ipmi_bmc_ctx *ctx,
> +					 struct bt_msg *bt_request,
> +					 u8 ccode)
> +{
> +	struct bt_msg error_response;
> +	int ret;
> +
> +	/* Payload contains 1 byte for completion code */
> +	error_response.len = bt_msg_payload_to_len(1);
> +	error_response.netfn_lun = bt_request->netfn_lun |
> +				   IPMI_NETFN_LUN_RESPONSE_MASK;
> +	error_response.seq = bt_request->seq;
> +	error_response.cmd = bt_request->cmd;
> +	error_response.payload[0] = ccode;
> +
> +	/*
> +	 * TODO(benjaminfair): Retry sending the response if it fails. The error
> +	 * response might fail to send if another response is in progress. In
> +	 * that case, the request will timeout rather than getting a more
> +	 * specific completion code. This should buffer up responses and send
> +	 * them when it can. Device drivers will generally handle error
> +	 * reporting themselves; this code is only a fallback when that's not
> +	 * available or when the drivers themselves fail.
> +	 */
> +	ret = ipmi_bmc_send_response(ctx, &error_response);
> +	if (ret)
> +		pr_warn(PFX "Failed to reply with completion code %u: ipmi_bmc_send_response returned %d\n",
> +			(u32) ccode, ret);
> +}
> +
> +void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
> +			     struct bt_msg *bt_request)
> +{
> +	struct ipmi_bmc_device *default_device;
> +	struct ipmi_bmc_device *device;
> +	int ret = -EINVAL;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(device, &ctx->devices, link) {
> +		if (device->match_request(device, bt_request)) {
> +			ret = device->handle_request(device, bt_request);
> +			goto out;
> +		}
> +	}
> +
> +	/* No specific handler found. Use default handler instead */
> +	default_device = rcu_dereference(ctx->default_device);
> +	if (default_device)
> +		ret = default_device->handle_request(default_device,
> +						     bt_request);
> +
> +out:
> +	rcu_read_unlock();
> +	if (ret)
> +		ipmi_bmc_send_error_response(ctx, bt_request,
> +					     errno_to_ccode(-ret));
> +}
> +EXPORT_SYMBOL(ipmi_bmc_handle_request);
> +
> +void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx)
> +{
> +	struct ipmi_bmc_device *default_device;
> +	struct ipmi_bmc_device *device;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(device, &ctx->devices, link) {
> +		device->signal_response_open(device);
> +	}
> +
> +	default_device = rcu_dereference(ctx->default_device);
> +	if (default_device)
> +		default_device->signal_response_open(default_device);
> +
> +	rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(ipmi_bmc_signal_response_open);
> +
> +int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
> +			  struct ipmi_bmc_bus *bus_in)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctx->drivers_mutex);
> +	if (!ctx->bus) {
> +		ctx->bus = bus_in;
> +		ret = 0;
> +	} else {
> +		ret = -EBUSY;
> +	}
> +	mutex_unlock(&ctx->drivers_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_register_bus);
> +
> +int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
> +			    struct ipmi_bmc_bus *bus_in)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctx->drivers_mutex);
> +	/* Tried to unregister when another bus is registered */
> +	if (ctx->bus == bus_in) {
> +		ctx->bus = NULL;
> +		ret = 0;
> +	} else {
> +		ret = -ENXIO;
> +	}
> +	mutex_unlock(&ctx->drivers_mutex);
> +	synchronize_rcu();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(ipmi_bmc_unregister_bus);
> +
> +static int __init ipmi_bmc_init(void)
> +{
> +	struct ipmi_bmc_ctx *ctx = ipmi_bmc_get_global_ctx();
> +
> +	mutex_init(&ctx->drivers_mutex);
> +	INIT_LIST_HEAD(&ctx->devices);
> +	return 0;
> +}
> +module_init(ipmi_bmc_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Benjamin Fair <benjaminfair@google.com>");
> +MODULE_DESCRIPTION("Core IPMI driver for the BMC side");
> diff --git a/include/linux/ipmi_bmc.h b/include/linux/ipmi_bmc.h
> index d0885c0bf190..2b494a79ec14 100644
> --- a/include/linux/ipmi_bmc.h
> +++ b/include/linux/ipmi_bmc.h
> @@ -20,6 +20,13 @@
>   #include <linux/types.h>
>   
>   #define BT_MSG_PAYLOAD_LEN_MAX 252
> +#define BT_MSG_SEQ_MAX 255
> +
> +/*
> + * The bit in this mask is set in the netfn_lun field of an IPMI message to
> + * indicate that it is a response.
> + */
> +#define IPMI_NETFN_LUN_RESPONSE_MASK (1 << 2)
>   
>   /**
>    * struct bt_msg - Block Transfer IPMI message.
> @@ -42,6 +49,84 @@ struct bt_msg {
>   	u8 payload[BT_MSG_PAYLOAD_LEN_MAX];
>   } __packed;
>   
> +/**
> + * struct ipmi_bmc_device - Device driver that wants to receive ipmi requests.
> + * @link: Used to make a linked list of devices.
> + * @match_request: Used to determine whether a request can be handled by this
> + *                 device. Note that the matchers are checked in an arbitrary
> + *                 order.
> + * @handle_request: Called when a request is received for this device.
> + * @signal_response_open: Called when a response is done being sent and another
> + *                        device can send a message. Make sure to check that the
> + *                        bus isn't busy even after this is called, because all
> + *                        devices receive this call and another one may have
> + *                        already submitted a new response.
> + *
> + * This collection of callback functions represents an upper-level handler of
> + * IPMI requests.
> + *
> + * Note that the callbacks may be called from an interrupt context.
> + */
> +struct ipmi_bmc_device {
> +	struct list_head link;
> +
> +	bool (*match_request)(struct ipmi_bmc_device *device,
> +			      struct bt_msg *bt_request);
> +	int (*handle_request)(struct ipmi_bmc_device *device,
> +			      struct bt_msg *bt_request);
> +	void (*signal_response_open)(struct ipmi_bmc_device *device);
> +};
> +
> +/**
> + * struct ipmi_bmc_bus - Bus driver that exchanges messages with the host.
> + * @send_response: Submits the given response to be sent to the host. May return
> + *                 -EBUSY if a response is already in progress, in which case
> + *                 the caller should wait for the response_open() callback to be
> + *                 called.
> + * @is_response_open: Determines whether a response can currently be sent. Note
> + *                    that &ipmi_bmc_bus->send_response() may still fail with
> + *                    -EBUSY even after this returns true.
> + *
> + * This collection of callback functions represents a lower-level driver which
> + * acts as a connection to the host.
> + */
> +struct ipmi_bmc_bus {
> +	int (*send_response)(struct ipmi_bmc_bus *bus,
> +			     struct bt_msg *bt_response);
> +	bool (*is_response_open)(struct ipmi_bmc_bus *bus);
> +};
> +
> +/**
> + * struct ipmi_bmc_ctx - Context object used to interact with the IPMI BMC
> + *                       core driver.
> + * @bus: Pointer to the bus which is currently registered, or NULL for none.
> + * @default_device: Pointer to a device which will receive messages that match
> + *                  no other devices, or NULL if none is registered.
> + * @devices: List of devices which are currently registered, besides the default
> + *           device.
> + * @drivers_mutex: Mutex which protects against concurrent editing of the
> + *                 bus driver, default device driver, and devices list.
> + *
> + * This struct should only be modified by the IPMI BMC core code and not by bus
> + * or device drivers.
> + */
> +struct ipmi_bmc_ctx {
> +	struct ipmi_bmc_bus __rcu	*bus;
> +	struct ipmi_bmc_device __rcu	*default_device;
> +	struct list_head		devices;
> +	struct mutex			drivers_mutex;

Since it does all it does, "drivers_mutex" IMHO is too specific a name.

> +};
> +
> +/**
> + * ipmi_bmc_get_global_ctx() - Get a pointer to the global ctx.
> + *
> + * This function gets a reference to the global ctx object which is used by
> + * bus and device drivers to interact with the IPMI BMC core driver.
> + *
> + * Return: Pointer to the global ctx object.
> + */
> +struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx(void);
> +
>   /**
>    * bt_msg_len() - Determine the total length of a Block Transfer message.
>    * @bt_msg: Pointer to the message.
> @@ -73,4 +158,103 @@ static inline u8 bt_msg_payload_to_len(u8 payload_len)
>   	return payload_len + 3;
>   }
>   
> +/**
> + * ipmi_bmc_send_response() - Send an IPMI response on the current bus.
> + * @bt_response: The response message to send.
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
> +			   struct bt_msg *bt_response);
> +/**
> + * ipmi_bmc_is_response_open() - Check whether we can currently send a new
> + *                               response.
> + *
> + * Note that even if this function returns true, ipmi_bmc_send_response() may
> + * still fail with -EBUSY if a response is submitted in the time between the two
> + * calls.
> + *
> + * Return: true if we can send a new response, false if one is already in
> + *         progress.
> + */
> +bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx);
> +/**
> + * ipmi_bmc_register_device() - Register a new device driver.
> + * @device: Pointer to the struct which represents this device,
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
> +			     struct ipmi_bmc_device *device);
> +/**
> + * ipmi_bmc_unregister_device() - Unregister an existing device driver.
> + * @device: Pointer to the struct which represents the existing device.
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
> +			       struct ipmi_bmc_device *device);
> +/**
> + * ipmi_bmc_register_default_device() - Register a new default device driver.
> + * @device: Pointer to the struct which represents this device,
> + *
> + * Make this device the default device. If none of the other devices match on a
> + * particular message, this device will receive it instead.  Note that only one
> + * default device may be registered at a time.
> + *
> + * This functionalisty is currently used to allow messages which aren't directly
> + * handled by the kernel to be sent to userspace and handled there.
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
> +				     struct ipmi_bmc_device *device);
> +/**
> + * ipmi_bmc_unregister_default_device() - Unregister the existing default device
> + *                                        driver.
> + * @device: Pointer to the struct which represents the existing device.
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
> +				       struct ipmi_bmc_device *device);
> +/**
> + * ipmi_bmc_handle_request() - Handle a new request that was received.
> + * @bt_request: The request that was received.
> + *
> + * This is called by the bus driver when it receives a new request message.
> + *
> + * Note that it may be called from an interrupt context.
> + */
> +void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
> +			    struct bt_msg *bt_request);
> +/**
> + * ipmi_bmc_signal_response_open() - Notify the upper layer that a response can
> + *                                   be sent.
> + *
> + * This is called by the bus driver after it finishes sending a response and is
> + * ready to begin sending another one.
> + */
> +void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx);
> +/**
> + * ipmi_bmc_register_bus() - Register a new bus driver.
> + * @bus: Pointer to the struct which represents this bus.
> + *
> + * Register a bus driver to handle communication with the host.
> + *
> + * Only one bus driver can be registered at any time.
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
> +			  struct ipmi_bmc_bus *bus);
> +/**
> + * ipmi_bmc_unregister_bus() - Unregister an existing bus driver.
> + * @bus: Pointer to the struct which represents the existing bus.
> + *
> + * Return: 0 on success, negative errno otherwise.
> + */
> +int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
> +			    struct ipmi_bmc_bus *bus);
> +
>   #endif /* __LINUX_IPMI_BMC_H */

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

[Brendan Higgins]

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

Sweet

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Yes, I as I mentioned above, having handlers in the kernel is a prime motivator
for this. I have been working on an interface for doing flash updates over IPMI.
IPMI is not really a great way to do this in terms of performance or
the interface
it provides; however, IPMI is great in the sense that all platforms
have it, so I
want to have alternative backends for this flash interface and provide an IPMI
OEM command implementation as a lowest common denominator option. I
have some other examples, but i think this is one of the most interesting.

> create
> a userland interface to create a bus and then when a LAN connection comes
> in you create one of these BMC contexts and route the LAN traffic through
> this
> code?  That's kind of clever, but I'm wondering if there would be better
> ways
> to do this than this design.

Well, I think that is up for discussion; my main goal here is starting a
conversation.

As far as this being a complete design; I do not consider what I have
presented as being complete. I mentioned some things above that I would like
to add and some people have already chimed in asking for some changes.
I just wanted to get some feedback before I went *too* far.

>
> -corey
>
>> Signed-off-by: Benjamin Fair <benjaminfair@google.com>
>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>>   drivers/char/ipmi_bmc/Makefile   |   1 +
>>   drivers/char/ipmi_bmc/ipmi_bmc.c | 294
>> +++++++++++++++++++++++++++++++++++++++
>>   include/linux/ipmi_bmc.h         | 184 ++++++++++++++++++++++++
>>   3 files changed, 479 insertions(+)
>>   create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc.c
>>
>> diff --git a/drivers/char/ipmi_bmc/Makefile
>> b/drivers/char/ipmi_bmc/Makefile
>> index 8bff32b55c24..9c7cd48d899f 100644
>> --- a/drivers/char/ipmi_bmc/Makefile
>> +++ b/drivers/char/ipmi_bmc/Makefile
>> @@ -2,5 +2,6 @@
>>   # Makefile for the ipmi bmc drivers.
>>   #
>>   +obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
>>   obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
>>   obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
>> diff --git a/drivers/char/ipmi_bmc/ipmi_bmc.c
>> b/drivers/char/ipmi_bmc/ipmi_bmc.c
>> new file mode 100644
>> index 000000000000..c1324ac9a83c
>> --- /dev/null
>> +++ b/drivers/char/ipmi_bmc/ipmi_bmc.c
>
>
> This is not really a BMC, it's a BMC message router, or something like that.

How about ipmi_bmc_core.c or ipmi_slave_core.c ?

>
>
>> @@ -0,0 +1,294 @@
>> +/*
>> + * Copyright 2017 Google Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/export.h>
>> +#include <linux/init.h>
>> +#include <linux/ipmi_bmc.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/rculist.h>
>> +#include <linux/rcupdate.h>
>> +
>> +#define PFX "IPMI BMC core: "
>> +
>> +struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx()
>> +{
>> +       static struct ipmi_bmc_ctx global_ctx;
>> +
>> +       return &global_ctx;
>> +}
>> +
>> +int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
>> +                          struct bt_msg *bt_response)
>> +{
>> +       struct ipmi_bmc_bus *bus;
>> +       int ret = -ENODEV;
>> +
>> +       rcu_read_lock();
>> +       bus = rcu_dereference(ctx->bus);
>> +
>> +       if (bus)
>> +               ret = bus->send_response(bus, bt_response);
>> +
>> +       rcu_read_unlock();
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_send_response);
>> +
>> +bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx)
>> +{
>> +       struct ipmi_bmc_bus *bus;
>> +       bool ret = false;
>> +
>> +       rcu_read_lock();
>> +       bus = rcu_dereference(ctx->bus);
>> +
>> +       if (bus)
>> +               ret = bus->is_response_open(bus);
>> +
>> +       rcu_read_unlock();
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_is_response_open);
>> +
>> +int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
>> +                            struct ipmi_bmc_device *device_in)
>> +{
>> +       struct ipmi_bmc_device *device;
>> +
>> +       mutex_lock(&ctx->drivers_mutex);
>> +       /* Make sure it hasn't already been registered. */
>> +       list_for_each_entry(device, &ctx->devices, link) {
>> +               if (device == device_in) {
>> +                       mutex_unlock(&ctx->drivers_mutex);
>> +                       return -EINVAL;
>> +               }
>> +       }
>> +
>> +       list_add_rcu(&device_in->link, &ctx->devices);
>> +       mutex_unlock(&ctx->drivers_mutex);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_register_device);
>> +
>> +int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
>> +                              struct ipmi_bmc_device *device_in)
>> +{
>> +       struct ipmi_bmc_device *device;
>> +       bool found = false;
>> +
>> +       mutex_lock(&ctx->drivers_mutex);
>> +       /* Make sure it is currently registered. */
>> +       list_for_each_entry(device, &ctx->devices, link) {
>> +               if (device == device_in) {
>> +                       found = true;
>> +                       break;
>> +               }
>> +       }
>> +       if (!found) {
>> +               mutex_unlock(&ctx->drivers_mutex);
>> +               return -ENXIO;
>> +       }
>> +
>> +       list_del_rcu(&device_in->link);
>> +       mutex_unlock(&ctx->drivers_mutex);
>> +       synchronize_rcu();
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_unregister_device);
>> +
>> +int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
>> +                                    struct ipmi_bmc_device *device)
>> +{
>> +       int ret;
>> +
>> +       mutex_lock(&ctx->drivers_mutex);
>> +       if (!ctx->default_device) {
>> +               ctx->default_device = device;
>> +               ret = 0;
>> +       } else {
>> +               ret = -EBUSY;
>> +       }
>> +       mutex_unlock(&ctx->drivers_mutex);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_register_default_device);
>> +
>> +int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
>> +                                      struct ipmi_bmc_device *device)
>> +{
>> +       int ret;
>> +
>> +       mutex_lock(&ctx->drivers_mutex);
>> +       if (ctx->default_device == device) {
>> +               ctx->default_device = NULL;
>> +               ret = 0;
>> +       } else {
>> +               ret = -ENXIO;
>> +       }
>> +       mutex_unlock(&ctx->drivers_mutex);
>> +       synchronize_rcu();
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_unregister_default_device);
>> +
>> +static u8 errno_to_ccode(int errno)
>> +{
>> +       switch (errno) {
>> +       case EBUSY:
>> +               return 0xC0; /* Node Busy */
>> +       case EINVAL:
>> +               return 0xC1; /* Invalid Command */
>> +       case ETIMEDOUT:
>> +               return 0xC3; /* Timeout while processing command */
>> +       case ENOMEM:
>> +               return 0xC4; /* Out of space */
>> +       default:
>> +               return 0xFF; /* Unspecified error */
>
>
> Instead of the hard-coded constants, you can use
> include/uapi/linux/ipmi_msgdefs.h
> and add things there as needed.  You probably knew that, but these numbers
> should
> all be the same.  Same for other constant that might apply.

Noted.

>
>
>> +       }
>> +}
>> +
>> +static void ipmi_bmc_send_error_response(struct ipmi_bmc_ctx *ctx,
>> +                                        struct bt_msg *bt_request,
>> +                                        u8 ccode)
>> +{
>> +       struct bt_msg error_response;
>> +       int ret;
>> +
>> +       /* Payload contains 1 byte for completion code */
>> +       error_response.len = bt_msg_payload_to_len(1);
>> +       error_response.netfn_lun = bt_request->netfn_lun |
>> +                                  IPMI_NETFN_LUN_RESPONSE_MASK;
>> +       error_response.seq = bt_request->seq;
>> +       error_response.cmd = bt_request->cmd;
>> +       error_response.payload[0] = ccode;
>> +
>> +       /*
>> +        * TODO(benjaminfair): Retry sending the response if it fails. The
>> error
>> +        * response might fail to send if another response is in progress.
>> In
>> +        * that case, the request will timeout rather than getting a more
>> +        * specific completion code. This should buffer up responses and
>> send
>> +        * them when it can. Device drivers will generally handle error
>> +        * reporting themselves; this code is only a fallback when that's
>> not
>> +        * available or when the drivers themselves fail.
>> +        */
>> +       ret = ipmi_bmc_send_response(ctx, &error_response);
>> +       if (ret)
>> +               pr_warn(PFX "Failed to reply with completion code %u:
>> ipmi_bmc_send_response returned %d\n",
>> +                       (u32) ccode, ret);
>> +}
>> +
>> +void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
>> +                            struct bt_msg *bt_request)
>> +{
>> +       struct ipmi_bmc_device *default_device;
>> +       struct ipmi_bmc_device *device;
>> +       int ret = -EINVAL;
>> +
>> +       rcu_read_lock();
>> +       list_for_each_entry_rcu(device, &ctx->devices, link) {
>> +               if (device->match_request(device, bt_request)) {
>> +                       ret = device->handle_request(device, bt_request);
>> +                       goto out;
>> +               }
>> +       }
>> +
>> +       /* No specific handler found. Use default handler instead */
>> +       default_device = rcu_dereference(ctx->default_device);
>> +       if (default_device)
>> +               ret = default_device->handle_request(default_device,
>> +                                                    bt_request);
>> +
>> +out:
>> +       rcu_read_unlock();
>> +       if (ret)
>> +               ipmi_bmc_send_error_response(ctx, bt_request,
>> +                                            errno_to_ccode(-ret));
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_handle_request);
>> +
>> +void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx)
>> +{
>> +       struct ipmi_bmc_device *default_device;
>> +       struct ipmi_bmc_device *device;
>> +
>> +       rcu_read_lock();
>> +       list_for_each_entry_rcu(device, &ctx->devices, link) {
>> +               device->signal_response_open(device);
>> +       }
>> +
>> +       default_device = rcu_dereference(ctx->default_device);
>> +       if (default_device)
>> +               default_device->signal_response_open(default_device);
>> +
>> +       rcu_read_unlock();
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_signal_response_open);
>> +
>> +int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
>> +                         struct ipmi_bmc_bus *bus_in)
>> +{
>> +       int ret;
>> +
>> +       mutex_lock(&ctx->drivers_mutex);
>> +       if (!ctx->bus) {
>> +               ctx->bus = bus_in;
>> +               ret = 0;
>> +       } else {
>> +               ret = -EBUSY;
>> +       }
>> +       mutex_unlock(&ctx->drivers_mutex);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_register_bus);
>> +
>> +int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
>> +                           struct ipmi_bmc_bus *bus_in)
>> +{
>> +       int ret;
>> +
>> +       mutex_lock(&ctx->drivers_mutex);
>> +       /* Tried to unregister when another bus is registered */
>> +       if (ctx->bus == bus_in) {
>> +               ctx->bus = NULL;
>> +               ret = 0;
>> +       } else {
>> +               ret = -ENXIO;
>> +       }
>> +       mutex_unlock(&ctx->drivers_mutex);
>> +       synchronize_rcu();
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL(ipmi_bmc_unregister_bus);
>> +
>> +static int __init ipmi_bmc_init(void)
>> +{
>> +       struct ipmi_bmc_ctx *ctx = ipmi_bmc_get_global_ctx();
>> +
>> +       mutex_init(&ctx->drivers_mutex);
>> +       INIT_LIST_HEAD(&ctx->devices);
>> +       return 0;
>> +}
>> +module_init(ipmi_bmc_init);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Benjamin Fair <benjaminfair@google.com>");
>> +MODULE_DESCRIPTION("Core IPMI driver for the BMC side");
>> diff --git a/include/linux/ipmi_bmc.h b/include/linux/ipmi_bmc.h
>> index d0885c0bf190..2b494a79ec14 100644
>> --- a/include/linux/ipmi_bmc.h
>> +++ b/include/linux/ipmi_bmc.h
>> @@ -20,6 +20,13 @@
>>   #include <linux/types.h>
>>     #define BT_MSG_PAYLOAD_LEN_MAX 252
>> +#define BT_MSG_SEQ_MAX 255
>> +
>> +/*
>> + * The bit in this mask is set in the netfn_lun field of an IPMI message
>> to
>> + * indicate that it is a response.
>> + */
>> +#define IPMI_NETFN_LUN_RESPONSE_MASK (1 << 2)
>>     /**
>>    * struct bt_msg - Block Transfer IPMI message.
>> @@ -42,6 +49,84 @@ struct bt_msg {
>>         u8 payload[BT_MSG_PAYLOAD_LEN_MAX];
>>   } __packed;
>>   +/**
>> + * struct ipmi_bmc_device - Device driver that wants to receive ipmi
>> requests.
>> + * @link: Used to make a linked list of devices.
>> + * @match_request: Used to determine whether a request can be handled by
>> this
>> + *                 device. Note that the matchers are checked in an
>> arbitrary
>> + *                 order.
>> + * @handle_request: Called when a request is received for this device.
>> + * @signal_response_open: Called when a response is done being sent and
>> another
>> + *                        device can send a message. Make sure to check
>> that the
>> + *                        bus isn't busy even after this is called,
>> because all
>> + *                        devices receive this call and another one may
>> have
>> + *                        already submitted a new response.
>> + *
>> + * This collection of callback functions represents an upper-level
>> handler of
>> + * IPMI requests.
>> + *
>> + * Note that the callbacks may be called from an interrupt context.
>> + */
>> +struct ipmi_bmc_device {
>> +       struct list_head link;
>> +
>> +       bool (*match_request)(struct ipmi_bmc_device *device,
>> +                             struct bt_msg *bt_request);
>> +       int (*handle_request)(struct ipmi_bmc_device *device,
>> +                             struct bt_msg *bt_request);
>> +       void (*signal_response_open)(struct ipmi_bmc_device *device);
>> +};
>> +
>> +/**
>> + * struct ipmi_bmc_bus - Bus driver that exchanges messages with the
>> host.
>> + * @send_response: Submits the given response to be sent to the host. May
>> return
>> + *                 -EBUSY if a response is already in progress, in which
>> case
>> + *                 the caller should wait for the response_open()
>> callback to be
>> + *                 called.
>> + * @is_response_open: Determines whether a response can currently be
>> sent. Note
>> + *                    that &ipmi_bmc_bus->send_response() may still fail
>> with
>> + *                    -EBUSY even after this returns true.
>> + *
>> + * This collection of callback functions represents a lower-level driver
>> which
>> + * acts as a connection to the host.
>> + */
>> +struct ipmi_bmc_bus {
>> +       int (*send_response)(struct ipmi_bmc_bus *bus,
>> +                            struct bt_msg *bt_response);
>> +       bool (*is_response_open)(struct ipmi_bmc_bus *bus);
>> +};
>> +
>> +/**
>> + * struct ipmi_bmc_ctx - Context object used to interact with the IPMI
>> BMC
>> + *                       core driver.
>> + * @bus: Pointer to the bus which is currently registered, or NULL for
>> none.
>> + * @default_device: Pointer to a device which will receive messages that
>> match
>> + *                  no other devices, or NULL if none is registered.
>> + * @devices: List of devices which are currently registered, besides the
>> default
>> + *           device.
>> + * @drivers_mutex: Mutex which protects against concurrent editing of the
>> + *                 bus driver, default device driver, and devices list.
>> + *
>> + * This struct should only be modified by the IPMI BMC core code and not
>> by bus
>> + * or device drivers.
>> + */
>> +struct ipmi_bmc_ctx {
>> +       struct ipmi_bmc_bus __rcu       *bus;
>> +       struct ipmi_bmc_device __rcu    *default_device;
>> +       struct list_head                devices;
>> +       struct mutex                    drivers_mutex;
>
>
> Since it does all it does, "drivers_mutex" IMHO is too specific a name.

Makes sense.

>
>
>> +};
>> +
>> +/**
>> + * ipmi_bmc_get_global_ctx() - Get a pointer to the global ctx.
>> + *
>> + * This function gets a reference to the global ctx object which is used
>> by
>> + * bus and device drivers to interact with the IPMI BMC core driver.
>> + *
>> + * Return: Pointer to the global ctx object.
>> + */
>> +struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx(void);
>> +
>>   /**
>>    * bt_msg_len() - Determine the total length of a Block Transfer
>> message.
>>    * @bt_msg: Pointer to the message.
>> @@ -73,4 +158,103 @@ static inline u8 bt_msg_payload_to_len(u8
>> payload_len)
>>         return payload_len + 3;
>>   }
>>   +/**
>> + * ipmi_bmc_send_response() - Send an IPMI response on the current bus.
>> + * @bt_response: The response message to send.
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
>> +                          struct bt_msg *bt_response);
>> +/**
>> + * ipmi_bmc_is_response_open() - Check whether we can currently send a
>> new
>> + *                               response.
>> + *
>> + * Note that even if this function returns true, ipmi_bmc_send_response()
>> may
>> + * still fail with -EBUSY if a response is submitted in the time between
>> the two
>> + * calls.
>> + *
>> + * Return: true if we can send a new response, false if one is already in
>> + *         progress.
>> + */
>> +bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx);
>> +/**
>> + * ipmi_bmc_register_device() - Register a new device driver.
>> + * @device: Pointer to the struct which represents this device,
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
>> +                            struct ipmi_bmc_device *device);
>> +/**
>> + * ipmi_bmc_unregister_device() - Unregister an existing device driver.
>> + * @device: Pointer to the struct which represents the existing device.
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
>> +                              struct ipmi_bmc_device *device);
>> +/**
>> + * ipmi_bmc_register_default_device() - Register a new default device
>> driver.
>> + * @device: Pointer to the struct which represents this device,
>> + *
>> + * Make this device the default device. If none of the other devices
>> match on a
>> + * particular message, this device will receive it instead.  Note that
>> only one
>> + * default device may be registered at a time.
>> + *
>> + * This functionalisty is currently used to allow messages which aren't
>> directly
>> + * handled by the kernel to be sent to userspace and handled there.
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
>> +                                    struct ipmi_bmc_device *device);
>> +/**
>> + * ipmi_bmc_unregister_default_device() - Unregister the existing default
>> device
>> + *                                        driver.
>> + * @device: Pointer to the struct which represents the existing device.
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
>> +                                      struct ipmi_bmc_device *device);
>> +/**
>> + * ipmi_bmc_handle_request() - Handle a new request that was received.
>> + * @bt_request: The request that was received.
>> + *
>> + * This is called by the bus driver when it receives a new request
>> message.
>> + *
>> + * Note that it may be called from an interrupt context.
>> + */
>> +void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
>> +                           struct bt_msg *bt_request);
>> +/**
>> + * ipmi_bmc_signal_response_open() - Notify the upper layer that a
>> response can
>> + *                                   be sent.
>> + *
>> + * This is called by the bus driver after it finishes sending a response
>> and is
>> + * ready to begin sending another one.
>> + */
>> +void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx);
>> +/**
>> + * ipmi_bmc_register_bus() - Register a new bus driver.
>> + * @bus: Pointer to the struct which represents this bus.
>> + *
>> + * Register a bus driver to handle communication with the host.
>> + *
>> + * Only one bus driver can be registered at any time.
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
>> +                         struct ipmi_bmc_bus *bus);
>> +/**
>> + * ipmi_bmc_unregister_bus() - Unregister an existing bus driver.
>> + * @bus: Pointer to the struct which represents the existing bus.
>> + *
>> + * Return: 0 on success, negative errno otherwise.
>> + */
>> +int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
>> +                           struct ipmi_bmc_bus *bus);
>> +
>>   #endif /* __LINUX_IPMI_BMC_H */
>
>
>

Thanks!

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

[Corey Minyard]

I'm removing LKML because we are leaving the realm of kernel stuff and 
talking mostly
about IPMI.

On 08/10/2017 08:06 PM, Brendan Higgins wrote:
> On Thu, Aug 10, 2017 at 6:58 AM, Corey Minyard <minyard@acm.org> wrote:
>> On 08/07/2017 10:52 PM, Brendan Higgins wrote:
>>> From: Benjamin Fair <benjaminfair@google.com>
>>>
>>> This patch introduces a framework for writing IPMI drivers which run on
>>> a Board Management Controller. It is similar in function to OpenIPMI.
>>> The framework handles registering devices and routing messages.
>>
>> Ok, I think I understand this.  I have a few nitpicky comments inline.  The
>> RCU usage
>> looks correct, and the basic design seems sound.
> Sweet
>
>> This piece of code takes a communication interface, called a bus, and
>> muxes/demuxes
>> messages on that bus to various users, called devices.  The name "devices"
>> confused
>> me for a bit, because I was thinking they were physical devices, what Linux
>> would
>> call a device.  I don't have a good suggestion for another name, though.
> We could maybe do "*_interface" instead of "*_bus" and "*_handler" instead
> of "*_device"; admittedly, it is not the best name ever: handler has some
> connotations.
>

I think the "_bus" name is ok, that's what I2C uses, and "communication 
bus" makes
sense, at least to me.  _handler is probably better than _device, but 
not that much.
The IPMI host driver uses _user, but that's not great, either. Maybe 
_service?  Naming
is such a pain.

>>
>>
>> I assume you would create one of these per bus for handling multiple busses,
>> which
>> you will obviously need to do in the future when IPMB comes.
> Yep, that is the intention. I was planning on adding support to use the device
> infrastructure so that multiple busses could be declared using device tree, etc.
> I do not have that now, but I thought that was a lot of work and I would want
> to get general buy-in before doing that. We just did enough to make code
> that achieves feature parity to what we have now and the core of the new
> proposed features.
>
>> I can see two big problems with the way the "match_request" is done:
>> * If multiple users want to handle the same request, only one of them will
>> get it
>>    and they will not know they have conflicted.
>> * Doing this for userland interfaces will be hard.
>> The other way to do this is for each user to register for each request type
>> and
>> manage it all in this code, which is kind of messy to use, but avoids the
>> above problems.
> Right, we considered this; however, I thought this is best because we also
> want to handle routing of OEM messages; I suppose we could register a
> type for OEM messages and then have a secondary set of handlers there;
> this would have some drawbacks though, the default handler interface would
> become a lot more complicated.
>
> On the other hand, now that I am thinking about it; having a common kernel
> interface for OEM messages could be really useful; this has definitely
> caused us some grief.
>
>> In thinking about the bigger picture here, you will need something like this
>> for every
>> communication interface the BMC supports: the system interface, LAN, IPMB,
>> ICMB
>> (let's hope not), and serial/modem interfaces (let's hope not, too, but
>> people really
>> use these in the field).  Maybe ICMB and serial aren't on your radar, but
>> I'd expect
>> LAN is pretty important, and you have already mentioned IPMB.
> Right, we are thinking about IPMB right now; I agree that the other stuff should
> be considered, but we don't really have a need for it now.
>
> My hope would be to make a similar interface for IPMB.
>
>> If you are thinking you will have a separate one of these for LAN in
>> userspace, I
>> would say just do it all in userspace.  For LAN you will have something that
>> has
>> to mux/demux all the messages from the LAN interface to the various users,
>> the
>> same code could sit on top of the current BT interface (and IPMB, etc.).
> So right now we do have handlers for a lot of basic commands in user space;
> this is why I have the default device file interface. However, it is incomplete
> and I am starting to look at commands that make more sense being
> implemented in the kernel.
>
> I have kind of danced around your point so far, for LAN, as far as I know we
> only support a REST interface right now; we should have the option of the
> LAN interface, but I don't think anyone has really tackled that yet.
>
> Basically, the way I see this now is sort of a mirror of what is done
> on the host
> side, we will have a kernel and a userland interface and then we will evolve it
> as necessary.
>
> In any case, I think there is probably a lot of room for additional discussion
> here.
>
>> I guess I'm trying to figure out how you expect all this work out in the
>> end.  What
>> you have now is a message mux/demux that can only have on thing underneath
>> it and one thing above it, which obviously isn't very useful.  Are you
>> thinking you
>> can have other in-kernel things that can handle specific messages? I'm
>> having
>> a hard time imagining that's the case.  Or are you thinking that you will
> Yes, I as I mentioned above, having handlers in the kernel is a prime motivator
> for this. I have been working on an interface for doing flash updates over IPMI.
> IPMI is not really a great way to do this in terms of performance or
> the interface
> it provides; however, IPMI is great in the sense that all platforms
> have it, so I
> want to have alternative backends for this flash interface and provide an IPMI
> OEM command implementation as a lowest common denominator option. I
> have some other examples, but i think this is one of the most interesting.
>
>> create
>> a userland interface to create a bus and then when a LAN connection comes
>> in you create one of these BMC contexts and route the LAN traffic through
>> this
>> code?  That's kind of clever, but I'm wondering if there would be better
>> ways
>> to do this than this design.
> Well, I think that is up for discussion; my main goal here is starting a
> conversation.
>
> As far as this being a complete design; I do not consider what I have
> presented as being complete. I mentioned some things above that I would like
> to add and some people have already chimed in asking for some changes.
> I just wanted to get some feedback before I went *too* far.

I'm not sure this is well know, but the OpenIPMI library has a fully 
functional BMC
that I wrote, originally for testing the library, but it has been 
deployed in a system
and people use it with QEMU.  So I have some experience here.

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

Firmware firewall, for instance, makes sense to implement in a single 
place that
handles all incoming messages.  However, it also deals with subcommands
(like making firewalls for each individual LAN parameter), so you either 
have
to put knowledge of the individual command structure in the main firewall,
or you have to embed pieces of the firewall in each command that has
subcommands.  But if you put it in the commands, then the commands
have to have knowledge of the communication channels.

I ended up running into this all over the place.  In the end I just 
hacked in
what I needed because what I designed was monolithic.  It seemed that
designing it in a way that was modular was so complex that it wasn't worth
the effort.  I've seen a few BMC designs, none were modular.

In the design you are working on here, firmware firewall will be a bit of a
challenge.

Also, if you implement a LAN interface, you have to deal with a shared
channel and privilege levels.  You will either have to have a context per
LAN connection, with user and privilege attached to the context, or you
will need a way to have user and privilege information in each message
so that the message router can handle rejecting messages it doesn't
have privilege to do, and the responses coming back will go to the
right connection.

There is also a strange situation where commands can come from a LAN
(or other) interface, be routed to a system interface where the host
picks up the command, handles it, send the response back to the
BMC which routes it back to the LAN interface.  People actually use this.

Another problem I see with this design is that most services don't care
about the interface the message comes from.  They won't want to have
to discover and make individual connections to each connection, they
will just want to say "Hook me up to everything, I don't care."

Some services will care, though.  Event queues and interface flag handling
will only want that on individual interfaces.  For these types of services,
it would be easier if they could discover and identify the interfaces.  If
interfaces are added dynamically (or each LAN connection appears as
a context) it needs a way to know when interfaces come and go.

If you end up implementing all this, you will have a fairly complex
piece of software in your message routing.  If you look at the message
handler in the host driver, it's fairly complex, but it makes the user's
job simple, and it makes the interfaces job simple(r).  IMHO that's a
fair trade-off.  If you have to have complexity, keep it in one place.

-corey

>> -corey
>>
>>> Signed-off-by: Benjamin Fair <benjaminfair@google.com>
>>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>>> ---
>>>    drivers/char/ipmi_bmc/Makefile   |   1 +
>>>    drivers/char/ipmi_bmc/ipmi_bmc.c | 294
>>> +++++++++++++++++++++++++++++++++++++++
>>>    include/linux/ipmi_bmc.h         | 184 ++++++++++++++++++++++++
>>>    3 files changed, 479 insertions(+)
>>>    create mode 100644 drivers/char/ipmi_bmc/ipmi_bmc.c
>>>
>>> diff --git a/drivers/char/ipmi_bmc/Makefile
>>> b/drivers/char/ipmi_bmc/Makefile
>>> index 8bff32b55c24..9c7cd48d899f 100644
>>> --- a/drivers/char/ipmi_bmc/Makefile
>>> +++ b/drivers/char/ipmi_bmc/Makefile
>>> @@ -2,5 +2,6 @@
>>>    # Makefile for the ipmi bmc drivers.
>>>    #
>>>    +obj-$(CONFIG_IPMI_BMC) += ipmi_bmc.o
>>>    obj-$(CONFIG_IPMI_BMC_BT_I2C) += ipmi_bmc_bt_i2c.o
>>>    obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += ipmi_bmc_bt_aspeed.o
>>> diff --git a/drivers/char/ipmi_bmc/ipmi_bmc.c
>>> b/drivers/char/ipmi_bmc/ipmi_bmc.c
>>> new file mode 100644
>>> index 000000000000..c1324ac9a83c
>>> --- /dev/null
>>> +++ b/drivers/char/ipmi_bmc/ipmi_bmc.c
>>
>> This is not really a BMC, it's a BMC message router, or something like that.
> How about ipmi_bmc_core.c or ipmi_slave_core.c ?
>
>>
>>> @@ -0,0 +1,294 @@
>>> +/*
>>> + * Copyright 2017 Google Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/errno.h>
>>> +#include <linux/export.h>
>>> +#include <linux/init.h>
>>> +#include <linux/ipmi_bmc.h>
>>> +#include <linux/list.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/rculist.h>
>>> +#include <linux/rcupdate.h>
>>> +
>>> +#define PFX "IPMI BMC core: "
>>> +
>>> +struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx()
>>> +{
>>> +       static struct ipmi_bmc_ctx global_ctx;
>>> +
>>> +       return &global_ctx;
>>> +}
>>> +
>>> +int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
>>> +                          struct bt_msg *bt_response)
>>> +{
>>> +       struct ipmi_bmc_bus *bus;
>>> +       int ret = -ENODEV;
>>> +
>>> +       rcu_read_lock();
>>> +       bus = rcu_dereference(ctx->bus);
>>> +
>>> +       if (bus)
>>> +               ret = bus->send_response(bus, bt_response);
>>> +
>>> +       rcu_read_unlock();
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_send_response);
>>> +
>>> +bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx)
>>> +{
>>> +       struct ipmi_bmc_bus *bus;
>>> +       bool ret = false;
>>> +
>>> +       rcu_read_lock();
>>> +       bus = rcu_dereference(ctx->bus);
>>> +
>>> +       if (bus)
>>> +               ret = bus->is_response_open(bus);
>>> +
>>> +       rcu_read_unlock();
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_is_response_open);
>>> +
>>> +int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
>>> +                            struct ipmi_bmc_device *device_in)
>>> +{
>>> +       struct ipmi_bmc_device *device;
>>> +
>>> +       mutex_lock(&ctx->drivers_mutex);
>>> +       /* Make sure it hasn't already been registered. */
>>> +       list_for_each_entry(device, &ctx->devices, link) {
>>> +               if (device == device_in) {
>>> +                       mutex_unlock(&ctx->drivers_mutex);
>>> +                       return -EINVAL;
>>> +               }
>>> +       }
>>> +
>>> +       list_add_rcu(&device_in->link, &ctx->devices);
>>> +       mutex_unlock(&ctx->drivers_mutex);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_register_device);
>>> +
>>> +int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
>>> +                              struct ipmi_bmc_device *device_in)
>>> +{
>>> +       struct ipmi_bmc_device *device;
>>> +       bool found = false;
>>> +
>>> +       mutex_lock(&ctx->drivers_mutex);
>>> +       /* Make sure it is currently registered. */
>>> +       list_for_each_entry(device, &ctx->devices, link) {
>>> +               if (device == device_in) {
>>> +                       found = true;
>>> +                       break;
>>> +               }
>>> +       }
>>> +       if (!found) {
>>> +               mutex_unlock(&ctx->drivers_mutex);
>>> +               return -ENXIO;
>>> +       }
>>> +
>>> +       list_del_rcu(&device_in->link);
>>> +       mutex_unlock(&ctx->drivers_mutex);
>>> +       synchronize_rcu();
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_unregister_device);
>>> +
>>> +int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
>>> +                                    struct ipmi_bmc_device *device)
>>> +{
>>> +       int ret;
>>> +
>>> +       mutex_lock(&ctx->drivers_mutex);
>>> +       if (!ctx->default_device) {
>>> +               ctx->default_device = device;
>>> +               ret = 0;
>>> +       } else {
>>> +               ret = -EBUSY;
>>> +       }
>>> +       mutex_unlock(&ctx->drivers_mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_register_default_device);
>>> +
>>> +int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
>>> +                                      struct ipmi_bmc_device *device)
>>> +{
>>> +       int ret;
>>> +
>>> +       mutex_lock(&ctx->drivers_mutex);
>>> +       if (ctx->default_device == device) {
>>> +               ctx->default_device = NULL;
>>> +               ret = 0;
>>> +       } else {
>>> +               ret = -ENXIO;
>>> +       }
>>> +       mutex_unlock(&ctx->drivers_mutex);
>>> +       synchronize_rcu();
>>> +
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_unregister_default_device);
>>> +
>>> +static u8 errno_to_ccode(int errno)
>>> +{
>>> +       switch (errno) {
>>> +       case EBUSY:
>>> +               return 0xC0; /* Node Busy */
>>> +       case EINVAL:
>>> +               return 0xC1; /* Invalid Command */
>>> +       case ETIMEDOUT:
>>> +               return 0xC3; /* Timeout while processing command */
>>> +       case ENOMEM:
>>> +               return 0xC4; /* Out of space */
>>> +       default:
>>> +               return 0xFF; /* Unspecified error */
>>
>> Instead of the hard-coded constants, you can use
>> include/uapi/linux/ipmi_msgdefs.h
>> and add things there as needed.  You probably knew that, but these numbers
>> should
>> all be the same.  Same for other constant that might apply.
> Noted.
>
>>
>>> +       }
>>> +}
>>> +
>>> +static void ipmi_bmc_send_error_response(struct ipmi_bmc_ctx *ctx,
>>> +                                        struct bt_msg *bt_request,
>>> +                                        u8 ccode)
>>> +{
>>> +       struct bt_msg error_response;
>>> +       int ret;
>>> +
>>> +       /* Payload contains 1 byte for completion code */
>>> +       error_response.len = bt_msg_payload_to_len(1);
>>> +       error_response.netfn_lun = bt_request->netfn_lun |
>>> +                                  IPMI_NETFN_LUN_RESPONSE_MASK;
>>> +       error_response.seq = bt_request->seq;
>>> +       error_response.cmd = bt_request->cmd;
>>> +       error_response.payload[0] = ccode;
>>> +
>>> +       /*
>>> +        * TODO(benjaminfair): Retry sending the response if it fails. The
>>> error
>>> +        * response might fail to send if another response is in progress.
>>> In
>>> +        * that case, the request will timeout rather than getting a more
>>> +        * specific completion code. This should buffer up responses and
>>> send
>>> +        * them when it can. Device drivers will generally handle error
>>> +        * reporting themselves; this code is only a fallback when that's
>>> not
>>> +        * available or when the drivers themselves fail.
>>> +        */
>>> +       ret = ipmi_bmc_send_response(ctx, &error_response);
>>> +       if (ret)
>>> +               pr_warn(PFX "Failed to reply with completion code %u:
>>> ipmi_bmc_send_response returned %d\n",
>>> +                       (u32) ccode, ret);
>>> +}
>>> +
>>> +void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
>>> +                            struct bt_msg *bt_request)
>>> +{
>>> +       struct ipmi_bmc_device *default_device;
>>> +       struct ipmi_bmc_device *device;
>>> +       int ret = -EINVAL;
>>> +
>>> +       rcu_read_lock();
>>> +       list_for_each_entry_rcu(device, &ctx->devices, link) {
>>> +               if (device->match_request(device, bt_request)) {
>>> +                       ret = device->handle_request(device, bt_request);
>>> +                       goto out;
>>> +               }
>>> +       }
>>> +
>>> +       /* No specific handler found. Use default handler instead */
>>> +       default_device = rcu_dereference(ctx->default_device);
>>> +       if (default_device)
>>> +               ret = default_device->handle_request(default_device,
>>> +                                                    bt_request);
>>> +
>>> +out:
>>> +       rcu_read_unlock();
>>> +       if (ret)
>>> +               ipmi_bmc_send_error_response(ctx, bt_request,
>>> +                                            errno_to_ccode(-ret));
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_handle_request);
>>> +
>>> +void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx)
>>> +{
>>> +       struct ipmi_bmc_device *default_device;
>>> +       struct ipmi_bmc_device *device;
>>> +
>>> +       rcu_read_lock();
>>> +       list_for_each_entry_rcu(device, &ctx->devices, link) {
>>> +               device->signal_response_open(device);
>>> +       }
>>> +
>>> +       default_device = rcu_dereference(ctx->default_device);
>>> +       if (default_device)
>>> +               default_device->signal_response_open(default_device);
>>> +
>>> +       rcu_read_unlock();
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_signal_response_open);
>>> +
>>> +int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
>>> +                         struct ipmi_bmc_bus *bus_in)
>>> +{
>>> +       int ret;
>>> +
>>> +       mutex_lock(&ctx->drivers_mutex);
>>> +       if (!ctx->bus) {
>>> +               ctx->bus = bus_in;
>>> +               ret = 0;
>>> +       } else {
>>> +               ret = -EBUSY;
>>> +       }
>>> +       mutex_unlock(&ctx->drivers_mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_register_bus);
>>> +
>>> +int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
>>> +                           struct ipmi_bmc_bus *bus_in)
>>> +{
>>> +       int ret;
>>> +
>>> +       mutex_lock(&ctx->drivers_mutex);
>>> +       /* Tried to unregister when another bus is registered */
>>> +       if (ctx->bus == bus_in) {
>>> +               ctx->bus = NULL;
>>> +               ret = 0;
>>> +       } else {
>>> +               ret = -ENXIO;
>>> +       }
>>> +       mutex_unlock(&ctx->drivers_mutex);
>>> +       synchronize_rcu();
>>> +
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL(ipmi_bmc_unregister_bus);
>>> +
>>> +static int __init ipmi_bmc_init(void)
>>> +{
>>> +       struct ipmi_bmc_ctx *ctx = ipmi_bmc_get_global_ctx();
>>> +
>>> +       mutex_init(&ctx->drivers_mutex);
>>> +       INIT_LIST_HEAD(&ctx->devices);
>>> +       return 0;
>>> +}
>>> +module_init(ipmi_bmc_init);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_AUTHOR("Benjamin Fair <benjaminfair@google.com>");
>>> +MODULE_DESCRIPTION("Core IPMI driver for the BMC side");
>>> diff --git a/include/linux/ipmi_bmc.h b/include/linux/ipmi_bmc.h
>>> index d0885c0bf190..2b494a79ec14 100644
>>> --- a/include/linux/ipmi_bmc.h
>>> +++ b/include/linux/ipmi_bmc.h
>>> @@ -20,6 +20,13 @@
>>>    #include <linux/types.h>
>>>      #define BT_MSG_PAYLOAD_LEN_MAX 252
>>> +#define BT_MSG_SEQ_MAX 255
>>> +
>>> +/*
>>> + * The bit in this mask is set in the netfn_lun field of an IPMI message
>>> to
>>> + * indicate that it is a response.
>>> + */
>>> +#define IPMI_NETFN_LUN_RESPONSE_MASK (1 << 2)
>>>      /**
>>>     * struct bt_msg - Block Transfer IPMI message.
>>> @@ -42,6 +49,84 @@ struct bt_msg {
>>>          u8 payload[BT_MSG_PAYLOAD_LEN_MAX];
>>>    } __packed;
>>>    +/**
>>> + * struct ipmi_bmc_device - Device driver that wants to receive ipmi
>>> requests.
>>> + * @link: Used to make a linked list of devices.
>>> + * @match_request: Used to determine whether a request can be handled by
>>> this
>>> + *                 device. Note that the matchers are checked in an
>>> arbitrary
>>> + *                 order.
>>> + * @handle_request: Called when a request is received for this device.
>>> + * @signal_response_open: Called when a response is done being sent and
>>> another
>>> + *                        device can send a message. Make sure to check
>>> that the
>>> + *                        bus isn't busy even after this is called,
>>> because all
>>> + *                        devices receive this call and another one may
>>> have
>>> + *                        already submitted a new response.
>>> + *
>>> + * This collection of callback functions represents an upper-level
>>> handler of
>>> + * IPMI requests.
>>> + *
>>> + * Note that the callbacks may be called from an interrupt context.
>>> + */
>>> +struct ipmi_bmc_device {
>>> +       struct list_head link;
>>> +
>>> +       bool (*match_request)(struct ipmi_bmc_device *device,
>>> +                             struct bt_msg *bt_request);
>>> +       int (*handle_request)(struct ipmi_bmc_device *device,
>>> +                             struct bt_msg *bt_request);
>>> +       void (*signal_response_open)(struct ipmi_bmc_device *device);
>>> +};
>>> +
>>> +/**
>>> + * struct ipmi_bmc_bus - Bus driver that exchanges messages with the
>>> host.
>>> + * @send_response: Submits the given response to be sent to the host. May
>>> return
>>> + *                 -EBUSY if a response is already in progress, in which
>>> case
>>> + *                 the caller should wait for the response_open()
>>> callback to be
>>> + *                 called.
>>> + * @is_response_open: Determines whether a response can currently be
>>> sent. Note
>>> + *                    that &ipmi_bmc_bus->send_response() may still fail
>>> with
>>> + *                    -EBUSY even after this returns true.
>>> + *
>>> + * This collection of callback functions represents a lower-level driver
>>> which
>>> + * acts as a connection to the host.
>>> + */
>>> +struct ipmi_bmc_bus {
>>> +       int (*send_response)(struct ipmi_bmc_bus *bus,
>>> +                            struct bt_msg *bt_response);
>>> +       bool (*is_response_open)(struct ipmi_bmc_bus *bus);
>>> +};
>>> +
>>> +/**
>>> + * struct ipmi_bmc_ctx - Context object used to interact with the IPMI
>>> BMC
>>> + *                       core driver.
>>> + * @bus: Pointer to the bus which is currently registered, or NULL for
>>> none.
>>> + * @default_device: Pointer to a device which will receive messages that
>>> match
>>> + *                  no other devices, or NULL if none is registered.
>>> + * @devices: List of devices which are currently registered, besides the
>>> default
>>> + *           device.
>>> + * @drivers_mutex: Mutex which protects against concurrent editing of the
>>> + *                 bus driver, default device driver, and devices list.
>>> + *
>>> + * This struct should only be modified by the IPMI BMC core code and not
>>> by bus
>>> + * or device drivers.
>>> + */
>>> +struct ipmi_bmc_ctx {
>>> +       struct ipmi_bmc_bus __rcu       *bus;
>>> +       struct ipmi_bmc_device __rcu    *default_device;
>>> +       struct list_head                devices;
>>> +       struct mutex                    drivers_mutex;
>>
>> Since it does all it does, "drivers_mutex" IMHO is too specific a name.
> Makes sense.
>
>>
>>> +};
>>> +
>>> +/**
>>> + * ipmi_bmc_get_global_ctx() - Get a pointer to the global ctx.
>>> + *
>>> + * This function gets a reference to the global ctx object which is used
>>> by
>>> + * bus and device drivers to interact with the IPMI BMC core driver.
>>> + *
>>> + * Return: Pointer to the global ctx object.
>>> + */
>>> +struct ipmi_bmc_ctx *ipmi_bmc_get_global_ctx(void);
>>> +
>>>    /**
>>>     * bt_msg_len() - Determine the total length of a Block Transfer
>>> message.
>>>     * @bt_msg: Pointer to the message.
>>> @@ -73,4 +158,103 @@ static inline u8 bt_msg_payload_to_len(u8
>>> payload_len)
>>>          return payload_len + 3;
>>>    }
>>>    +/**
>>> + * ipmi_bmc_send_response() - Send an IPMI response on the current bus.
>>> + * @bt_response: The response message to send.
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_send_response(struct ipmi_bmc_ctx *ctx,
>>> +                          struct bt_msg *bt_response);
>>> +/**
>>> + * ipmi_bmc_is_response_open() - Check whether we can currently send a
>>> new
>>> + *                               response.
>>> + *
>>> + * Note that even if this function returns true, ipmi_bmc_send_response()
>>> may
>>> + * still fail with -EBUSY if a response is submitted in the time between
>>> the two
>>> + * calls.
>>> + *
>>> + * Return: true if we can send a new response, false if one is already in
>>> + *         progress.
>>> + */
>>> +bool ipmi_bmc_is_response_open(struct ipmi_bmc_ctx *ctx);
>>> +/**
>>> + * ipmi_bmc_register_device() - Register a new device driver.
>>> + * @device: Pointer to the struct which represents this device,
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_register_device(struct ipmi_bmc_ctx *ctx,
>>> +                            struct ipmi_bmc_device *device);
>>> +/**
>>> + * ipmi_bmc_unregister_device() - Unregister an existing device driver.
>>> + * @device: Pointer to the struct which represents the existing device.
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_unregister_device(struct ipmi_bmc_ctx *ctx,
>>> +                              struct ipmi_bmc_device *device);
>>> +/**
>>> + * ipmi_bmc_register_default_device() - Register a new default device
>>> driver.
>>> + * @device: Pointer to the struct which represents this device,
>>> + *
>>> + * Make this device the default device. If none of the other devices
>>> match on a
>>> + * particular message, this device will receive it instead.  Note that
>>> only one
>>> + * default device may be registered at a time.
>>> + *
>>> + * This functionalisty is currently used to allow messages which aren't
>>> directly
>>> + * handled by the kernel to be sent to userspace and handled there.
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_register_default_device(struct ipmi_bmc_ctx *ctx,
>>> +                                    struct ipmi_bmc_device *device);
>>> +/**
>>> + * ipmi_bmc_unregister_default_device() - Unregister the existing default
>>> device
>>> + *                                        driver.
>>> + * @device: Pointer to the struct which represents the existing device.
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_unregister_default_device(struct ipmi_bmc_ctx *ctx,
>>> +                                      struct ipmi_bmc_device *device);
>>> +/**
>>> + * ipmi_bmc_handle_request() - Handle a new request that was received.
>>> + * @bt_request: The request that was received.
>>> + *
>>> + * This is called by the bus driver when it receives a new request
>>> message.
>>> + *
>>> + * Note that it may be called from an interrupt context.
>>> + */
>>> +void ipmi_bmc_handle_request(struct ipmi_bmc_ctx *ctx,
>>> +                           struct bt_msg *bt_request);
>>> +/**
>>> + * ipmi_bmc_signal_response_open() - Notify the upper layer that a
>>> response can
>>> + *                                   be sent.
>>> + *
>>> + * This is called by the bus driver after it finishes sending a response
>>> and is
>>> + * ready to begin sending another one.
>>> + */
>>> +void ipmi_bmc_signal_response_open(struct ipmi_bmc_ctx *ctx);
>>> +/**
>>> + * ipmi_bmc_register_bus() - Register a new bus driver.
>>> + * @bus: Pointer to the struct which represents this bus.
>>> + *
>>> + * Register a bus driver to handle communication with the host.
>>> + *
>>> + * Only one bus driver can be registered at any time.
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_register_bus(struct ipmi_bmc_ctx *ctx,
>>> +                         struct ipmi_bmc_bus *bus);
>>> +/**
>>> + * ipmi_bmc_unregister_bus() - Unregister an existing bus driver.
>>> + * @bus: Pointer to the struct which represents the existing bus.
>>> + *
>>> + * Return: 0 on success, negative errno otherwise.
>>> + */
>>> +int ipmi_bmc_unregister_bus(struct ipmi_bmc_ctx *ctx,
>>> +                           struct ipmi_bmc_bus *bus);
>>> +
>>>    #endif /* __LINUX_IPMI_BMC_H */
>>
>>
> Thanks!

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

[Patrick Williams]

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

On Mon, Aug 07, 2017 at 08:52:57PM -0700, Brendan Higgins wrote:
> Currently, OpenBMC handles all IPMI message routing and handling in userland;
> the existing drivers simply provide a file interface for the hardware on the
> device. In this patchset, we propose a common file interface to be shared by all
> IPMI hardware interfaces, but also a framework for implementing handlers at the
> kernel level, similar to how the existing OpenIPMI framework supports both
> kernel users, as well as misc device file interface.

Brendan,

Can you expand on why this is a good thing from an OpenBMC perspective?
We have a pretty significant set of IPMI providers that run in the
userspace daemon(s) and I can't picture more than a very small subset
even being possible to run in kernel space without userspace assistance.
We also already have an implementation of a RMCP+ daemon that can, and
does, share most of its providers with the host-side daemon.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Brendan Higgins]

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

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

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

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

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

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

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

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

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

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

>
> --
> Patrick Williams

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

Thanks!

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

[Brendan Higgins]

Sorry for the delayed response.

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

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

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

Is this the openipmi/lanserv?

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

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

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

That sounds like fun :-P

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

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

Nevertheless, that substantially broadens the scope of what I am trying
to do.

I think a good place to start is still to create a common interface for
hardware interfaces (BT, KCS, SSIF, and their varying implementations)
to implement, as I have done, and while we are working on the rest of the
stack on top of it, we have the device file interface that can be used in the
meantime.

Let me know what you think.

Thanks!

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

[Brendan Higgins]

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

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

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

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

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

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

[Corey Minyard]

On 08/23/2017 01:03 AM, Brendan Higgins wrote:

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

Yes.  That name is terrible, but it started out as a server that 
provided a LAN
interface to an existing BMC over the local interface, primarily for my 
testing.

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

If you just need the ability to catch a few commands in the kernel, what you
have is fairly complicated.  I think a simple notifier called from every 
driver
would provide what you needed with just a few lines of code.

As far as moving all the message routing to the kernel, my (fairly 
uneducated)
opinion would be that it's a bad idea.  It's a lot of complexity to put 
in there.
I can see some advantages to putting it there: it's simpler to interact 
with than
a userspace daemon and it gives consistent access to kernel and userspace
users.  But I don't know your whole design.

-corey

> Thanks!

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

[Brendan Higgins]

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

What do you mean by a simple notifier called from every driver? I think what
I have here is pretty simple. I think regardless of how we do routing, having
a common framework for low level IPMI hardware interfaces to implement
is pretty useful, even if it is only used to make a common dev file interface.

>
> As far as moving all the message routing to the kernel, my (fairly
> uneducated)
> opinion would be that it's a bad idea.  It's a lot of complexity to put in
> there.
> I can see some advantages to putting it there: it's simpler to interact with
> than
> a userspace daemon and it gives consistent access to kernel and userspace
> users.  But I don't know your whole design.

How about we focus on getting a common framework for the hardware
interfaces to implement? This way at least all of the hardware interfaces
are exposed the same way (similar to what you did on the host side). We
then at least have something to build on.

>
> -corey
>
>> Thanks!
>
>
>

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

[Corey Minyard]

On 09/06/2017 04:56 AM, Brendan Higgins wrote:
> On Thu, Aug 24, 2017 at 6:01 AM, Corey Minyard <minyard@acm.org> wrote:
>>
>> If you just need the ability to catch a few commands in the kernel, what you
>> have is fairly complicated.  I think a simple notifier called from every
>> driver
>> would provide what you needed with just a few lines of code.
> What do you mean by a simple notifier called from every driver? I think what
> I have here is pretty simple. I think regardless of how we do routing, having
> a common framework for low level IPMI hardware interfaces to implement
> is pretty useful, even if it is only used to make a common dev file interface.
>

I'm talking about maybe 10 lines of code to do this.  The device 
interface would
call  xxx_notifier_call_chain() for each message, and depending on the
return value it would send the message up or drop it.  Every piece of 
code that
cared about IPMI host messages would call 
xxx_notifier_chain_register/unregister.
That would be it.

>> As far as moving all the message routing to the kernel, my (fairly
>> uneducated)
>> opinion would be that it's a bad idea.  It's a lot of complexity to put in
>> there.
>> I can see some advantages to putting it there: it's simpler to interact with
>> than
>> a userspace daemon and it gives consistent access to kernel and userspace
>> users.  But I don't know your whole design.
> How about we focus on getting a common framework for the hardware
> interfaces to implement? This way at least all of the hardware interfaces
> are exposed the same way (similar to what you did on the host side). We
> then at least have something to build on.

The device interface you have in patch 2 provides that, and I think 
that's a good
thing.  Most other drivers like this have something similar.

What you have for the message routing seems like massive overkill for 
catching
a few messages in the kernel.  It's really more at the level that you 
have lots of
users in and out of kernel and you need to handle routing of all these 
messages.

The IPMI client side message handler is there for two reasons:  You have 
to be able
to route messages down and back up to all the users, and those users may be
sending the same commands.  And you need some way to allow multiple users
to send on the IPMB and return the responses to those users.  And then 
there's
that LAN routing thing.  It's a lot more complexity than I like, and it 
occasionally
causes me problems.  I would have preferred to have something simpler.

-corey