Message-ID: <c47dcd57-7576-e03e-f70b-0c4d25f724b5@codeaurora.org>
Date:   Mon, 30 Nov 2020 17:16:45 -0800
From:   Hemant Kumar <hemantk@...eaurora.org>
To:     Loic Poulain <loic.poulain@...aro.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        Bhaumik Bhatt <bbhatt@...eaurora.org>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver

Hi Loic,

On 11/30/20 10:22 AM, Loic Poulain wrote:
> On Sat, 28 Nov 2020 at 04:26, Hemant Kumar <hemantk@...eaurora.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

Powered by blists - more mailing lists