On Fri, Apr 12, 2019 at 05:59:16PM -0400, Asmaa Mnebhi wrote: > 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.
I haven't done a detailed analysis of this, but from a cursory look it looks structurally good. I made a few small comments inline. But there is really very little here that is IPMB-specific. You could push the few IPMB-specific formatting things up to userland, I think. I am wondering if you could make this into a generic I2C slave interface. I'm adding the I2C mailing list to the CC and for their review. For the I2C mailing list... IPMB is a protocol that lets IPMI devices talk between themselves, so it is master/slave on all device on the IPMB. It's basically designed so that modular parts of the system (like a power supply) can have their own IPMI management controller that handles things for that device, and a management system can discover these and use them through the main IPMI management controller. It's used like a normal packet-oriented bus, with only writes, no reads. It seems to me that you could create a slave-specific I2C device that only received messages and took responses to those messages. You could use the normal I2C dev interface to to the sends and open a separate I2C slave dev interface to do the receives. -corey > > Signed-off-by: Asmaa Mnebhi <as...@mellanox.com> > --- > drivers/char/ipmi/Kconfig | 8 + > drivers/char/ipmi/Makefile | 1 + > drivers/char/ipmi/ipmb_dev_int.c | 418 > +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 427 insertions(+) > create mode 100644 drivers/char/ipmi/ipmb_dev_int.c > > diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig > index 94719fc..784bc84 100644 > --- a/drivers/char/ipmi/Kconfig > +++ b/drivers/char/ipmi/Kconfig > @@ -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' > + select I2C > + 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' > diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile > index 3f06b20..0822adc 100644 > --- a/drivers/char/ipmi/Makefile > +++ b/drivers/char/ipmi/Makefile > @@ -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 > diff --git a/drivers/char/ipmi/ipmb_dev_int.c > b/drivers/char/ipmi/ipmb_dev_int.c > new file mode 100644 > index 0000000..a45a686 > --- /dev/null > +++ b/drivers/char/ipmi/ipmb_dev_int.c > @@ -0,0 +1,418 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Mellanox IPMB driver to receive a request and send a response > + * > + * Copyright (C) 2018 Mellanox Techologies, Ltd. > + * > + * This was inspired by Brendan Higgins' ipmi-bmc-bt-i2c driver. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/errno.h> > +#include <linux/i2c.h> > +#include <linux/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 "IPMB DEV INT: " > + > +#define DEVICE_NAME "ipmb-dev" > + > +#define MAX_MSG_LEN 128 > +#define IPMB_REQUEST_LEN_MIN 7 > +#define NETFN_RSP_BIT_MASK 0x4 > +#define REQUEST_QUEUE_MAX_LEN 256 > + > +#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) > + > +struct ipmb_msg { > + u8 len; > + u8 rs_sa; > + u8 netfn_rs_lun; > + u8 checksum1; > + u8 rq_sa; > + u8 rq_seq_rq_lun; > + u8 cmd; > + u8 payload[IPMB_MSG_PAYLOAD_LEN_MAX]; > + /* checksum2 is included in payload */ > +} __packed; > + > +static u32 ipmb_msg_len(struct ipmb_msg *ipmb_msg) > +{ > + return ipmb_msg->len + 1; > +} > + > +struct ipmb_request_elem { > + struct list_head list; > + struct ipmb_msg request; > +}; > + > +struct ipmb_dev { > + struct i2c_client *client; > + struct miscdevice miscdev; > + struct ipmb_msg request; > + struct list_head request_queue; > + atomic_t request_queue_len; > + struct ipmb_msg response; > + size_t msg_idx; > + spinlock_t lock; > + wait_queue_head_t wait_queue; > + struct mutex file_mutex; > +}; > + > +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; > + > + if (!non_blocking) { > + > +try_again: > + 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 (!atomic_read(&ipmb_dev_p->request_queue_len)) { > + spin_unlock_irqrestore(&ipmb_dev_p->lock, flags); > + if (non_blocking) > + return -EAGAIN; > + goto try_again; > + } > + > + if (list_empty(&ipmb_dev_p->request_queue)) { > + pr_err(PFX "request_queue is empty\n"); > + 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; > +} > + > +static inline struct ipmb_dev *to_ipmb_dev(struct file *file) > +{ > + return container_of(file->private_data, struct ipmb_dev, miscdev); > +} > + > +static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count, > + loff_t *ppos) > +{ > + struct ipmb_dev *ipmb_dev_p = to_ipmb_dev(file); > + struct ipmb_msg msg; > + ssize_t ret; > + > + if (!ipmb_dev_p) > + return -ENOMEM; I don't think this can happen. Same for other things like this. > + > + memset(&msg, 0, sizeof(msg)); > + > + mutex_lock(&ipmb_dev_p->file_mutex); > + ret = receive_ipmb_request(ipmb_dev_p, file->f_flags & O_NONBLOCK, > + &msg); > + if (ret < 0) > + goto out; > + count = min_t(size_t, count, ipmb_msg_len(&msg)); > + if (copy_to_user(buf, &msg, count)) { > + ret = -EFAULT; > + goto out; > + } > + > +out: > + mutex_unlock(&ipmb_dev_p->file_mutex); > + if (ret < 0) > + return ret; > + else > + return count; > +} > + > +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; > + > + 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; > +} > + > +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; > + > + if (!ipmb_dev_p) > + return -ENOMEM; > + > + if (count > sizeof(msg)) > + return -EINVAL; > + > + if (copy_from_user(&msg, buf, count) || count < msg[0]) > + return -EINVAL; copy_from_user() should return -EFAULT on an error. > + > + rq_sa = (u16)(msg[RQ_SA_8BIT_IDX] >> 1); > + netf_rq_lun = msg[NETFN_LUN_IDX]; > + /* > + * 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] - 2; > + > + 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 + 3); > + mutex_unlock(&ipmb_dev_p->file_mutex); > + > + if (ret) > + return ret; > + else > + return count; > +} > + > +static unsigned int ipmb_poll(struct file *file, poll_table *wait) > +{ > + struct ipmb_dev *ipmb_dev_p = to_ipmb_dev(file); > + unsigned int mask = 0; > + > + mutex_lock(&ipmb_dev_p->file_mutex); > + poll_wait(file, &ipmb_dev_p->wait_queue, wait); > + > + if (atomic_read(&ipmb_dev_p->request_queue_len)) > + mask |= POLLIN; > + mask |= POLLOUT; > + mutex_unlock(&ipmb_dev_p->file_mutex); > + return mask; > +} > + > +static const struct file_operations ipmb_fops = { > + .owner = THIS_MODULE, > + .read = ipmb_read, > + .write = ipmb_write, > + .poll = ipmb_poll, > +}; > + > +/* Called with ipmb_dev->lock held. */ > +static int handle_request(struct ipmb_dev *ipmb_dev_p) The return value from handle_request is ignored below, do you need to return it? > +{ > + struct ipmb_request_elem *queue_elem; > + > + if (atomic_read(&ipmb_dev_p->request_queue_len) >= > + REQUEST_QUEUE_MAX_LEN) > + return -EFAULT; EFAULT is not what you want here. I'm not sure exactly what to do here, but you should probably just drop the data on the floor, maybe have a counter to count dropped packets. > + > + queue_elem = kmalloc(sizeof(*queue_elem), GFP_KERNEL); > + if (!queue_elem) > + return -ENOMEM; > + > + memcpy(&queue_elem->request, &ipmb_dev_p->request, > + sizeof(struct ipmb_msg)); > + list_add(&queue_elem->list, &ipmb_dev_p->request_queue); > + atomic_inc(&ipmb_dev_p->request_queue_len); > + wake_up_all(&ipmb_dev_p->wait_queue); > + > + return 0; > +} > + > +static u8 verify_checksum1(struct ipmb_dev *ipmb_dev_p, u8 rs_sa) > +{ > + return (rs_sa + ipmb_dev_p->request.netfn_rs_lun + > + ipmb_dev_p->request.checksum1); > +} > + > +static bool is_ipmb_request(struct ipmb_dev *ipmb_dev_p, u8 rs_sa) > +{ > + if (ipmb_dev_p->msg_idx >= IPMB_REQUEST_LEN_MIN) { > + if (verify_checksum1(ipmb_dev_p, rs_sa)) > + return false; > + > + /* > + * Check whether this is an IPMB request or > + * response. > + * The 6 MSB of netfn_rs_lun are dedicated to the netfn > + * while the remaining bits are dedicated to the lun. > + * If the LSB of the netfn is cleared, it is associated > + * with an IPMB request. > + * If the LSB of the netfn is set, it is associated with > + * an IPMB response. > + */ > + if (!(ipmb_dev_p->request.netfn_rs_lun & NETFN_RSP_BIT_MASK)) > + return true; > + } > + return false; > +} > + > +/* > + * The IPMB protocol only supports I2C Writes so there is no need > + * to support I2C_SLAVE_READ* events. > + * This i2c callback function only monitors IPMB request messages > + * and adds them in a queue, so that they can be handled by > + * receive_ipmb_request. > + */ > +static int ipmb_slave_cb(struct i2c_client *client, > + enum i2c_slave_event event, u8 *val) > +{ > + struct ipmb_dev *ipmb_dev_p = i2c_get_clientdata(client); > + u8 *buf = (u8 *)&ipmb_dev_p->request; > + > + spin_lock(&ipmb_dev_p->lock); > + switch (event) { > + case I2C_SLAVE_WRITE_REQUESTED: > + memset(&ipmb_dev_p->request, 0, sizeof(ipmb_dev_p->request)); > + ipmb_dev_p->msg_idx = 0; > + > + /* > + * At index 0, ipmb_msg stores the length of msg, > + * skip it for now. > + * The len will be populated once the whole > + * buf is populated. > + * > + * The I2C bus driver's responsibility is to pass the > + * data bytes to the backend driver; it does not > + * forward the i2c slave address. > + * Since the first byte in the IPMB message is the > + * address of the responder, it is the responsibility > + * of the IPMB driver to format the message properly. > + * So this driver prepends the address of the responder > + * to the received i2c data before the request message > + * is handled in userland. > + */ > + buf[++ipmb_dev_p->msg_idx] = (u8)(client->addr << 1); > + break; > + > + case I2C_SLAVE_WRITE_RECEIVED: > + if (ipmb_dev_p->msg_idx >= sizeof(struct ipmb_msg)) > + break; > + > + buf[++ipmb_dev_p->msg_idx] = *val; > + break; > + > + case I2C_SLAVE_STOP: > + ipmb_dev_p->request.len = ipmb_dev_p->msg_idx; > + > + if (is_ipmb_request(ipmb_dev_p, (u8)(client->addr << 1))) > + handle_request(ipmb_dev_p); > + break; > + > + default: > + break; > + } > + spin_unlock(&ipmb_dev_p->lock); > + > + return 0; > +} > + > +static int ipmb_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct ipmb_dev *ipmb_dev_p; > + int ret; > + > + ipmb_dev_p = devm_kzalloc(&client->dev, sizeof(*ipmb_dev_p), > + GFP_KERNEL); > + if (!ipmb_dev_p) > + return -ENOMEM; > + > + spin_lock_init(&ipmb_dev_p->lock); > + init_waitqueue_head(&ipmb_dev_p->wait_queue); > + atomic_set(&ipmb_dev_p->request_queue_len, 0); > + INIT_LIST_HEAD(&ipmb_dev_p->request_queue); > + > + mutex_init(&ipmb_dev_p->file_mutex); > + > + ipmb_dev_p->miscdev.minor = MISC_DYNAMIC_MINOR; > + ipmb_dev_p->miscdev.name = DEVICE_NAME; > + ipmb_dev_p->miscdev.fops = &ipmb_fops; > + ipmb_dev_p->miscdev.parent = &client->dev; > + ret = misc_register(&ipmb_dev_p->miscdev); > + if (ret) > + return ret; > + > + ipmb_dev_p->client = client; > + i2c_set_clientdata(client, ipmb_dev_p); > + ret = i2c_slave_register(client, ipmb_slave_cb); > + if (ret) { > + misc_deregister(&ipmb_dev_p->miscdev); > + return ret; > + } > + > + return 0; > +} > + > +static int ipmb_remove(struct i2c_client *client) > +{ > + struct ipmb_dev *ipmb_dev_p = i2c_get_clientdata(client); > + > + if (!ipmb_dev_p) > + return 0; > + > + i2c_slave_unregister(client); > + misc_deregister(&ipmb_dev_p->miscdev); > + > + return 0; > +} > + > +static const struct i2c_device_id ipmb_id[] = { > + {"ipmb-dev", 0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(i2c, ipmb_id); > + > +static struct i2c_driver ipmb_driver = { > + .driver = { > + .name = "ipmb-dev", > + }, > + .probe = ipmb_probe, > + .remove = ipmb_remove, > + .id_table = ipmb_id, > +}; > +module_i2c_driver(ipmb_driver); > + > +MODULE_AUTHOR("Mellanox Technologies"); > +MODULE_DESCRIPTION("Mellanox BlueField IPMB driver"); > +MODULE_LICENSE("GPL"); > -- > 2.1.2 >