On Wed, Oct 21, 2020 at 05:43:14PM +0200, Loic Poulain wrote:
> On Wed, 21 Oct 2020 at 17:27, Manivannan Sadhasivam <
> [email protected]> wrote:
>
> > On Fri, Oct 16, 2020 at 09:04:14PM -0700, Hemant Kumar wrote:
> > > Introduce mhi_get_free_desc_count() API to return number
> >
>
> Would it not be a good idea to have naming aligned with other methods?
> Like mhi_queue_num_free() or mhi_queue_no_free_elem...
>

'queue_num_free' doesn't sound like getting the number of available
descriptors...

Thanks,
Mani

> Loic

2020-10-22 12:03:08

by Hemant Kumar

[permalink] [raw]

Subject: Re: [PATCH v7 4/4] bus: mhi: Add userspace client interface driver

Hi Mani,

On 10/21/20 9:11 AM, Manivannan Sadhasivam wrote:
> 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 <[email protected]>
>> ---
>> 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)
Done.
>
> And please use the caps form UCI in comments throughout the driver.
Done. In commit text : "uci device object", "uci channel object" and
"uci device file node" shall we change these as well ?
>
>> + 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 ().
Done.
>
>> +
>> +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.
This was added because checkpatch --strict required to add a comment
when lock is added to struct, after adding inline comment, checkpatch
error was gone.
>
>> + 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".
Done
>
>> + * @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.
Done.
>
>> + 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?
Added this based on the comment from idr_alloc API
* The caller should provide their own locking to ensure that two
* concurrent modifications to the IDR are not possible.
>
>> + 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.
Done.
>
> Thanks,
> Mani
>
Thanks,
Hemant
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project