Message-ID: <57bd8357-3a5d-30e0-293a-eb71d947d3a5@codeaurora.org>
Date:   Tue, 1 Dec 2020 13:59:03 -0800
From:   Hemant Kumar <hemantk@...eaurora.org>
To:     Jeffrey Hugo <jhugo@...eaurora.org>,
        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>,
        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 12/1/20 10:04 AM, Jeffrey Hugo wrote:
> On 12/1/2020 11:05 AM, Loic Poulain wrote:
>> On Tue, 1 Dec 2020 at 18:51, Jeffrey Hugo <jhugo@...eaurora.org> wrote:
>>>
>>> On 12/1/2020 10:52 AM, Loic Poulain wrote:
>>>> On Tue, 1 Dec 2020 at 18:37, Jeffrey Hugo <jhugo@...eaurora.org> wrote:
>>>>>
>>>>> On 12/1/2020 10:36 AM, Loic Poulain wrote:
>>>>>> On Tue, 1 Dec 2020 at 02:16, Hemant Kumar <hemantk@...eaurora.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@...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.
>>>>>>
>>>>>> 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.
>>>>
>>>> That one part of his problem, yes, but if you unconditionally call
>>>> mhi_prepare_for_transfer in open(), it should fail for subsequent
>>>> users, and so only one user will successfully open the device.
>>>
>>> I'm pretty sure that falls under "trying to prevent users from opening a
>>> device" which Greg gave a pretty strong NACK to.  So, that's not a 
>>> solution.
>>
>> That would deserve clarification since other drivers like
>> virtio_console clearly prevent that (guest_connected).
> 
> Quoting Greg from the source - https://lkml.org/lkml/2020/9/17/873
> 
> "
> I told you before, do not try to keep a device node from being opened
> multiple times, as it will always fail (think about passing file handles
> around between programs...)
> 
> If userspace wants to do this, it will do it.  If your driver can't
> handle that, that's fine, userspace will learn not to do that.  But the
> kernel can not prevent this from happening.
> "
> 
>>
>>>
>>> So, the complete problem is -
>>>
>>> N users need to be able to use the device (and by proxy, the channel)
>>> concurrently, but prepare needs to be called on the first user coming
>>> into the picture, and unprepare needs to be called on the last user
>>> exiting the picture.
>>>
>>> Hemant has supported this usecase in every rev of this series I've
>>> looked at, but apparently every solution he has proposed to handle this
>>> has caused confusion.
>>
>> Understood, but that can be done with a simple counter in that case.
If you look at the old original patch set, it was using simple counter 
and Greg suggested to move away from that and use kref object, and then all
incremental changes done using single kref object udev to two separate 
objects udev and uchan (introduced in v7).

Based on every reviewer's comment i thought driver looked cleaner and 
moved away from complicated implementations when it was introduced 
initially. If every one still agrees to that we can move forward with 
its current form which includes valuable suggestions from all the 
reviewers.

Thanks for testing my Change on your setup Loic. Mani wants me to update 
the Documentation patch by modifying the QMI usage example. I am going 
to upload next revision by updating the documentation.

LOOPBACK test application and its inclusion to mhi device id table would 
be a separate patch series. Will share the LOOPBACK test application 
public git hub link in Documentation.

Thanks Jeff for providing the background of the discussions from the past.

Thanks,
Hemant
>>
>> Regards,
>> Loic
>>
> 
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists