linux-kernel - Re: [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <33334ab5-1dff-b637-17c1-2a92f209b6d6@quicinc.com>
Date:   Tue, 29 Mar 2022 17:55:36 +0530
From:   Deepak Kumar Singh <quic_deesin@...cinc.com>
To:     Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>,
        <bjorn.andersson@...aro.org>, <swboyd@...omium.org>,
        <quic_clew@...cinc.com>, <mathieu.poirier@...aro.org>
CC:     <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>,
        Ohad Ben-Cohen <ohad@...ery.com>
Subject: Re: [PATCH V2 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support


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@...cinc.com>
>> Signed-off-by: Deepak Kumar Singh <quic_deesin@...cinc.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).

> 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.
>   
> 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 = {