| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201021161156.GE3334@Mani-XPS-13-9360>
Date: Wed, 21 Oct 2020 21:41:56 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Hemant Kumar <hemantk@...eaurora.org>
Cc: gregkh@...uxfoundation.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, jhugo@...eaurora.org,
bbhatt@...eaurora.org, loic.poulain@...aro.org
Subject: Re: [PATCH v7 4/4] bus: mhi: Add userspace client interface driver
On Fri, Oct 16, 2020 at 09:04:17PM -0700, Hemant Kumar wrote:
> This MHI client driver allows userspace clients to transfer
> raw data between MHI device and host using standard file operations.
> Driver instantiates uci device object which is associated to device
> file node. uci device object instantiates uci channel object when device
> file node is opened. uci channel object is used to manage MHI channels
> by calling MHI core APIs for read and write operations. MHI channels
> are started as part of device open(). MHI channels remain in start
> state until last release() is called on uci device file node. Device
> file node is created with format
>
> /dev/mhi_<controller_name>_<mhi_device_name>
>
> Currently it supports LOOPBACK channel.
>
> Signed-off-by: Hemant Kumar <hemantk@...eaurora.org>
> ---
> drivers/bus/mhi/Kconfig | 13 +
> drivers/bus/mhi/Makefile | 4 +
> drivers/bus/mhi/uci.c | 656 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 673 insertions(+)
> create mode 100644 drivers/bus/mhi/uci.c
>
> diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
> index e841c10..3891b31 100644
> --- a/drivers/bus/mhi/Kconfig
> +++ b/drivers/bus/mhi/Kconfig
> @@ -20,3 +20,16 @@ config MHI_BUS_DEBUG
> Enable debugfs support for use with the MHI transport. Allows
> reading and/or modifying some values within the MHI controller
> for debug and test purposes.
> +
> +config MHI_UCI
> + tristate "MHI UCI"
> + depends on MHI_BUS
> + help
> + MHI based userspace client interface driver is used for transferring
Userspace Client Interface (UCI)
And please use the caps form UCI in comments throughout the driver.
> + raw data between host and device using standard file operations from
> + userspace. Open, read, write, and close operations are supported
> + by this driver. Please check mhi_uci_match_table for all supported
> + channels that are exposed to userspace.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called mhi_uci.
> diff --git a/drivers/bus/mhi/Makefile b/drivers/bus/mhi/Makefile
> index 19e6443..80feefb 100644
> --- a/drivers/bus/mhi/Makefile
> +++ b/drivers/bus/mhi/Makefile
> @@ -1,2 +1,6 @@
> # core layer
> obj-y += core/
> +
> +# MHI client
> +mhi_uci-y := uci.o
> +obj-$(CONFIG_MHI_UCI) += mhi_uci.o
> diff --git a/drivers/bus/mhi/uci.c b/drivers/bus/mhi/uci.c
> new file mode 100644
> index 0000000..8334836
> --- /dev/null
> +++ b/drivers/bus/mhi/uci.c
> @@ -0,0 +1,656 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2018-2020, The Linux Foundation. All rights reserved.*/
> +
> +#include <linux/kernel.h>
> +#include <linux/mhi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +
> +#define DEVICE_NAME "mhi"
> +#define MHI_UCI_DRIVER_NAME "mhi_uci"
> +#define MAX_UCI_MINORS (128)
No need of ().
> +
> +static DEFINE_IDR(uci_idr);
> +static DEFINE_MUTEX(uci_drv_mutex);
> +static struct class *uci_dev_class;
> +static int uci_dev_major;
> +
> +/**
> + * struct uci_chan - MHI channel for a uci device
> + * @udev: associated uci device object
> + * @ul_wq: wait queue for writer
> + * @write_lock: mutex write lock for ul channel
> + * @dl_wq: wait queue for reader
> + * @read_lock: mutex read lock for dl channel
> + * @dl_lock: spin lock
> + * @pending: list of dl buffers userspace is waiting to read
> + * @cur_buf: current buffer userspace is reading
> + * @dl_size: size of the current dl buffer userspace is reading
> + * @ref_count: uci_chan reference count
> + */
> +struct uci_chan {
> + struct uci_dev *udev;
> + wait_queue_head_t ul_wq;
> +
> + /* ul channel lock to synchronize multiple writes */
Please move these inline comments to Kdoc.
> + struct mutex write_lock;
> +
> + wait_queue_head_t dl_wq;
> +
> + /* dl channel lock to synchronize multiple reads */
> + struct mutex read_lock;
> +
> + /*
> + * protects pending and cur_buf members in bh context, channel release,
> + * read and poll
> + */
> + spinlock_t dl_lock;
> +
> + struct list_head pending;
> + struct uci_buf *cur_buf;
> + size_t dl_size;
> + struct kref ref_count;
> +};
> +
> +/**
> + * struct uci_buf - uci buffer
> + * @data: data buffer
> + * @len: length of data buffer
> + * @node: list node of the uci buffer
> + */
> +struct uci_buf {
> + void *data;
> + size_t len;
> + struct list_head node;
> +};
> +
> +/**
> + * struct uci_dev - MHI uci device
> + * @minor: uci device node minor number
> + * @mhi_dev: associated mhi device object
> + * @uchan: uci uplink and downlink channel object
> + * @mtu: max TRE buffer length
> + * @enabled: uci device probed
Use something like, "Flag to track the state of the UCI device".
> + * @lock: mutex lock to manage uchan object
> + * @ref_count: uci_dev reference count
> + */
> +struct uci_dev {
> + unsigned int minor;
> + struct mhi_device *mhi_dev;
> + struct uci_chan *uchan;
> + size_t mtu;
> + bool enabled;
> +
> + /* synchronize open, release and driver remove */
> + struct mutex lock;
> + struct kref ref_count;
> +};
> +
[...]
> +
> +static int mhi_uci_dev_start_chan(struct uci_dev *udev)
> +{
> + int ret = 0;
> + struct uci_chan *uchan;
> +
> + mutex_lock(&udev->lock);
> + if (!udev->uchan || !kref_get_unless_zero(&udev->uchan->ref_count)) {
> + uchan = kzalloc(sizeof(*uchan), GFP_KERNEL);
> + if (!uchan) {
> + ret = -ENOMEM;
> + goto error_chan_start;
> + }
> +
> + udev->uchan = uchan;
> + uchan->udev = udev;
> + init_waitqueue_head(&uchan->ul_wq);
> + init_waitqueue_head(&uchan->dl_wq);
> + mutex_init(&uchan->write_lock);
> + mutex_init(&uchan->read_lock);
> + spin_lock_init(&uchan->dl_lock);
> + INIT_LIST_HEAD(&uchan->pending);
> +
> + ret = mhi_prepare_for_transfer(udev->mhi_dev);
> + if (ret) {
> + dev_err(&udev->mhi_dev->dev, "Error starting transfer channels\n");
> + goto error_chan_cleanup;
> + }
> +
> + ret = mhi_queue_inbound(udev);
> + if (ret)
> + goto error_chan_cleanup;
> +
> + kref_init(&uchan->ref_count);
> + }
> +
> + mutex_unlock(&udev->lock);
Please leave a new line before return.
> + return 0;
> +
> +error_chan_cleanup:
> + mhi_uci_dev_chan_release(&uchan->ref_count);
> +error_chan_start:
> + mutex_unlock(&udev->lock);
> + return ret;
> +}
> +
[...]
> +
> +static int mhi_uci_probe(struct mhi_device *mhi_dev,
> + const struct mhi_device_id *id)
> +{
> + struct uci_dev *udev;
> + struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> + struct device *dev;
> + int index;
> +
> + udev = kzalloc(sizeof(*udev), GFP_KERNEL);
> + if (!udev)
> + return -ENOMEM;
> +
> + kref_init(&udev->ref_count);
> + mutex_init(&udev->lock);
> + udev->mhi_dev = mhi_dev;
> +
> + mutex_lock(&uci_drv_mutex);
Do we really need the lock here?
> + index = idr_alloc(&uci_idr, udev, 0, MAX_UCI_MINORS, GFP_KERNEL);
> + mutex_unlock(&uci_drv_mutex);
> + if (index < 0) {
> + kfree(udev);
> + return index;
> + }
> +
> + udev->minor = index;
> +
> + udev->mtu = min_t(size_t, id->driver_data, MHI_MAX_MTU);
> + dev_set_drvdata(&mhi_dev->dev, udev);
> + udev->enabled = true;
> +
> + /* create device file node /dev/mhi_<cntrl_dev_name>_<mhi_dev_name> */
> + dev = device_create(uci_dev_class, &mhi_dev->dev,
> + MKDEV(uci_dev_major, index), udev,
> + DEVICE_NAME "_%s_%s",
> + dev_name(mhi_cntrl->cntrl_dev), mhi_dev->name);
> + if (IS_ERR(dev)) {
> + mutex_lock(&uci_drv_mutex);
> + idr_remove(&uci_idr, udev->minor);
> + mutex_unlock(&uci_drv_mutex);
> + dev_set_drvdata(&mhi_dev->dev, NULL);
> + kfree(udev);
> + return PTR_ERR(dev);
> + }
> +
> + dev_dbg(&mhi_dev->dev, "probed uci dev: minor %d\n", index);
> +
> + return 0;
> +};
> +
> +static void mhi_uci_remove(struct mhi_device *mhi_dev)
> +{
> + struct uci_dev *udev = dev_get_drvdata(&mhi_dev->dev);
> +
> + /* disable the node */
> + mutex_lock(&udev->lock);
> + udev->enabled = false;
> +
> + /* delete the node to prevent new opens */
> + device_destroy(uci_dev_class, MKDEV(uci_dev_major, udev->minor));
> +
> + /* return error for any blocked read or write */
> + if (udev->uchan) {
> + wake_up(&udev->uchan->ul_wq);
> + wake_up(&udev->uchan->dl_wq);
> + }
> + mutex_unlock(&udev->lock);
> +
> + mutex_lock(&uci_drv_mutex);
> + idr_remove(&uci_idr, udev->minor);
> + kref_put(&udev->ref_count, mhi_uci_dev_release);
> + mutex_unlock(&uci_drv_mutex);
> +}
> +
> +/* .driver_data stores max mtu */
> +static const struct mhi_device_id mhi_uci_match_table[] = {
> + { .chan = "LOOPBACK", .driver_data = 0x1000},
> + {},
> +};
> +MODULE_DEVICE_TABLE(mhi, mhi_uci_match_table);
> +
> +static struct mhi_driver mhi_uci_driver = {
> + .id_table = mhi_uci_match_table,
> + .remove = mhi_uci_remove,
> + .probe = mhi_uci_probe,
> + .ul_xfer_cb = mhi_ul_xfer_cb,
> + .dl_xfer_cb = mhi_dl_xfer_cb,
> + .driver = {
> + .name = MHI_UCI_DRIVER_NAME,
> + },
> +};
> +
> +static int mhi_uci_init(void)
> +{
> + int ret;
> +
> + ret = register_chrdev(0, MHI_UCI_DRIVER_NAME, &mhidev_fops);
> + if (ret < 0)
> + return ret;
> +
> + uci_dev_major = ret;
> + uci_dev_class = class_create(THIS_MODULE, MHI_UCI_DRIVER_NAME);
> + if (IS_ERR(uci_dev_class)) {
> + unregister_chrdev(uci_dev_major, MHI_UCI_DRIVER_NAME);
Use an error path for cleaning this.
Thanks,
Mani
Powered by blists - more mailing lists