On Mon, 29 Mar 2021 at 12:18, Quan Nguyen [off-list ref] 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@os.amperecomputing.com>

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.

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;
+}
+

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel