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