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

\
 
 \ /
  Last update: 2020-12-01 18:39    [W:0.050 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site