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

Commit Message

Sarannya S Dec. 7, 2022, 1:04 p.m. UTC

Add TICOMGET and TIOCMSET ioctl support for rpmsg char device nodes
to get/set the low level transport signals.

Signed-off-by: Chris Lew <[e-mail]>
Signed-off-by: Deepak Kumar Singh <[e-mail]>
Signed-off-by: Sarannya S <[e-mail]>
---
 drivers/rpmsg/rpmsg_char.c | 60 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 8 deletions(-)

Comments

Arnaud POULIQUEN Dec. 21, 2022, 4:28 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Deepak Kumar Singh <[e-mail]>
> Signed-off-by: Sarannya S <[e-mail]>
> ---
>  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)...

>  };
>  
>  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 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.

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

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

Bjorn Andersson Dec. 27, 2022, 3:56 p.m. UTC | #2

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 <[e-mail]>
> > Signed-off-by: Deepak Kumar Singh <[e-mail]>
> > Signed-off-by: Sarannya S <[e-mail]>
> > ---
> >  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...

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

Arnaud POULIQUEN Jan. 3, 2023, 2:50 p.m. UTC | #3

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 <[e-mail]>
>>> Signed-off-by: Deepak Kumar Singh <[e-mail]>
>>> Signed-off-by: Sarannya S <[e-mail]>
>>> ---
>>>  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 = {

Bjorn Andersson Jan. 4, 2023, 4:03 p.m. UTC | #4

On Tue, Jan 03, 2023 at 03:50:10PM +0100, Arnaud POULIQUEN wrote:
> 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:
[..]
> >>>  	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?
> 

We're in understanding of the usefulness and the question about the
validity of reusing the tty's TIOCM{GET,SET} ioctl here. I don't know
the answer to the latter, and haven't pushed on this point.

> [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.
> 

Using a new pair of rpmsg_char ioctls for "set/get flow enable/disable"
would, IMHO, be easier to understand and it would avoid assumptions
inherited about all the other bits in the TIOCMSET ioctl.

Regards,
Bjorn

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

Arnaud POULIQUEN Jan. 4, 2023, 6:31 p.m. UTC | #5

On 1/4/23 17:03, Bjorn Andersson wrote:
> On Tue, Jan 03, 2023 at 03:50:10PM +0100, Arnaud POULIQUEN wrote:
>> 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:
> [..]
>>>>>  	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?
>>
> 
> We're in understanding of the usefulness and the question about the
> validity of reusing the tty's TIOCM{GET,SET} ioctl here. I don't know
> the answer to the latter, and haven't pushed on this point.
> 
>> [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.
>>
> 
> Using a new pair of rpmsg_char ioctls for "set/get flow enable/disable"
> would, IMHO, be easier to understand and it would avoid assumptions
> inherited about all the other bits in the TIOCMSET ioctl.

This also seems to me the best approach

Regards,
Arnaud


> 
> Regards,
> Bjorn
> 
>> 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 = {

Patch

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;
 };
 
 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;
+	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 = {