On Sat, 28 Nov 2020 at 04:26, Hemant Kumar <hemantk@xxxxxxxxxxxxxx> 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
