Messages in this thread | | | Subject | Re: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver | From | Jeffrey Hugo <> | Date | Tue, 1 Dec 2020 10:37:00 -0700 |
| |
On 12/1/2020 10:36 AM, Loic Poulain wrote: > On Tue, 1 Dec 2020 at 02:16, Hemant Kumar <hemantk@codeaurora.org> wrote: >> >> 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. > > What Greg complained about is the two kref in the same structure and > that you were using kref as an open() counter. But splitting your > struct in two in order to keep the two kref does not make the much > code better (and simpler). I'm still a bit puzzled about the driver > complexity, it's supposed to be just a passthrough interface to MHI > after all. > > I would suggest several changes, that IMHO would simplify reviewing: > - Use only one structure representing the 'uci' context (uci_dev) > - Keep the read path simple (mhi_uci_read), do no use an intermediate > cur_buf pointer, only dequeue the buffer when it is fully consumed. > - As I commented before, take care of the dl_pending list access > concurrency, even in wait_event. > - You don't need to count the number of open() calls, AFAIK, > mhi_prepare_for_transfer() simply fails if channels are already > started...
Unless I missed something, you seem to have ignored the root issue that Hemant needs to solve, which is when to call mhi_unprepare_for_transfer(). You can't just call that when close() is called because there might be multiple users, and each one is going to trigger a close(), so you need to know how many close() instances to expect, and only call mhi_unprepare_for_transfer() for the last one.
-- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |