Re: [PATCH V4 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support

[Arnaud POULIQUEN]

On 12/27/22 16:56, Bjorn Andersson wrote:
> On Wed, Dec 21, 2022 at 05:28:16PM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> On 12/7/22 14:04, Sarannya S 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@xxxxxxxxxxx>
>>> Signed-off-by: Deepak Kumar Singh <quic_deesin@xxxxxxxxxxx>
>>> Signed-off-by: Sarannya S <quic_sarannya@xxxxxxxxxxx>
>>> ---
>>>  drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 52 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>> index 3e0b8f3..8109d18 100644
>>> --- a/drivers/rpmsg/rpmsg_char.c
>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>> @@ -23,6 +23,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>
>>>  
>>> @@ -68,6 +69,8 @@ struct rpmsg_eptdev {
>>>  	struct sk_buff_head queue;
>>>  	wait_queue_head_t readq;
>>>  
>>> +	u32 remote_signals;
>>> +	bool signals_pending;
>>
>> Could you detail the need/use of signals_pending, in your implementation?
>> This is not obvious (at least for me)...
>>
> 
> I agree. With the move to use the concept of flow control in the rpmsg
> API, there's no longer any "signals" in this client driver.
> 
>>>  };
>>>  
>>>  int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>>> @@ -109,7 +112,22 @@ 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_ept_flow_cb(struct rpmsg_device *rpdev, void *priv, bool enable)
>>> +{
>>> +	struct rpmsg_eptdev *eptdev = priv;
>>> +
>>> +	if (enable)
>>> +		eptdev->remote_signals = TIOCM_DSR | TIOCM_CTS;
>>> +	else
>>> +		eptdev->remote_signals = 0;
>>> +
>>> +	eptdev->signals_pending = true;
>>> +
>>>  	wake_up_interruptible(&eptdev->readq);
>>>  
>>>  	return 0;
>>> @@ -146,6 +164,7 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> +	ept->flow_cb = rpmsg_ept_flow_cb;
>>>  	eptdev->ept = ept;
>>>  	filp->private_data = eptdev;
>>>  	mutex_unlock(&eptdev->ept_lock);
>>> @@ -166,6 +185,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>>>  		eptdev->ept = NULL;
>>>  	}
>>>  	mutex_unlock(&eptdev->ept_lock);
>>> +	eptdev->signals_pending = false;
>>>  
>>>  	/* Discard all SKBs */
>>>  	skb_queue_purge(&eptdev->queue);
>>> @@ -279,6 +299,9 @@ static __poll_t rpmsg_eptdev_poll(struct file *filp, poll_table *wait)
>>>  	if (!skb_queue_empty(&eptdev->queue))
>>>  		mask |= EPOLLIN | EPOLLRDNORM;
>>>  
>>> +	if (eptdev->signals_pending)
>>> +		mask |= EPOLLPRI;
>>> +
>>>  	mask |= rpmsg_poll(eptdev->ept, filp, wait);
>>>  
>>>  	return mask;
>>> @@ -289,14 +312,35 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>>>  {
>>>  	struct rpmsg_eptdev *eptdev = fp->private_data;
>>>  
>>> -	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>>> -		return -EINVAL;
>>> -
>>> -	/* Don't allow to destroy a default endpoint. */
>>> -	if (eptdev->default_ept)
>>> -		return -EINVAL;
>>> +	bool set;
>>> +	u32 val;
>>> +	int ret;
>>> +	
>>> +	switch (cmd) {
>>> +	case TIOCMGET:
>>> +		eptdev->signals_pending = false;
>>> +		ret = put_user(eptdev->remote_signals, (int __user *)arg);
>>> +		break;
>>> +	case TIOCMSET:
>>> +		ret = get_user(val, (int __user *)arg);
>>> +		if (ret)
>>> +			break;
>>> +		set = (val & (TIOCM_DTR | TIOCM_RTS)) ? true : false;
>>> +		ret = rpmsg_set_flow_control(eptdev->ept, set, 0);
>>> +		break;
>>
>> I still wonder if it makes sense to implement serial IOCTRL in rpmsg_char.
> 
> I've thinking about this since v1 as well...
> 
>> I think it is quite dangerous to have such kind of mixed interface.
>> User application would want to use the serial interface should use the tty
>> interface.
>>
> 
> Can you please elaborate on this statement, because I have a hard time
> to state why the user space application must use the tty interface
> instead of rpmsg_char.
> 
> And in particular, I don't think this is a question for the "user
> application", but rather for the system configuration.
> 
> In order to move an application that works with rpmsg_char to the tty
> driver ("because it's the right thing to do..."?) means that the system
> needs to be reconfigured, such that the given rpmsg channel is exposed
> through the tty driver instead.
> 
> This in turn either implies that the firmware needs to be changed to
> expose these channels with the name "rpmsg-tty" - and the application
> taught how to figure out which ttyRPMSGn to open - or the rpmsg_ctrl
> interface needs to be extended to allow the Linux side to request a
> particular channel to be exposed as rpmsg_char vs rpmsg-tty...
> 

You are right, it can be not straightforward to migrate to rpmsg_tty. That's why
it also makes sense to implement flow control in the rpmsg char.

What I try to highlight is the use of the RS232 signaling(e.g TIOCM_DTR) and
TIOCMGET/TIOCMSE  terminal IOCTL in this patch.
Please tell me if I wrong, but seems to me that such interface is dedicated to
the serial/TTY frameworks [1].
So does it make sense to reuse this interface for the rpmsg char?

[1]https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/ioctls.h#L8

Instead we could have generic RPMSG IOCTLs that can be implemented on different
rpmsg clients whatever the rpmsg channel (so not only the rpmsg char). This is
the proposal below.

Regards,
Arnaud

>> For the rpmsg char, I would be in favor of creating a specific RPMSG IOCTRLs
>> to avoid confusion.
>>
>> For instance:
>>
>>  - RPMSG_GET_SIGN_IOCTRL
>>  - RPMSG_SET_SIGN_IOCTRL
>>
> 
> Again, we're talking "flow control" at this level. So either we follow
> the standard IOCTL and make it easy for existing applications to use
> rpmsg_char, or we provide a _good_ explanation why they must use the
> tty interface instead (and if so solve above mentioned problems).
> 
> Regards,
> Bjorn
> 
>> With associated parameter corresponding to the bitmap proposed in my comment of
>> your patch 1/4.
>>
>> Of course, this is only a suggestion, I let Bjorn and Mathieu comment.
>>
>> Regards,
>> Arnaud
>>
>>
>>> +	case RPMSG_DESTROY_EPT_IOCTL:
>>> +		/* Don't allow to destroy a default endpoint. */
>>> +		if (eptdev->default_ept) {
>>> +			ret = -EINVAL;
>>> +			break;
>>> +		}
>>> +		ret = rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
>>> +		break;
>>> +	default:
>>> +		ret = -EINVAL;
>>> +	}
>>>  
>>> -	return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
>>> +	return ret;
>>>  }
>>>  
>>>  static const struct file_operations rpmsg_eptdev_fops = {