On Mon, 29 Mar 2021 at 12:18, Quan Nguyen <quan@xxxxxxxxxxxxxxxxxxxxxx> wrote: > > The SMBus system interface (SSIF) IPMI BMC driver can be used to perform > in-band IPMI communication with their host in management (BMC) side. > > This commits adds support specifically for Aspeed AST2500 which commonly > used as Board Management Controllers. > > Signed-off-by: Quan Nguyen <quan@xxxxxxxxxxxxxxxxxxxxxx> I don't have any SSIF or IPMI related feedback on your patch, but some general things I noticed when reading it. > --- > drivers/char/ipmi/Kconfig | 22 + > drivers/char/ipmi/Makefile | 2 + > drivers/char/ipmi/ssif_bmc.c | 645 ++++++++++++++++++++++++++++ > drivers/char/ipmi/ssif_bmc.h | 92 ++++ > drivers/char/ipmi/ssif_bmc_aspeed.c | 132 ++++++ > 5 files changed, 893 insertions(+) > create mode 100644 drivers/char/ipmi/ssif_bmc.c > create mode 100644 drivers/char/ipmi/ssif_bmc.h > create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c It would make sense to split the aspeed implementation into a separate patch form the ssif framework. > +++ b/drivers/char/ipmi/ssif_bmc.c > @@ -0,0 +1,645 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * The driver for BMC side of SSIF interface > + * > + * Copyright (c) 2021, Ampere Computing LLC > + * > + * 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. > + * > + * 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. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <https://www.gnu.org/licenses/>. You should omit the licence text; it is replaced by the SPDX tags. > +static int send_ssif_bmc_response(struct ssif_bmc_ctx *ssif_bmc, bool non_blocking) > +{ > + unsigned long flags; > + int ret; > + > + if (!non_blocking) { > +retry: > + ret = wait_event_interruptible(ssif_bmc->wait_queue, > + !ssif_bmc->response_in_progress); > + if (ret) > + return ret; > + } > + > + spin_lock_irqsave(&ssif_bmc->lock, flags); What's the lock doing here? We've just waited for response_in_progress to be false, so we then take the lock to check what value it is? Should it also be sending some data in this function? > + if (ssif_bmc->response_in_progress) { > + spin_unlock_irqrestore(&ssif_bmc->lock, flags); > + if (non_blocking) > + return -EAGAIN; > + > + goto retry; The goto threw me, so I tried re-writing it without. Again, I don't quite follow what the spinlock is doing. while (1) { if (blocking) { ret = wait_event_interruptible(); if (ret) return ret; } spin_lock_irqsave() if (ssif_bmc->response_in_progress) { spin_lock_irqrestore() if (!blocking) return -EAGAIN; } else { spin_lock_irqrestore() break; } } > + } > + > + /* > + * Check the response data length from userspace to determine the type > + * of the response message whether it is single-part or multi-part. > + */ > + ssif_bmc->is_singlepart_read = > + (ssif_msg_len(&ssif_bmc->response) <= (MAX_PAYLOAD_PER_TRANSACTION + 1)) ? > + true : false; /* 1: byte of length */ I don't follow the 1: byte of length comment, what is it telling me? The ternary operator is a bit messy here, I'd go for a good old if statement. > + > + ssif_bmc->response_in_progress = true; > + spin_unlock_irqrestore(&ssif_bmc->lock, flags); > + > + return 0; > +} > +/* Handle SSIF message that will be sent to user */ > +static ssize_t ssif_bmc_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) > +{ > + struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file); > + struct ssif_msg msg; > + unsigned long flags; > + ssize_t ret; > + > + mutex_lock(&ssif_bmc->file_mutex); ->file_mutex is protecting the device against more than one user of the character device? Can you enforce that in open() instead? > + > + ret = receive_ssif_bmc_request(ssif_bmc, file->f_flags & O_NONBLOCK); > + if (ret < 0) > + goto out; > + > + spin_lock_irqsave(&ssif_bmc->lock, flags); > + count = min_t(ssize_t, count, ssif_msg_len(&ssif_bmc->request)); count is user controlled, so I assume ssif_msg_len will always be less than or equal to struct ssif_msg? > + memcpy(&msg, &ssif_bmc->request, count); > + ssif_bmc->request_available = false; > + spin_unlock_irqrestore(&ssif_bmc->lock, flags); > + > + ret = copy_to_user(buf, &msg, count); > +out: > + mutex_unlock(&ssif_bmc->file_mutex); > + > + return (ret < 0) ? ret : count; > +} > + > +/* Handle SSIF message that is written by user */ > +static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t count, > + loff_t *ppos) > +{ > + struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file); > + struct ssif_msg msg; > + unsigned long flags; > + ssize_t ret; > + > + if (count > sizeof(struct ssif_msg)) > + return -EINVAL; > + > + mutex_lock(&ssif_bmc->file_mutex); > + > + ret = copy_from_user(&msg, buf, count); > + if (ret) > + goto out; > + > + spin_lock_irqsave(&ssif_bmc->lock, flags); > + if (count >= ssif_msg_len(&ssif_bmc->response)) Is that test correct? > + memcpy(&ssif_bmc->response, &msg, count); > + else > + ret = -EINVAL; > + spin_unlock_irqrestore(&ssif_bmc->lock, flags); > + > + if (ret) > + goto out; > + > + ret = send_ssif_bmc_response(ssif_bmc, file->f_flags & O_NONBLOCK); > + if (!ret && ssif_bmc->set_ssif_bmc_status) > + ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_READY); > +out: > + mutex_unlock(&ssif_bmc->file_mutex); > + > + return (ret < 0) ? ret : count; > +} > + > +static long ssif_bmc_ioctl(struct file *file, unsigned int cmd, unsigned long param) > +{ If you're not using this I suspect you should omit the callback. > + return 0; > +} > + > +static unsigned int ssif_bmc_poll(struct file *file, poll_table *wait) > +{ > + struct ssif_bmc_ctx *ssif_bmc = to_ssif_bmc(file); > + unsigned int mask = 0; > + > + mutex_lock(&ssif_bmc->file_mutex); > + poll_wait(file, &ssif_bmc->wait_queue, wait); > + > + /* > + * The request message is now available so userspace application can > + * get the request > + */ > + if (ssif_bmc->request_available) > + mask |= POLLIN; > + > + mutex_unlock(&ssif_bmc->file_mutex); > + return mask; > +} > + > +/* > + * System calls to device interface for user apps > + */ > +static const struct file_operations ssif_bmc_fops = { > + .owner = THIS_MODULE, > + .read = ssif_bmc_read, > + .write = ssif_bmc_write, > + .poll = ssif_bmc_poll, > + .unlocked_ioctl = ssif_bmc_ioctl, > +}; > + > +/* Called with ssif_bmc->lock held. */ > +static int handle_request(struct ssif_bmc_ctx *ssif_bmc) Could return void. > +{ > + if (ssif_bmc->set_ssif_bmc_status) > + ssif_bmc->set_ssif_bmc_status(ssif_bmc, SSIF_BMC_BUSY); > + > + /* Request message is available to process */ > + ssif_bmc->request_available = true; > + /* > + * This is the new READ request. > + * Clear the response buffer of the previous transaction > + */ > + memset(&ssif_bmc->response, 0, sizeof(struct ssif_msg)); > + wake_up_all(&ssif_bmc->wait_queue); > + return 0; > +} > + > +/* Called with ssif_bmc->lock held. */ > +static int complete_response(struct ssif_bmc_ctx *ssif_bmc) Could return void. > +{ > + /* Invalidate response in buffer to denote it having been sent. */ > + ssif_bmc->response.len = 0; > + ssif_bmc->response_in_progress = false; > + ssif_bmc->nbytes_processed = 0; > + ssif_bmc->remain_len = 0; > + memset(&ssif_bmc->response_buf, 0, MAX_PAYLOAD_PER_TRANSACTION); > + wake_up_all(&ssif_bmc->wait_queue); > + return 0; > +} > + > +static void set_multipart_response_buffer(struct ssif_bmc_ctx *ssif_bmc, u8 *val) > +{ > + default: > + /* Do not expect to go to this case */ > + pr_err("Error: Unexpected SMBus command received 0x%x\n", ssif_bmc->smbus_cmd); Use dev_err if you can, so the message is associated with the device. > + break; > + } > + > + ssif_bmc->nbytes_processed += response_len; > +} > +
