lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver
From
Date
Hi Loic,

On 11/30/20 10:22 AM, Loic Poulain wrote:
> On Sat, 28 Nov 2020 at 04:26, Hemant Kumar <hemantk@codeaurora.org> 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
>
> [...]
>
>> +struct uci_chan {
>> + struct uci_dev *udev;
>> + wait_queue_head_t ul_wq;
>> +
>> + /* ul channel lock to synchronize multiple writes */
>> + struct mutex write_lock;
>> +
>> + wait_queue_head_t dl_wq;
>> +
>> + /* dl channel lock to synchronize multiple reads */
>> + struct mutex read_lock;
>> +
>> + /*
>> + * protects pending list in bh context, channel release, read and
>> + * poll
>> + */
>> + spinlock_t dl_pending_lock;
>> +
>> + struct list_head dl_pending;
>> + struct uci_buf *cur_buf;
>> + size_t dl_size;
>> + struct kref ref_count;
>> +};
>
> [...]
>
>> + * 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: 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;
>
> Why a pointer to uci_chan and not just plainly integrating the
> structure here, AFAIU uci_chan describes the channels and is just a
> subpart of uci_dev. That would reduce the number of dynamic
> allocations you manage and the extra kref. do you even need a separate
> structure for this?

This goes back to one of my patch versions i tried to address concern
from Greg. Since we need to ref count the channel as well as the uci
device i decoupled the two objects and used two reference counts for two
different objects.

Copying from V7 patch history

V7:
- Decoupled uci device and uci channel objects. uci device is
associated with device file node. uci channel is associated
with MHI channels. uci device refers to uci channel to perform
MHI channel operations for device file operations like read()
and write(). uci device increments its reference count for
every open(). uci device calls mhi_uci_dev_start_chan() to start
the MHI channel. uci channel object is tracking number of times
MHI channel is referred. This allows to keep the MHI channel in
start state until last release() is called. After that uci channel
reference count goes to 0 and uci channel clean up is performed
which stops the MHI channel. After the last call to release() if
driver is removed uci reference count becomes 0 and uci object is
cleaned up.

>
> [...]
>
>> +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)) {
>
> This test is suspicious, kref_get_unless_zero should be enough to test, right?
kref_get_unless_zero is de-referencing uchan->ref_count for the first
open uchan is set to NULL, for that NULL check is added for uchan.
>
> if (kref_get_unless_zero(&udev->ref))
> goto skip_init;
>
>> + 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_pending_lock);
>> + INIT_LIST_HEAD(&uchan->dl_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);
>> +
>> + return 0;
>> +
>> +error_chan_cleanup:
>> + mhi_uci_dev_chan_release(&uchan->ref_count);
>> +error_chan_start:
>> + mutex_unlock(&udev->lock);
>> + return ret;
>> +}
>
> Regards,
> Loic
>

Thanks,
Hemant
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

\
 
 \ /
  Last update: 2020-12-01 02:18    [W:0.114 / U:0.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site