[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

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!

[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

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