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

[Bjorn Andersson <andersson@kernel.org>]

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