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

[Arnaud POULIQUEN]

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