From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
To: Deepak Kumar Singh <quic_deesin@quicinc.com>,
<bjorn.andersson@linaro.org>, <swboyd@chromium.org>,
<quic_clew@quicinc.com>, <mathieu.poirier@linaro.org>
Cc: <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
<linux-remoteproc@vger.kernel.org>,
Ohad Ben-Cohen <ohad@wizery.com>
Subject: Re: [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
Date: Fri, 1 Apr 2022 15:54:34 +0200 [thread overview]
Message-ID: <2740dcf9-a455-c9a0-d780-e3ff62b2894c@foss.st.com> (raw)
In-Reply-To: <33334ab5-1dff-b637-17c1-2a92f209b6d6@quicinc.com>
On 3/29/22 14:25, Deepak Kumar Singh wrote:
>
> On 3/23/2022 7:08 PM, Arnaud POULIQUEN wrote:
>>
>> On 1/18/22 20:43, Deepak Kumar Singh wrote:
>>> Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
>>> to get/set the low level transport signals.
>>>
>>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
>>> ---
>>> drivers/rpmsg/rpmsg_char.c | 47
>>> ++++++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 43 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> index b5907b8..c03a118 100644
>>> --- a/drivers/rpmsg/rpmsg_char.c
>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>> @@ -19,6 +19,7 @@
>>> #include <linux/rpmsg.h>
>>> #include <linux/skbuff.h>
>>> #include <linux/slab.h>
>>> +#include <linux/termios.h>
>>> #include <linux/uaccess.h>
>>> #include <uapi/linux/rpmsg.h>
>>> @@ -74,6 +75,9 @@ struct rpmsg_eptdev {
>>> spinlock_t queue_lock;
>>> struct sk_buff_head queue;
>>> wait_queue_head_t readq;
>>> +
>>> + u32 rsigs;
>>> + bool sig_pending;
>>> };
>>> static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>>> @@ -112,7 +116,18 @@ static int rpmsg_ept_cb(struct rpmsg_device
>>> *rpdev, void *buf, int len,
>>> skb_queue_tail(&eptdev->queue, skb);
>>> spin_unlock(&eptdev->queue_lock);
>>> - /* wake up any blocking processes, waiting for new data */
>>> + wake_up_interruptible(&eptdev->readq);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rpmsg_sigs_cb(struct rpmsg_device *rpdev, void *priv, u32
>>> sigs)
>>> +{
>>> + struct rpmsg_eptdev *eptdev = priv;
>>> +
>>> + eptdev->rsigs = sigs;
>>> + eptdev->sig_pending = true;
>>> +
>>> wake_up_interruptible(&eptdev->readq);
>> Regarding the Glink code, the callback is used to be informed that the
>> remote
>> is ready to send (DSR) and to receive (CTS or DSR)
>> So I suppose that the transmission should also be conditioned by the
>> sig_pending
>
> I think client need to get signal value before starting transmission, so
> that it knows that
>
> it good to transmit data. Also it is not be enforced for every client.
> Some clients may not require
>
> to use signalling/flow control.
>
>>
>> That said tell me if I'm wrong but look to me that what is implemented
>> here is the
>> hardware flow control already managed by the TTY interface. What
>> about using the
>> TTY interface in this case?
>
> Correct. But some clients are using rpmsg char driver directly and don't
> go through tty interface.
>
> So we are incorporating tty like interface here(flow control).
This is the point I would like to highlight to be sure that it is the
good direction.
From my windows I would say if application wants a basic link it uses
the rpmsg_char, if it wants more complex link with TIOCMGET and
TIOCMSET signaling it should migrate on rmsg tty as the link is now
available (so implementing signaling in rpmsg_tty instead of rpmsg char).
Anyway here I only share my opinion. It is not my role to give the direction.
>
>> And What about using the "software flow control" instead? [1]
>>
>> [1] https://en.wikipedia.org/wiki/Software_flow_control
>>
>>> return 0;
>>> @@ -137,6 +152,7 @@ static int rpmsg_eptdev_open(struct inode *inode,
>>> struct file *filp)
>>> return -EINVAL;
>>> }
>>> + ept->sig_cb = rpmsg_sigs_cb;
>>> eptdev->ept = ept;
>>> filp->private_data = eptdev;
>>> @@ -155,6 +171,7 @@ static int rpmsg_eptdev_release(struct inode
>>> *inode, struct file *filp)
>>> eptdev->ept = NULL;
>>> }
>>> mutex_unlock(&eptdev->ept_lock);
>>> + eptdev->sig_pending = false;
>>> /* Discard all SKBs */
>>> skb_queue_purge(&eptdev->queue);
>>> @@ -265,6 +282,9 @@ static __poll_t rpmsg_eptdev_poll(struct file
>>> *filp, poll_table *wait)
>>> if (!skb_queue_empty(&eptdev->queue))
>>> mask |= EPOLLIN | EPOLLRDNORM;
>>> + if (eptdev->sig_pending)
>>> + mask |= EPOLLPRI;
>>> +
>>> mask |= rpmsg_poll(eptdev->ept, filp, wait);
>>> return mask;
>>> @@ -274,11 +294,30 @@ static long rpmsg_eptdev_ioctl(struct file *fp,
>>> unsigned int cmd,
>>> unsigned long arg)
>>> {
>>> struct rpmsg_eptdev *eptdev = fp->private_data;
>>> + bool set;
>>> + u32 val;
>>> + int ret;
>>> - if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>>> - return -EINVAL;
>>> + switch (cmd) {
>>> + case TIOCMGET:
>>> + eptdev->sig_pending = false;
>>> + ret = put_user(eptdev->rsigs, (int __user *)arg);
>>> + break;
>>> + case TIOCMSET:
>>> + ret = get_user(val, (int __user *)arg);
>>> + if (ret)
>>> + break;
>>> + set = (val & TIOCM_DTR) ? true : false;
>>> + ret = rpmsg_set_flow_control(eptdev->ept, set);
>>> + break;
>> Could this directly be handled by the driver on open close?
>> If application wants to suspend the link it could just close de
>> /dev/rpmsgX.
> All clients may not require setting flow control.
Agree, but this could be conditioned by rpdrv->signals, right?
And this could avoid to expose controls
- in open/close the rpmsg_set_flow_control would be called,
- in rpmsg_eptdev_write_iter an error would be returned ( or 0)
if remote side has suspended the transmission.
But perhaps you need more that a ON/OFF flow control?
Regards,
Arnaud
>> Regards,
>> Arnaud
>>
>>> + case RPMSG_DESTROY_EPT_IOCTL:
>>> + ret = rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + }
>>> - return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>>> + return ret;
>>> }
>>> static const struct file_operations rpmsg_eptdev_fops = {
next prev parent reply other threads:[~2022-04-01 13:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-18 19:43 [PATCH V2 0/3] rpmsg and glink signaling API support Deepak Kumar Singh
2022-01-18 19:43 ` [PATCH V2 1/3] rpmsg: core: Add signal " Deepak Kumar Singh
2022-03-11 21:11 ` Bjorn Andersson
2022-03-23 10:57 ` Arnaud POULIQUEN
2022-03-29 11:00 ` Deepak Kumar Singh
2022-04-01 13:23 ` Arnaud POULIQUEN
2022-01-18 19:43 ` [PATCH V2 2/3] rpmsg: glink: Add support to handle signals command Deepak Kumar Singh
2022-03-11 21:09 ` Bjorn Andersson
2022-03-23 7:20 ` Deepak Kumar Singh
2022-04-01 13:27 ` Arnaud POULIQUEN
2022-01-18 19:43 ` [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support Deepak Kumar Singh
2022-03-23 13:38 ` Arnaud POULIQUEN
2022-03-29 12:25 ` Deepak Kumar Singh
2022-04-01 13:54 ` Arnaud POULIQUEN [this message]
2022-03-23 10:17 ` [PATCH V2 0/3] rpmsg and glink signaling API support Arnaud POULIQUEN
2022-04-25 8:59 ` Arnaud POULIQUEN
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=2740dcf9-a455-c9a0-d780-e3ff62b2894c@foss.st.com \
--to=arnaud.pouliquen@foss.st.com \
--cc=bjorn.andersson@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=mathieu.poirier@linaro.org \
--cc=ohad@wizery.com \
--cc=quic_clew@quicinc.com \
--cc=quic_deesin@quicinc.com \
--cc=swboyd@chromium.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).