lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver
    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?

    [...]

    > +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?

    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

    \
     
     \ /
      Last update: 2020-11-30 19:18    [W:3.057 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site