From: Loic Poulain <loic.poulain@linaro.org>
To: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Hemant Kumar <hemantk@codeaurora.org>,
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>,
Bhaumik Bhatt <bbhatt@codeaurora.org>,
Network Development <netdev@vger.kernel.org>
Subject: Re: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver
Date: Tue, 1 Dec 2020 18:52:57 +0100 [thread overview]
Message-ID: <CAMZdPi-tjmXWAFzZJAg_6U5h2ZJv478E88T-Lmk=YA-B6=MzRA@mail.gmail.com> (raw)
In-Reply-To: <1247e32e-ed67-de6b-81ec-3bde9ad93250@codeaurora.org>
On Tue, 1 Dec 2020 at 18:37, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> 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.
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.
Regards,
Loic
next prev parent reply other threads:[~2020-12-01 17:48 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-28 3:26 [PATCH v13 0/4] userspace MHI client interface driver Hemant Kumar
2020-11-28 3:26 ` [PATCH v13 1/4] bus: mhi: core: Add helper API to return number of free TREs Hemant Kumar
2020-11-28 3:26 ` [PATCH v13 2/4] bus: mhi: core: Move MHI_MAX_MTU to external header file Hemant Kumar
2020-11-28 3:26 ` [PATCH v13 3/4] docs: Add documentation for userspace client interface Hemant Kumar
2020-11-28 3:26 ` [PATCH v13 4/4] bus: mhi: Add userspace client interface driver Hemant Kumar
2020-11-28 6:11 ` Manivannan Sadhasivam
2020-12-01 1:08 ` Hemant Kumar
2020-11-30 18:22 ` Loic Poulain
2020-12-01 1:16 ` Hemant Kumar
2020-12-01 17:36 ` Loic Poulain
2020-12-01 17:37 ` Jeffrey Hugo
2020-12-01 17:52 ` Loic Poulain [this message]
2020-12-01 17:51 ` Jeffrey Hugo
2020-12-01 18:05 ` Loic Poulain
2020-12-01 18:04 ` Jeffrey Hugo
2020-12-01 21:59 ` Hemant Kumar
2020-12-01 19:29 ` [PATCH v13 0/4] userspace MHI " Jakub Kicinski
2020-12-01 19:40 ` Jeffrey Hugo
2020-12-01 20:03 ` Jakub Kicinski
2020-12-01 20:48 ` Jeffrey Hugo
2020-12-02 2:55 ` Jakub Kicinski
2020-12-02 4:59 ` Jeffrey Hugo
2020-12-06 8:33 ` Leon Romanovsky
2020-12-08 16:59 ` Manivannan Sadhasivam
2020-12-08 19:16 ` Leon Romanovsky
2020-12-02 4:15 ` Manivannan Sadhasivam
2020-12-02 4:38 ` Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMZdPi-tjmXWAFzZJAg_6U5h2ZJv478E88T-Lmk=YA-B6=MzRA@mail.gmail.com' \
--to=loic.poulain@linaro.org \
--cc=bbhatt@codeaurora.org \
--cc=gregkh@linuxfoundation.org \
--cc=hemantk@codeaurora.org \
--cc=jhugo@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).