Add support for IPMB driver
Subsystems (3)
v7 cover Add support for IPMB driver
Hi Vadim,
I have addressed your comments:
1) removed typecasting and replaced it with a macro to get the 8 bit address. I think it is more readable this way.
2) added space
Thank you for your feedback!
Asmaa Mnebhi (1):
Add support for IPMB driver
Documentation/IPMB.txt | 103 ++++++++++ drivers/char/ipmi/Kconfig | 8 + drivers/char/ipmi/Makefile | 1 + drivers/char/ipmi/ipmb_dev_int.c | 399 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 511 insertions(+) create mode 100644 Documentation/IPMB.txt create mode 100644 drivers/char/ipmi/ipmb_dev_int.c
Signature · 1 line
2.1.2
PATCH v7 1/1 Add support for IPMB driver
Support receiving IPMB requests on a Satellite MC from the BMC. Once a response is ready, this driver will send back a response to the BMC via the IPMB channel.
Signed-off-by Asmaa Mnebhi <[email protected]>
--- Documentation/IPMB.txt | 103 ++++++++++ drivers/char/ipmi/Kconfig | 8 + drivers/char/ipmi/Makefile | 1 + drivers/char/ipmi/ipmb_dev_int.c | 399 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 511 insertions(+) create mode 100644 Documentation/IPMB.txt create mode 100644 drivers/char/ipmi/ipmb_dev_int.c
Documentation/IPMB.txt new file +103 -0
@@ -0,0 +1,103 @@ +============================ +IPMB Driver fro Satellite MC +============================ + +The Intelligent Platform Management Bus, or IPMB is an +I2C bus that provides a standardized interconnection between +different boards within a chassis. This interconnection is +between the baseboard management (BMC) and chassis electronics. +IPMB is also associated with the messaging protocol through the +IPMB bus. + +The devices using the IPMB are usually management +controllers that perform management functions such as servicing +the front panel interface, monitoring the baseboard, +hot-swapping disk drivers in the system chassis, etc... + +When an IPMB is implemented in the system, the BMC serves as +a controller to give system software access to the IPMB. The BMC +sends IPMI requests to a device (usually a Satellite Management +Controller or Satellite MC) via IPMB and the device +sends a response back to the BMC. + +For more information on IPMB and the format of an IPMB message, +refer to the IPMB and IPMI specifications. + +IPMB driver for Satellite MC +---------------------------- + +ipmb-dev-int - This is the driver needed on a Satellite MC to +receive IPMB messages from a BMC and send a response back. +This driver works hand with the i2c driver and a userspace +program such as OpenIPMI: + +1) It is an I2C slave backend driver. So, it defines a callback +function to set the Satellite MC as an I2C slave. +This callback function handles the received IPMI requests. + +2) It defines the read and write functions to enable a user +space program (such as OpenIPMI) to communicate with the kernel. + + +Load the IPMB driver +-------------------- + +The driver needs to be loaded at boot time or manually first. +First, make sure you have the following in your config file: +CONFIG_IPMB_DEVICE_INTERFACE=y + +1) If you want the driver to be loaded at boot time: + +a) Add this entry to your ACPI table, under the appropriate SMBus: + +Device (SMB0) // Example SMBus host controller +{ + Name (_HID, "<Vendor-Specific HID>") // Vendor-Specific HID + Name (_UID, 0) // Unique ID of particular host controller + : + : + Device (IPMB) + { + Name (_HID, "IPMB0001") // IPMB device interface + Name (_UID, 0) // Unique device identifier + } +} + +b) Example for device tree: + +&i2c2 { + status = "okay"; + + ipmb@10 { + compatible = "ipmb-dev"; + reg = <0x10>; + }; +}; + +2) Manually from linux: +modprobe ipmb-dev-int + + +Instantiate the device +---------------------- + +After loading the driver, you can instantiate the device as +described in the document 'instantiating-devices'. +If you have multiple BMCs, each connected to your Satellite MC via +a different I2C bus, you can instantiate a device for each of +those BMCs. +The name of the instantiated device contains the I2C bus number +associated with it as follows: + +BMC1 ------ IPMB/I2C bus 1 ---------| /dev/ipmb-1 + Satellite MC +BMC1 ------ IPMB/I2C bus 2 ---------| /dev/ipmb-2 + +For instance, you can instantiate the ipmb-dev-int device from +user space at the 7 bit address 0x10 on bus 2: + + # echo ipmb-dev 0x10 > /sys/bus/i2c/devices/i2c-2/new_device + +This will create device file /dev/ipmb-2, which can be accessed +by the user space program. The device needs to be instantiated +before running the user space program.
drivers/char/ipmi/Kconfig +8 -0
@@ -74,6 +74,14 @@ config IPMI_SSIF have a driver that must be accessed over an I2C bus instead of a standard interface. This module requires I2C support. +config IPMB_DEVICE_INTERFACE + tristate 'IPMB Interface handler' + depends on I2C && I2C_SLAVE + help + Provides a driver for a device (Satellite MC) to + receive requests and send responses back to the BMC via + the IPMB interface. This module requires I2C support. + config IPMI_POWERNV depends on PPC_POWERNV tristate 'POWERNV (OPAL firmware) IPMI interface'
drivers/char/ipmi/Makefile +1 -0
@@ -26,3 +26,4 @@ obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o +obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
drivers/char/ipmi/ipmb_dev_int.c new file +399 -0
399 lines not on this page · raw message
Signature · 1 line
2.1.2
Hi,
some more words from the I2C world.
Documentation/IPMB.txt line 99
Quoted 4 lines
Documentation/IPMB.txt new file +4
-0+For instance, you can instantiate the ipmb-dev-int device from +user space at the 7 bit address 0x10 on bus 2: + + # echo ipmb-dev 0x10 > /sys/bus/i2c/devices/i2c-2/new_device
"0x1010" as described in Documentation/i2c/slave-interface
drivers/char/ipmi/Kconfig config IPMI_SSIF · line 79
Quoted 3 lines
drivers/char/ipmi/Kconfig +3
-0+config IPMB_DEVICE_INTERFACE + tristate 'IPMB Interface handler' + depends on I2C && I2C_SLAVE
Minor nit: I2C could be dropped because I2C_SLAVE depends on it.
drivers/char/ipmi/Makefile obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o · line 29
Quoted 7 lines
drivers/char/ipmi/Makefile +1
-0@@ -26,3 +26,4 @@ obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o +obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o
Dunno if IPMI maintainers care about sorting here?
Quoted 1 line
+#define
dev_fmt(fmt)"ipmb_dev_int: " fmt
I think this can go now. dev_* with miscchar device (as it is done now) should be all good.
Quoted 5 lines
drivers/char/ipmi/Makefile +5
-0+#define MAX_MSG_LEN 128 +#define IPMB_REQUEST_LEN_MIN 7 +#define NETFN_RSP_BIT_MASK 0x4 +#define REQUEST_QUEUE_MAX_LEN 256 +
drivers/char/ipmi/Makefile
Quoted 10 lines
drivers/char/ipmi/Makefile +10
-0+#define IPMB_MSG_LEN_IDX 0 +#define RQ_SA_8BIT_IDX 1 +#define NETFN_LUN_IDX 2 + +#define IPMB_MSG_PAYLOAD_LEN_MAX (MAX_MSG_LEN - IPMB_REQUEST_LEN_MIN - 1) + +#define SMBUS_MSG_HEADER_LENGTH 2 +#define SMBUS_MSG_IDX_OFFSET (SMBUS_MSG_HEADER_LENGTH + 1) + +#define GET_8BIT_ADDR(addr_7bit) ((addr_7bit << 1) && 0xff)
Still wondering about the tabs after define.
Quoted 15 lines
drivers/char/ipmi/Makefile +15
-0+static int receive_ipmb_request(struct ipmb_dev *ipmb_dev_p, + bool non_blocking, + struct ipmb_msg *ipmb_request) +{ + struct ipmb_request_elem *queue_elem; + unsigned long flags; + int res; + + spin_lock_irqsave(&ipmb_dev_p->lock, flags); + + while (!atomic_read(&ipmb_dev_p->request_queue_len)) { + spin_unlock_irqrestore(&ipmb_dev_p->lock, flags); + if (non_blocking) + return -EAGAIN; +
drivers/char/ipmi/Makefile
Quoted 10 lines
drivers/char/ipmi/Makefile +10
-0+ res = wait_event_interruptible(ipmb_dev_p->wait_queue, + atomic_read(&ipmb_dev_p->request_queue_len)); + if (res) + return res; + + spin_lock_irqsave(&ipmb_dev_p->lock, flags); + } + + if (list_empty(&ipmb_dev_p->request_queue)) { + dev_err(&ipmb_dev_p->client->dev, "request_queue is empty\n");
Spinlock still held?? Kinda proves that the flow of code is still hard to read. (I mean it is way better without the goto, but still...)
Quoted 15 lines
drivers/char/ipmi/Makefile +15
-0+ return -EIO; + } + + queue_elem = list_first_entry(&ipmb_dev_p->request_queue, + struct ipmb_request_elem, list); + memcpy(ipmb_request, &queue_elem->request, sizeof(*ipmb_request)); + list_del(&queue_elem->list); + kfree(queue_elem); + atomic_dec(&ipmb_dev_p->request_queue_len); + + spin_unlock_irqrestore(&ipmb_dev_p->lock, flags); + + return 0; +} +
...
Quoted 11 lines
drivers/char/ipmi/Makefile +11
-0+static s32 i2c_smbus_write_block_data_local(struct i2c_client *client, + u8 command, u8 length, + u16 requester_i2c_addr, + const char *msg) +{ + union i2c_smbus_data data; + int ret; + + if (length > I2C_SMBUS_BLOCK_MAX) + length = I2C_SMBUS_BLOCK_MAX; +
drivers/char/ipmi/Makefile
Quoted 10 lines
drivers/char/ipmi/Makefile +10
-0+ data.block[0] = length; + memcpy(&data.block[1], msg, length); + + ret = i2c_smbus_xfer(client->adapter, requester_i2c_addr, + client->flags, + I2C_SMBUS_WRITE, command, + I2C_SMBUS_BLOCK_DATA, &data); + + return ret; +}
This function must go. You need it solely to pass 'requester_i2c_addr' along, but this shows that you are using the wrong i2c_client struct. And, in deed, you don't want your own here because you don't want to send to yourself here. Usually, you'd register a new i2c_client with the address you want to talk to using 'i2c_new_device'. However, since this is a userspace interface, I guess we can do something similar as in i2c-dev.c, namely an anonymous i2c_client. Read on.
drivers/char/ipmi/Makefile
Quoted 8 lines
drivers/char/ipmi/Makefile +8
-0+ +static ssize_t ipmb_write(struct file *file, const char __user *buf, + size_t count, loff_t *ppos) +{ + struct ipmb_dev *ipmb_dev_p = to_ipmb_dev(file); + u8 msg[MAX_MSG_LEN]; + ssize_t ret; + u8 rq_sa, netf_rq_lun, msg_len;
From the top of my head, not sure if I got all details right:
+ struct i2c_client rq_client = { };
drivers/char/ipmi/Makefile
Quoted 8 lines
drivers/char/ipmi/Makefile +8
-0+ if (count > sizeof(msg)) + return -EINVAL; + + if (copy_from_user(&msg, buf, count) || count < msg[0]) + return -EFAULT; + + rq_sa = msg[RQ_SA_8BIT_IDX] >> 1; + netf_rq_lun = msg[NETFN_LUN_IDX];
+ rq_client.name = "IPMB anonymous"
+ rq_client.adapter = ipmb_dev_p->client->adapter;
+ rq_client.addr = rq_sa;
drivers/char/ipmi/Makefile
Quoted 10 lines
drivers/char/ipmi/Makefile +10
-0+ /* + * subtract rq_sa and netf_rq_lun from the length of the msg passed to + * i2c_smbus_write_block_data_local + */ + msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH; + + mutex_lock(&ipmb_dev_p->file_mutex); + ret = i2c_smbus_write_block_data_local(ipmb_dev_p->client, + netf_rq_lun, msg_len, rq_sa, msg + + SMBUS_MSG_IDX_OFFSET);
and then replace the call above with
ret = i2c_smbus_write_block_data(&rq_client, ...)
Quoted 5 lines
drivers/char/ipmi/Makefile +5
-0+ mutex_unlock(&ipmb_dev_p->file_mutex); + + return ret ? : count; +} +
...
drivers/char/ipmi/Makefile
Quoted 4 lines
drivers/char/ipmi/Makefile +4
-0+static int ipmb_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct ipmb_dev *ipmb_dev_p;
And I still dislike the _p suffix. It is not common Kernel coding style and unless it is common in this subsystem, I'd suggest to drop it for consistency reasons.
Thanks for keeping at this driver,
Wolfram