Advertising

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

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