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