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?
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.
[...]
+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