Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <72b4fa78-0526-5ff9-c5ed-9301227a7770@foss.st.com>
Date:   Wed, 4 Jan 2023 19:31:24 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH V4 3/3] rpmsg: char: Add TIOCMGET/TIOCMSET ioctl support
Content-Language: en-US
To:     Bjorn Andersson <andersson@kernel.org>
CC:     Sarannya S <quic_sarannya@quicinc.com>,
        <quic_bjorande@quicinc.com>, <swboyd@chromium.org>,
        <quic_clew@quicinc.com>, <mathieu.poirier@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>,
        Deepak Kumar Singh <quic_deesin@quicinc.com>
References: <1670418258-11502-1-git-send-email-quic_sarannya@quicinc.com>
 <1670418258-11502-4-git-send-email-quic_sarannya@quicinc.com>
 <12f53ff1-a358-7129-c9ed-9b9fd7dad7e7@foss.st.com>
 <20221227155641.xlkel7uhk7jr4qru@builder.lan>
 <45444451-26e3-c203-bdeb-59170b6cc5ba@foss.st.com>
 <20230104160331.w2sicwlqiygr42hl@builder.lan>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Organization: STMicroelectronics
In-Reply-To: <20230104160331.w2sicwlqiygr42hl@builder.lan>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


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