Re: [PATCH v1 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver

[Joel Stanley]

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